-
Notifications
You must be signed in to change notification settings - Fork 0
feat(ui): refreshHighlights, onHighlightError, sign-out guard on highlights #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
866e713
cd3aa25
23b3847
74c1d1a
916b636
05aa013
95aa618
d4ef5fb
75a3811
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| --- | ||
| '@youversion/platform-react-native-expo-ui': minor | ||
| --- | ||
|
|
||
| YPE-104 UI deltas on the highlights stack. | ||
|
|
||
| ## BibleReader | ||
|
|
||
| - **`refreshHighlights()` ref handle** — call `reader.current?.refreshHighlights()` to re-fetch highlights for the reader's current scope (for example after a screen refocus). | ||
| - **`onHighlightError(error)`** — optional callback for offline or queued highlight writes. Fires for `{ status: 'queued' }` and `{ status: 'error', reason: 'transient' }` only; auth, invalid, ok, and noop outcomes stay silent. The `HighlightWriteError` type is exported from the UI package. | ||
|
|
||
| ## Sign-out guard | ||
|
|
||
| - **`BibleReader`** and **`YouVersionAuthButton`** now ask before signing out, matching the Swift SDK. When the highlight write queue still holds unsent work, the copy escalates to "Save your highlights?"; confirming calls `signOut()` only — core clears the queue and cache on sign-out. | ||
| - **Web bypass** — on `Platform.OS === 'web'`, both surfaces call `signOut()` directly because React Native Web's `Alert.alert` is a no-op. | ||
| - **`useSignOutGuard`** is exported for apps that need the same confirmation on their own sign-out UI. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| import type { HighlightWriteOutcome } from '@youversion/platform-react-native-expo-core' | ||
|
|
||
| import { | ||
| reportHighlightWriteError, | ||
| type HighlightWriteError, | ||
| } from '../report-highlight-write-error' | ||
|
|
||
| type AssertQueuedHasNoReason = Extract< | ||
| HighlightWriteError, | ||
| { status: 'queued' } | ||
| > extends { reason?: unknown } | ||
| ? never | ||
| : true | ||
|
|
||
| const assertQueuedHasNoReason: AssertQueuedHasNoReason = true | ||
| void assertQueuedHasNoReason | ||
|
|
||
| describe('reportHighlightWriteError', () => { | ||
| it('fires for queued outcomes', () => { | ||
| const onHighlightError = jest.fn() | ||
|
|
||
| reportHighlightWriteError({ status: 'queued', verses: [1, 2] }, onHighlightError) | ||
|
|
||
| expect(onHighlightError).toHaveBeenCalledWith({ status: 'queued', verses: [1, 2] }) | ||
| }) | ||
|
|
||
| it('fires for transient error outcomes', () => { | ||
| const onHighlightError = jest.fn() | ||
|
|
||
| reportHighlightWriteError( | ||
| { | ||
| status: 'error', | ||
| reason: 'transient', | ||
| message: 'Network request failed', | ||
| failedVerses: [1, 2], | ||
| succeededVerses: [], | ||
| }, | ||
| onHighlightError, | ||
| ) | ||
|
|
||
| expect(onHighlightError).toHaveBeenCalledWith({ | ||
| status: 'error', | ||
| reason: 'transient', | ||
| verses: [1, 2], | ||
| message: 'Network request failed', | ||
| }) | ||
| }) | ||
|
|
||
| it('does nothing when no handler is passed', () => { | ||
| expect(() => | ||
| reportHighlightWriteError({ status: 'queued', verses: [1, 2] }), | ||
| ).not.toThrow() | ||
| }) | ||
|
|
||
| it('swallows a throwing onHighlightError callback', () => { | ||
| const onHighlightError = jest.fn(() => { | ||
| throw new Error('consumer blew up') | ||
| }) | ||
| const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined) | ||
|
|
||
| expect(() => | ||
| reportHighlightWriteError({ status: 'queued', verses: [1, 2] }, onHighlightError), | ||
| ).not.toThrow() | ||
| expect(onHighlightError).toHaveBeenCalledTimes(1) | ||
| expect(consoleError).toHaveBeenCalledWith('onHighlightError failed:', expect.any(Error)) | ||
|
|
||
| consoleError.mockRestore() | ||
| }) | ||
|
|
||
| it('swallows a rejected async onHighlightError callback', async () => { | ||
| const onHighlightError = jest.fn(async () => { | ||
| throw new Error('async consumer blew up') | ||
| }) | ||
| const consoleError = jest.spyOn(console, 'error').mockImplementation(() => undefined) | ||
|
|
||
| expect(() => | ||
| reportHighlightWriteError({ status: 'queued', verses: [1, 2] }, onHighlightError), | ||
| ).not.toThrow() | ||
| expect(onHighlightError).toHaveBeenCalledTimes(1) | ||
|
|
||
| await Promise.resolve() | ||
| expect(consoleError).toHaveBeenCalledWith('onHighlightError failed:', expect.any(Error)) | ||
|
|
||
| consoleError.mockRestore() | ||
| }) | ||
|
|
||
| it.each([ | ||
| ['ok', { status: 'ok', verses: [1, 2] } satisfies HighlightWriteOutcome], | ||
| ['noop', { status: 'noop' } satisfies HighlightWriteOutcome], | ||
| [ | ||
| 'invalid', | ||
| { | ||
| status: 'error', | ||
| reason: 'invalid', | ||
| message: 'Unsupported highlight color.', | ||
| failedVerses: [1, 2], | ||
| succeededVerses: [], | ||
| } satisfies HighlightWriteOutcome, | ||
| ], | ||
| [ | ||
| 'auth', | ||
| { | ||
| status: 'error', | ||
| reason: 'auth', | ||
| message: 'Request failed with status 403', | ||
| failedVerses: [1, 2], | ||
| succeededVerses: [], | ||
| } satisfies HighlightWriteOutcome, | ||
| ], | ||
| [ | ||
| 'not-signed-in', | ||
| { | ||
| status: 'error', | ||
| reason: 'not-signed-in', | ||
| message: 'Not signed in', | ||
| failedVerses: [1, 2], | ||
| succeededVerses: [], | ||
| } satisfies HighlightWriteOutcome, | ||
| ], | ||
| ] as const)('does not fire for %s outcomes', (_label, outcome) => { | ||
| const onHighlightError = jest.fn() | ||
|
|
||
| reportHighlightWriteError(outcome, onHighlightError) | ||
|
|
||
| expect(onHighlightError).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('queued member has no reason field at the type level', () => { | ||
| // @ts-expect-error — queued outcomes never carry reason | ||
| const illegal: HighlightWriteError = { status: 'queued', reason: 'transient', verses: [1] } | ||
| void illegal | ||
| }) | ||
| }) | ||
|
cameronapak marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import type { HighlightWriteOutcome } from '@youversion/platform-react-native-expo-core' | ||
|
|
||
| /** | ||
| * Consumer-facing slice of a highlight write outcome. Fired only for offline or | ||
| * queued writes — not auth, invalid, ok, or noop. | ||
| */ | ||
| export type HighlightWriteError = | ||
| | { status: 'queued'; verses: number[] } | ||
| | { status: 'error'; reason: 'transient'; verses: number[]; message?: string } | ||
|
|
||
| function invokeHighlightErrorHandler( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: This name only says that the function calls the handler. The body isolates a throw from the consumer. A name that states that policy (for example Code Reviewer bot, sent on behalf of Cam. |
||
| onHighlightError: (error: HighlightWriteError) => void, | ||
| error: HighlightWriteError, | ||
| ): void { | ||
| try { | ||
| void Promise.resolve(onHighlightError(error)).catch((err) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): This rejection branch is unreachable through the public type. The prop is typed |
||
| console.error('onHighlightError failed:', err) | ||
| }) | ||
| } catch (err) { | ||
| console.error('onHighlightError failed:', err) | ||
| } | ||
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
|
|
||
| export function reportHighlightWriteError( | ||
| outcome: HighlightWriteOutcome, | ||
| onHighlightError?: (error: HighlightWriteError) => void, | ||
| ): void { | ||
| if (onHighlightError === undefined) { | ||
| return | ||
| } | ||
| if (outcome.status === 'queued') { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: Filter matches the lock — Code Reviewer bot, sent on behalf of Cam. |
||
| invokeHighlightErrorHandler(onHighlightError, { status: 'queued', verses: outcome.verses }) | ||
| return | ||
| } | ||
| if (outcome.status === 'error' && outcome.reason === 'transient') { | ||
| invokeHighlightErrorHandler(onHighlightError, { | ||
| status: 'error', | ||
| reason: 'transient', | ||
| verses: outcome.failedVerses, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question (non-blocking): A partially-failed batch reports only its failures — |
||
| message: outcome.message, | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.