diff --git a/.changeset/biblecard-single-error-alert.md b/.changeset/biblecard-single-error-alert.md new file mode 100644 index 00000000..c5e13397 --- /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 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 52649147..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', @@ -186,32 +187,83 @@ export const Error: Story = { args: { reference: 'LUK.1.39-45', versionId: 111, + showVersionPicker: true, }, 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, ], }, }, 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.', + ); + // 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', { + name: /change bible version/i, + }); + + await waitFor(async () => { + 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 3f58a99e..fa0cefce 100644 --- a/packages/ui/src/components/bible-card.test.tsx +++ b/packages/ui/src/components/bible-card.test.tsx @@ -6,7 +6,7 @@ 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 { usePassage, useTheme, useVersion } from '@youversion/platform-react-hooks'; import type { BiblePassage, BibleVersion } from '@youversion/platform-core'; vi.mock('@youversion/platform-react-hooks'); @@ -156,6 +156,56 @@ describe('BibleCard - Delayed spinner', () => { }); }); +describe('BibleCard - Error state', () => { + 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: Object.assign(new Error('Request failed with status 503'), { status: 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(); + }); + + // 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', () => { 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..3bec741b 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 ? (
@@ -164,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 ? ( { }); }); + it('should render one polite 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); + // 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'); + }); + it('should prioritize 5xx errors over "not found" text in the message', async () => { const { getByRole } = render(
);