From 95e6c7e97066b2fd7b33a30b72bb6cd870c37c26 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 7 Aug 2026 12:03:18 -0500 Subject: [PATCH 1/6] fix(ui): collapse the BibleCard error state into one alert region A failed passage request rendered two competing role="alert" regions: the "ERROR" label in the header slot and the icon plus status-aware sentence in the body. Screen readers announced both. The body block is now the only alert region. The header label keeps its place and its styling but drops role="alert" and aria-live. The body block drops its redundant aria-live and hides its icon with aria-hidden. No new i18n keys, and no change to how errors are derived. Co-Authored-By: Claude Opus 5 --- .../ui/src/components/bible-card.stories.tsx | 12 +++-- .../ui/src/components/bible-card.test.tsx | 49 +++++++++++++++++++ packages/ui/src/components/bible-card.tsx | 21 ++++++-- packages/ui/src/components/verse.test.tsx | 28 +++++++++++ packages/ui/src/components/verse.tsx | 11 +++-- 5 files changed, 108 insertions(+), 13 deletions(-) diff --git a/packages/ui/src/components/bible-card.stories.tsx b/packages/ui/src/components/bible-card.stories.tsx index 52649147..75d419c7 100644 --- a/packages/ui/src/components/bible-card.stories.tsx +++ b/packages/ui/src/components/bible-card.stories.tsx @@ -206,12 +206,16 @@ export const Error: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); + // The header slot carries the "Error" label; the body block is the one alert. await waitFor(async () => { await expect(canvas.getByRole('heading', { level: 2, name: /error/i })).toBeInTheDocument(); - const errorMessages = canvas.getAllByText( - 'The Bible service is having trouble right now. Please try again in a moment.', - ); - await expect(errorMessages.length).toBeGreaterThan(0); }); + + const alerts = canvas.getAllByRole('alert'); + + await expect(alerts).toHaveLength(1); + await expect(alerts[0]).toHaveTextContent( + 'The Bible service is having trouble right now. Please try again in a moment.', + ); }, }; diff --git a/packages/ui/src/components/bible-card.test.tsx b/packages/ui/src/components/bible-card.test.tsx index 3f58a99e..7ab6f707 100644 --- a/packages/ui/src/components/bible-card.test.tsx +++ b/packages/ui/src/components/bible-card.test.tsx @@ -156,6 +156,55 @@ describe('BibleCard - Delayed spinner', () => { }); }); +describe('BibleCard - Error state', () => { + function createError(message: string, status?: number): Error { + return Object.assign(new Error(message), status === undefined ? {} : { status }); + } + + beforeEach(() => { + vi.mocked(useTheme).mockReturnValue('light'); + vi.mocked(useVersion).mockReturnValue({ + version: mockVersion, + loading: false, + error: null, + refetch: vi.fn(), + }); + vi.mocked(usePassage).mockReturnValue({ + passage: null, + loading: false, + error: createError('Request failed with status 503', 503), + refetch: vi.fn(), + }); + }); + + it('should render exactly one alert region', () => { + const { container } = render(); + + expect(within(container).getAllByRole('alert')).toHaveLength(1); + }); + + it('should show the status message in that one alert region', () => { + const { container } = render(); + const alert = within(container).getByRole('alert'); + + expect(alert).toHaveTextContent( + 'The Bible service is having trouble right now. Please try again in a moment.', + ); + }); + + it('should render the error heading in the header slot', () => { + const { container } = render(); + + expect(within(container).getByRole('heading', { level: 2 })).toHaveTextContent('Error'); + }); + + it('should not render a loading spinner while an error is set', () => { + const { container } = render(); + + expect(within(container).queryByRole('status')).toBeNull(); + }); +}); + describe('BibleCard - onFootnotePress callback', () => { const mockPassageWithFootnote: BiblePassage = { id: 'JHN.1', diff --git a/packages/ui/src/components/bible-card.tsx b/packages/ui/src/components/bible-card.tsx index f505b457..596eb1d8 100644 --- a/packages/ui/src/components/bible-card.tsx +++ b/packages/ui/src/components/bible-card.tsx @@ -26,14 +26,21 @@ export type BibleCardProps = { onFootnotePress?: (data: FootnoteData) => void; }; +/** + * The "Error" label for the header slot. + * + * It matches `BibleCardHeaderReference` exactly. The card already renders an + * `

` in this slot for the passage reference, so this injects no new heading + * level into the host page's outline. It carries no `role="alert"` and no + * `aria-live`: the body block stays the single alert region, so screen readers + * announce one alert. + */ function BibleCardHeaderError(): React.ReactNode { const { t } = useTranslation(undefined, { i18n }); return ( -
-

- {t('errorHeading')} -

-
+

+ {t('errorHeading')} +

); } @@ -151,6 +158,10 @@ export function BibleCard({ >
+ {/* + The error branch stays separate rather than folding into the loading + branch, which would spin forever on error. + */} {passage && !passageError ? (
diff --git a/packages/ui/src/components/verse.test.tsx b/packages/ui/src/components/verse.test.tsx index ece936e3..9bebc023 100644 --- a/packages/ui/src/components/verse.test.tsx +++ b/packages/ui/src/components/verse.test.tsx @@ -1044,6 +1044,34 @@ describe('BibleTextView - Error messaging', () => { }); }); + it('should render one alert region with a hidden icon and no heading line', async () => { + const { getAllByRole, getByRole } = render( + , + ); + + await waitFor(() => { + expect(getByRole('alert')).toHaveTextContent( + 'The Bible service is having trouble right now. Please try again in a moment.', + ); + }); + + const alert = getByRole('alert'); + + expect(getAllByRole('alert')).toHaveLength(1); + expect(alert).not.toHaveAttribute('aria-live'); + expect(alert.querySelector('svg')).toHaveAttribute('aria-hidden', 'true'); + // Standalone BibleTextView has no header slot, so no "Error" label renders. + expect(alert).not.toHaveTextContent('Error'); + }); + it('should prioritize 5xx errors over "not found" text in the message', async () => { const { getByRole } = render(
); From 52a1b965e02319e89f2853e8bd252a5e59143799 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 7 Aug 2026 12:10:42 -0500 Subject: [PATCH 2/6] fix(ui): keep the BibleCard version picker usable during an error A 404 means the passage is missing from the selected version, and switching versions is the fix. The picker was hidden whenever passageError was set, so the card offered no way out. The picker reads versionNum rather than the passage, so it renders and works while an error is showing. The header row needs no change: the "ERROR" label still sits on the left and the picker on the right. Co-Authored-By: Claude Opus 5 --- .../ui/src/components/bible-card.stories.tsx | 11 +++++ .../ui/src/components/bible-card.test.tsx | 48 ++++++++++++++++++- packages/ui/src/components/bible-card.tsx | 6 ++- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/packages/ui/src/components/bible-card.stories.tsx b/packages/ui/src/components/bible-card.stories.tsx index 75d419c7..ef2a9ad9 100644 --- a/packages/ui/src/components/bible-card.stories.tsx +++ b/packages/ui/src/components/bible-card.stories.tsx @@ -186,6 +186,7 @@ export const Error: Story = { args: { reference: 'LUK.1.39-45', versionId: 111, + showVersionPicker: true, }, tags: ['integration'], parameters: { @@ -217,5 +218,15 @@ export const Error: Story = { await expect(alerts[0]).toHaveTextContent( 'The Bible service is having trouble right now. Please try again in a moment.', ); + + // The picker is the in-card recovery path: a 404 is fixed by switching versions. + const versionPickerButton = await canvas.findByRole('button', { + name: /change bible version/i, + }); + + await waitFor(async () => { + await expect(versionPickerButton).toBeEnabled(); + await expect(versionPickerButton).toHaveTextContent(/NIV/i); + }); }, }; diff --git a/packages/ui/src/components/bible-card.test.tsx b/packages/ui/src/components/bible-card.test.tsx index 7ab6f707..b48b398a 100644 --- a/packages/ui/src/components/bible-card.test.tsx +++ b/packages/ui/src/components/bible-card.test.tsx @@ -6,8 +6,17 @@ import { render, act, within, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { BibleCard } from './bible-card'; import type { FootnoteData } from './verse'; -import { usePassage, useVersion, useTheme } from '@youversion/platform-react-hooks'; -import type { BiblePassage, BibleVersion } from '@youversion/platform-core'; +import { + useFilteredVersions, + useLanguage, + useLanguages, + useOrganizations, + usePassage, + useTheme, + useVersion, + useVersions, +} from '@youversion/platform-react-hooks'; +import type { BiblePassage, BibleVersion, Language } from '@youversion/platform-core'; vi.mock('@youversion/platform-react-hooks'); @@ -203,6 +212,41 @@ describe('BibleCard - Error state', () => { expect(within(container).queryByRole('status')).toBeNull(); }); + + it('should keep the version picker usable so a bad version can be swapped', () => { + // The version picker mounts its own hook tree. The file-level auto-mock + // returns undefined for each one, so give them values here. + vi.mocked(useLanguages).mockReturnValue({ + languages: { data: [] as Language[], next_page_token: null }, + loading: false, + error: null, + refetch: vi.fn(), + }); + vi.mocked(useLanguage).mockReturnValue({ + language: { id: 'en', language: 'English', display_names: { en: 'English' } } as Language, + loading: false, + error: null, + refetch: vi.fn(), + }); + vi.mocked(useVersions).mockReturnValue({ + versions: { data: [], next_page_token: null }, + loading: false, + error: null, + refetch: vi.fn(), + }); + vi.mocked(useFilteredVersions).mockReturnValue([]); + vi.mocked(useOrganizations).mockReturnValue({ organizations: new Map() }); + + const { container } = render( + , + ); + + const picker = within(container).getByRole('button', { name: /change bible version/i }); + + expect(picker).toBeInTheDocument(); + expect(picker).toBeEnabled(); + expect(picker).toHaveTextContent('BSB'); + }); }); describe('BibleCard - onFootnotePress callback', () => { diff --git a/packages/ui/src/components/bible-card.tsx b/packages/ui/src/components/bible-card.tsx index 596eb1d8..3bec741b 100644 --- a/packages/ui/src/components/bible-card.tsx +++ b/packages/ui/src/components/bible-card.tsx @@ -175,7 +175,11 @@ export function BibleCard({ )} - {showVersionPicker && !passageError ? ( + {/* + The picker stays available during an error. A 404 means the passage + is not in the selected version, so switching versions is the fix. + */} + {showVersionPicker ? ( Date: Fri, 7 Aug 2026 12:17:18 -0500 Subject: [PATCH 3/6] chore: add changeset for the BibleCard error state fix Co-Authored-By: Claude Opus 5 --- .changeset/biblecard-single-error-alert.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/biblecard-single-error-alert.md diff --git a/.changeset/biblecard-single-error-alert.md b/.changeset/biblecard-single-error-alert.md new file mode 100644 index 00000000..7f05a1a5 --- /dev/null +++ b/.changeset/biblecard-single-error-alert.md @@ -0,0 +1,7 @@ +--- +'@youversion/platform-react-ui': patch +--- + +Fix the `BibleCard` error state announcing two alerts, and keep the version picker usable while an error is showing. The "Error" label stays in the header slot but drops its `role="alert"` and `aria-live`, leaving the message block in the card body as the only alert region. The picker no longer disappears on error, so a 404 has an in-card fix: switch to a version that carries the passage. + +The shared message block also drops a redundant `aria-live` and hides its icon with `aria-hidden`, so `VerseOfTheDay` and standalone `BibleTextView` pick up the same accessibility fixes. Their visible text is unchanged, and neither gains an "Error" label. The eight status-aware messages, their six locales, and how errors are derived are untouched. From 9ba2dc2c6840f7cf8f04a39734de5ec9bb4d3d4b Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Tue, 11 Aug 2026 09:32:16 -0500 Subject: [PATCH 4/6] fix(ui): address review feedback on the BibleCard error state Keep aria-live="polite" on VerseUnavailableMessage. role="alert" implies assertive, so the attribute was an override, not a duplicate. Removing it would have moved VerseOfTheDay and standalone BibleTextView from polite to assertive announcements. Spread globalHandlers back into the Error story's msw handlers. A story-level handlers array replaces the preview-level one, so mounting the version picker sent the languages and versions requests to the live API. Extend the Error story's play function through the recovery path: switch to a version whose passage resolves and assert the alert clears. Drop the jsdom picker test that hand-mocked five BibleVersionPicker internals; the story covers it. Move createError into a shared test helper. Co-Authored-By: Claude Opus 5 --- .changeset/biblecard-single-error-alert.md | 2 +- .../ui/src/components/bible-card.stories.tsx | 49 ++++++++++++++-- .../ui/src/components/bible-card.test.tsx | 56 +++---------------- packages/ui/src/components/verse.test.tsx | 11 ++-- packages/ui/src/components/verse.tsx | 10 +++- packages/ui/src/test/errors.ts | 8 +++ 6 files changed, 72 insertions(+), 64 deletions(-) create mode 100644 packages/ui/src/test/errors.ts diff --git a/.changeset/biblecard-single-error-alert.md b/.changeset/biblecard-single-error-alert.md index 7f05a1a5..c5e13397 100644 --- a/.changeset/biblecard-single-error-alert.md +++ b/.changeset/biblecard-single-error-alert.md @@ -4,4 +4,4 @@ Fix the `BibleCard` error state announcing two alerts, and keep the version picker usable while an error is showing. The "Error" label stays in the header slot but drops its `role="alert"` and `aria-live`, leaving the message block in the card body as the only alert region. The picker no longer disappears on error, so a 404 has an in-card fix: switch to a version that carries the passage. -The shared message block also drops a redundant `aria-live` and hides its icon with `aria-hidden`, so `VerseOfTheDay` and standalone `BibleTextView` pick up the same accessibility fixes. Their visible text is unchanged, and neither gains an "Error" label. The eight status-aware messages, their six locales, and how errors are derived are untouched. +The shared message block now hides its icon with `aria-hidden`, so `VerseOfTheDay` and standalone `BibleTextView` pick up that fix too. Their announcement stays polite and their visible text is unchanged, and neither gains an "Error" label. The eight status-aware messages, their six locales, and how errors are derived are untouched. diff --git a/packages/ui/src/components/bible-card.stories.tsx b/packages/ui/src/components/bible-card.stories.tsx index ef2a9ad9..c1d02529 100644 --- a/packages/ui/src/components/bible-card.stories.tsx +++ b/packages/ui/src/components/bible-card.stories.tsx @@ -2,6 +2,7 @@ import type { Meta, StoryObj } from '@storybook/react-vite'; import { within, expect, userEvent, screen, waitFor } from 'storybook/test'; import { http, HttpResponse } from 'msw'; import { BibleCard } from './bible-card'; +import { globalHandlers } from '../test/mocks/handlers'; const meta = { title: 'Components/BibleCard', @@ -191,16 +192,22 @@ export const Error: Story = { tags: ['integration'], parameters: { msw: { + /* + A story-level `handlers` array replaces the preview-level `globalHandlers` + rather than merging with it, so `globalHandlers` is spread back in. + Without it the version picker's `useLanguages`/`useVersions` calls fall + through to the live API, because `onUnhandledRequest` is 'warn'. + + The 500 override comes first: MSW takes the first matching handler, so it + wins over the successful NIV passage handler in `globalHandlers`. Version + 1588 (AMP) is left on `globalHandlers` and still resolves, which is what + makes the recovery path below testable. + */ handlers: [ - http.get('*/v1/bibles/111', () => { - return HttpResponse.json({ - id: 111, - localized_abbreviation: 'NIV', - }); - }), http.get('*/v1/bibles/111/passages/LUK.1.39-45', () => { return new HttpResponse(null, { status: 500 }); }), + ...globalHandlers, ], }, }, @@ -218,6 +225,9 @@ export const Error: Story = { await expect(alerts[0]).toHaveTextContent( 'The Bible service is having trouble right now. Please try again in a moment.', ); + // role="alert" implies assertive. The explicit polite value holds the + // announcement down, so a failed load does not interrupt the reader. + await expect(alerts[0]).toHaveAttribute('aria-live', 'polite'); // The picker is the in-card recovery path: a 404 is fixed by switching versions. const versionPickerButton = await canvas.findByRole('button', { @@ -228,5 +238,32 @@ export const Error: Story = { await expect(versionPickerButton).toBeEnabled(); await expect(versionPickerButton).toHaveTextContent(/NIV/i); }); + + // Walk the recovery path: switch to a version whose passage resolves. + await userEvent.click(versionPickerButton); + + const dialog = await screen.findByRole('dialog'); + const searchInput = within(dialog).getByRole('textbox', { name: /search bible versions/i }); + + await userEvent.type(searchInput, 'amplified bible'); + + await waitFor(async () => { + const versionList = within(dialog).getByTestId('version-list'); + const versionItems = within(versionList).getAllByRole('listitem'); + await expect(versionItems).toHaveLength(1); + await expect(versionItems[0]).toHaveTextContent(/amplified bible/i); + }); + + await userEvent.click(within(dialog).getByRole('listitem', { name: /amplified bible/i })); + + // The error clears: no alert, and the passage replaces the "Error" heading. + await waitFor(async () => { + await expect(canvas.queryByRole('alert')).toBeNull(); + await expect(canvas.getByText(/at that time mary got ready/i)).toBeInTheDocument(); + }); + + await expect( + canvas.getByRole('heading', { level: 2, name: /luke 1:39-45/i }), + ).toHaveTextContent(/amp/i); }, }; diff --git a/packages/ui/src/components/bible-card.test.tsx b/packages/ui/src/components/bible-card.test.tsx index b48b398a..227f43f6 100644 --- a/packages/ui/src/components/bible-card.test.tsx +++ b/packages/ui/src/components/bible-card.test.tsx @@ -6,17 +6,9 @@ import { render, act, within, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { BibleCard } from './bible-card'; import type { FootnoteData } from './verse'; -import { - useFilteredVersions, - useLanguage, - useLanguages, - useOrganizations, - usePassage, - useTheme, - useVersion, - useVersions, -} from '@youversion/platform-react-hooks'; -import type { BiblePassage, BibleVersion, Language } from '@youversion/platform-core'; +import { usePassage, useTheme, useVersion } from '@youversion/platform-react-hooks'; +import type { BiblePassage, BibleVersion } from '@youversion/platform-core'; +import { createBibleTextError as createError } from '../test/errors'; vi.mock('@youversion/platform-react-hooks'); @@ -166,10 +158,6 @@ describe('BibleCard - Delayed spinner', () => { }); describe('BibleCard - Error state', () => { - function createError(message: string, status?: number): Error { - return Object.assign(new Error(message), status === undefined ? {} : { status }); - } - beforeEach(() => { vi.mocked(useTheme).mockReturnValue('light'); vi.mocked(useVersion).mockReturnValue({ @@ -213,40 +201,10 @@ describe('BibleCard - Error state', () => { expect(within(container).queryByRole('status')).toBeNull(); }); - it('should keep the version picker usable so a bad version can be swapped', () => { - // The version picker mounts its own hook tree. The file-level auto-mock - // returns undefined for each one, so give them values here. - vi.mocked(useLanguages).mockReturnValue({ - languages: { data: [] as Language[], next_page_token: null }, - loading: false, - error: null, - refetch: vi.fn(), - }); - vi.mocked(useLanguage).mockReturnValue({ - language: { id: 'en', language: 'English', display_names: { en: 'English' } } as Language, - loading: false, - error: null, - refetch: vi.fn(), - }); - vi.mocked(useVersions).mockReturnValue({ - versions: { data: [], next_page_token: null }, - loading: false, - error: null, - refetch: vi.fn(), - }); - vi.mocked(useFilteredVersions).mockReturnValue([]); - vi.mocked(useOrganizations).mockReturnValue({ organizations: new Map() }); - - const { container } = render( - , - ); - - const picker = within(container).getByRole('button', { name: /change bible version/i }); - - expect(picker).toBeInTheDocument(); - expect(picker).toBeEnabled(); - expect(picker).toHaveTextContent('BSB'); - }); + // The version picker staying usable during an error is covered by the `Error` + // story's play function. A jsdom test would have to hand-mock the five hooks + // that BibleVersionPicker.Root reads, which couples this file to that + // component's internals. See packages/ui/CLAUDE.md → TESTING. }); describe('BibleCard - onFootnotePress callback', () => { diff --git a/packages/ui/src/components/verse.test.tsx b/packages/ui/src/components/verse.test.tsx index 9bebc023..4e45a151 100644 --- a/packages/ui/src/components/verse.test.tsx +++ b/packages/ui/src/components/verse.test.tsx @@ -7,6 +7,7 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { render, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { Verse, BibleTextView, type BibleTextViewPassageState, type FootnoteData } from './verse'; +import { createBibleTextError as createError } from '../test/errors'; // BibleTextView always calls usePassage/useTheme internally (even when passageState // is provided), so we must mock the hooks to avoid requiring YouVersionProvider. @@ -885,10 +886,6 @@ describe('BibleTextView - Refetch loading behavior', () => { describe('BibleTextView - Error messaging', () => { const originalNavigator = globalThis.navigator; - function createError(message: string, status?: number): Error { - return Object.assign(new Error(message), status === undefined ? {} : { status }); - } - afterEach(() => { Object.defineProperty(globalThis, 'navigator', { configurable: true, @@ -1044,7 +1041,7 @@ describe('BibleTextView - Error messaging', () => { }); }); - it('should render one alert region with a hidden icon and no heading line', async () => { + it('should render one polite alert region with a hidden icon and no heading line', async () => { const { getAllByRole, getByRole } = render( { const alert = getByRole('alert'); expect(getAllByRole('alert')).toHaveLength(1); - expect(alert).not.toHaveAttribute('aria-live'); + // role="alert" implies assertive, so this attribute is what keeps the + // announcement polite. Removing it would be a behavior change. + expect(alert).toHaveAttribute('aria-live', 'polite'); expect(alert.querySelector('svg')).toHaveAttribute('aria-hidden', 'true'); // Standalone BibleTextView has no header slot, so no "Error" label renders. expect(alert).not.toHaveTextContent('Error'); diff --git a/packages/ui/src/components/verse.tsx b/packages/ui/src/components/verse.tsx index 0a7a4a98..e6c7b471 100644 --- a/packages/ui/src/components/verse.tsx +++ b/packages/ui/src/components/verse.tsx @@ -255,13 +255,19 @@ const VerseFootnoteButton = memo(function VerseFootnoteButton({ * exclamation icon and the status-aware message. * * The "Error" label lives in the BibleCard header slot, not here, so this block - * stays a single sentence. `role="alert"` already implies an assertive live - * region, so no `aria-live` is set, and the icon is hidden from screen readers. + * stays a single sentence. + * + * `role="alert"` implies `aria-live="assertive"`, so the explicit + * `aria-live="polite"` is not redundant: it holds the announcement down to + * polite. Keep it. `VerseOfTheDay` renders this block on page load, and an + * assertive announcement would interrupt whatever the screen reader is saying. + * The icon is decorative, so it is hidden from screen readers. */ function VerseUnavailableMessage({ message }: { message: string }): React.ReactElement { return (