From 24237c38bceef8dcc476d3c82d6c90cbd452d50b Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 13:20:28 -0500 Subject: [PATCH 1/7] docs(ype-1034): ADR + glossary for server-only highlights Co-Authored-By: Claude Fable 5 --- CONTEXT.md | 85 +++++++++++++++++++++ docs/adr/YPE-1034-highlights-server-only.md | 52 +++++++++++++ docs/adr/YPE-642-verse-action-popover.md | 3 + 3 files changed, 140 insertions(+) create mode 100644 CONTEXT.md create mode 100644 docs/adr/YPE-1034-highlights-server-only.md diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 00000000..f6702d7b --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,85 @@ +# Ubiquitous Language + +Glossary of domain terms for the YouVersion Platform SDK. Terms here are +canonical: code, docs, and conversation should use them exactly. + +## Highlight + +A user-owned color marking on a Bible passage, stored on the user's +YouVersion account. Highlights are **account data, not device data**: they +require an authenticated user and are never persisted locally by the SDK +(decided in YPE-1034, superseding the temporary localStorage store from +YPE-642 / ADR-001). + +Identified by the pair (**Bible version**, **passage**). A highlight has +exactly one **color**. + +## Passage + +A verse or contiguous verse range in one chapter of one Bible version, +identified by a USFM string (`JHN.3.16`, `JHN.3.16-18`). A chapter USFM +(`JHN.3`) is a passage *scope* used for querying, not a highlightable unit. + +## Bible version + +A translation/edition of the Bible, identified by a numeric id. The SDK +calls this `version_id`; the highlights wire API calls the same value +`bible_id`. The SDK name is canonical in public types; mapping happens at +the API boundary only. + +## Color + +A highlight's fill, a 6-character lowercase hex string without `#` +(`fff9b1`). Uppercase input is accepted and normalized at the API boundary. + +## Verse selection + +The ephemeral set of verses the reader has tapped in `BibleReader`. Drives +the verse action popover. Never persisted; cleared on navigation. + +## Self-contained mode + +The default `BibleReader` posture (YPE-1034): the reader fetches and writes +highlights itself through the SDK's own auth session. The color row is +always offered; tapping a color when the user lacks a session or the +highlights permission enters the highlight auth flow (July 9 2026 sync, +superseding the earlier hide-when-signed-out idea). + +## Highlights permission + +The per-app grant that authorizes highlight reads/writes for a user. It is +**not an OIDC scope**: it travels as `requested_permissions[]=highlights` +alongside `scope` at authorize time (PR #280) and is granted via a data +exchange consent. Permissions are open-ended strings, never enums (more +arrive later, e.g. verse notes). The **server is the source of truth** for +whether it is granted; any client-side permission cache is optimistic only. + +## Pending highlight + +The user's stashed highlight intent (verses + color + timestamp) while the +highlight auth flow is in flight. Survives a redirect round-trip via +sessionStorage; expires stale (~10 min) so an abandoned round-trip can +never apply a highlight during a later sign-in. Discarded on decline, +cancel, or failure. + +## Highlight auth flow + +The state machine (see the React Web auth state machine doc) that turns a +color tap into an applied highlight across two user paths: one-fell-swoop +(sign-in requesting the highlights permission together) and just-in-time +(already signed in, permission confirm dialog → data exchange grant). +Cancellation at any point keeps the verse selection intact and discards +only the pending highlight. + +## Controlled mode + +A planned `BibleReader` posture (YPE-3705): the host application supplies +rendered highlights as data and receives highlight-intent events, and the +reader makes no highlight network calls. The host owns auth, persistence, +and conflict rules. Used by native hosts (RN Expo SDK) embedding the web +reader. + +## Verse action popover + +The floating action bar (YPE-642) that appears over a verse selection, +offering highlight colors, copy, and share. diff --git a/docs/adr/YPE-1034-highlights-server-only.md b/docs/adr/YPE-1034-highlights-server-only.md new file mode 100644 index 00000000..326cecf3 --- /dev/null +++ b/docs/adr/YPE-1034-highlights-server-only.md @@ -0,0 +1,52 @@ +# YPE-1034 — Wire the highlights API into BibleReader + +Status: **Decided** (grilling session 2026-07-10, Cam + Dustin) +Component: `packages/ui/src/components/bible-reader.tsx` (+ hooks `useHighlights`) +Inputs: July 9 2026 highlights sync (Notion: "React Web SDK Highlights: +Implementation Brief"), auth state machine doc, YPE-642 gotchas doc, +Swift reference PR platform-sdk-swift#179. + +## ADR-001 — Highlights are server-only; the localStorage store is removed + +**Supersedes ADR-001 in [YPE-642](./YPE-642-verse-action-popover.md).** + +### Decision + +The localStorage highlight store (`youversion-platform:highlights:`) +is deleted outright — no migration, no signed-out fallback. Highlights are +fetched from and written to `/v1/highlights` exclusively, through the SDK's +authenticated session. A user with no session (or whose app lacks the +`highlights` permission) enters the highlight auth flow when they tap a color; +their intent is stashed as a **pending highlight** (sessionStorage, ~10-minute +expiry), never as a persisted local highlight. + +The only local traces of highlight state are: +- the in-memory optimistic overlay while a write is in flight, +- the pending highlight during an auth round-trip, +- an optimistic localStorage cache of the *permission* grant (not highlight + data), which the server can invalidate at any time via 401/403. + +### Why + +- Highlights are account data. A browser-profile copy silently diverges from + the user's YouVersion account and dies at the browser boundary. +- YPE-642's store was explicitly a stand-in for this ticket (its ADR-001 said + "server sync is a separate ticket" — this is that ticket). +- The SDK is pre-1.0 with few consumers; the migration code for weeks-old + throwaway data would outlive its usefulness. +- Two persistence paths (API + local) double the state machine and create an + unanswerable merge question on sign-in. + +### Consequences + +- Sign-out immediately un-renders all highlights. +- Signed-out readers see no highlights; that is correct, not a regression. +- The RN Expo / native-host story (YPE-3705) supplies highlights via + controlled props instead — it does not resurrect local persistence. + +### Alternatives rejected + +- **localStorage for signed-out + API for signed-in:** merge-on-sign-in + conflicts, doubled state machine, data that still dies per-device. +- **One-time migration of existing local highlights:** permanent code for + transient data nobody has accumulated meaningfully. diff --git a/docs/adr/YPE-642-verse-action-popover.md b/docs/adr/YPE-642-verse-action-popover.md index ec13acf8..97a2376b 100644 --- a/docs/adr/YPE-642-verse-action-popover.md +++ b/docs/adr/YPE-642-verse-action-popover.md @@ -30,6 +30,9 @@ gone.** #131's `VerseActionPopover` (correct AC logic, tested, already uses Radi ## Decisions (ADRs) ### ADR-001 — localStorage only this PR +> **Superseded** by [YPE-1034 ADR-001](./YPE-1034-highlights-server-only.md): +> highlights are server-only; the localStorage store is removed. + Highlights persist client-side only. Server sync is a **separate ticket**. No network, no API client this PR. From 3fb8f0ab1a527e537e68d2ae39405b5a90a63fbe Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 13:31:40 -0500 Subject: [PATCH 2/7] feat(ui): wire BibleReader highlights to the highlights API behind a dark-launch flag Replace the localStorage highlight store (YPE-642 stand-in) with real API wiring per YPE-1034 ADR-001: highlights are server-only account data, deleted locally with no migration. - New internal useBibleReaderHighlights seam: fetches the current chapter's highlights via useHighlights (chapter USFM passage_id, keyed on version), renders through an in-memory optimistic overlay, reverts + console.error on write failure (toasts and 401/403 handling are PR 2) - Contiguous verse runs collapse to range USFMs on the wire (usfm-ranges.ts, mirroring the verse-share run-grouping idiom); colors sent as lowercase hex - Internal HIGHLIGHTS_LIVE dark-launch flag (off, not exported from the package entry) gates fetches, writes, and rendering; setHighlightsLive() is the test/Storybook-only override - Auth-gated: reads YouVersionAuthContext directly (now exported from hooks) so a missing auth provider degrades to signed-out instead of the useYVAuth throw; sign-out un-renders highlights immediately - better-result added to the ui package for error typing at the new seam's write boundary; core's throwing clients are untouched Co-Authored-By: Claude Fable 5 --- .changeset/bible-reader-highlights-api.md | 11 + packages/hooks/src/context/index.ts | 4 + packages/ui/package.json | 3 +- packages/ui/src/components/bible-reader.tsx | 77 +---- .../use-bible-reader-highlights.test.tsx | 323 ++++++++++++++++++ .../components/use-bible-reader-highlights.ts | 230 +++++++++++++ packages/ui/src/lib/feature-flags.ts | 31 ++ packages/ui/src/lib/usfm-ranges.test.ts | 46 +++ packages/ui/src/lib/usfm-ranges.ts | 41 +++ pnpm-lock.yaml | 8 + 10 files changed, 711 insertions(+), 63 deletions(-) create mode 100644 .changeset/bible-reader-highlights-api.md create mode 100644 packages/ui/src/components/use-bible-reader-highlights.test.tsx create mode 100644 packages/ui/src/components/use-bible-reader-highlights.ts create mode 100644 packages/ui/src/lib/feature-flags.ts create mode 100644 packages/ui/src/lib/usfm-ranges.test.ts create mode 100644 packages/ui/src/lib/usfm-ranges.ts diff --git a/.changeset/bible-reader-highlights-api.md b/.changeset/bible-reader-highlights-api.md new file mode 100644 index 00000000..e70f655b --- /dev/null +++ b/.changeset/bible-reader-highlights-api.md @@ -0,0 +1,11 @@ +--- +'@youversion/platform-react-hooks': patch +'@youversion/platform-react-ui': patch +--- + +BibleReader highlights now wire to the highlights API behind an internal dark-launch flag; the localStorage highlight store is removed. + +- Highlights are server-only account data (YPE-1034): the temporary `youversion-platform:highlights:` localStorage store is deleted outright, with no migration. +- A new internal `useBibleReaderHighlights` seam fetches the current chapter's highlights via `useHighlights`, renders them through an in-memory optimistic overlay (reverted on write failure), and collapses contiguous verse selections into range USFMs (e.g. `JHN.3.16-18`) on the wire. +- Everything is gated on an internal `HIGHLIGHTS_LIVE` flag (currently off) plus an authenticated session: while off or signed out, the color row is inert — no fetches, no writes, no rendered highlights. Signing out un-renders highlights immediately. +- `@youversion/platform-react-hooks` now exports `YouVersionAuthContext`, the no-throw alternative to `useYVAuth` for components that must tolerate a missing auth provider (its error message already advertised the export). diff --git a/packages/hooks/src/context/index.ts b/packages/hooks/src/context/index.ts index e2183601..6f4da546 100644 --- a/packages/hooks/src/context/index.ts +++ b/packages/hooks/src/context/index.ts @@ -1,2 +1,6 @@ export * from './YouVersionContext'; export * from './YouVersionProvider'; +// The raw auth context (no-throw alternative to `useYVAuth` for consumers that +// must tolerate a missing auth provider). Its own error message already +// advertises it as importable from this package. +export { YouVersionAuthContext } from './YouVersionAuthContext'; diff --git a/packages/ui/package.json b/packages/ui/package.json index 32284a04..ef6778dc 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -53,12 +53,13 @@ "@radix-ui/react-use-controllable-state": "^1.2.2", "@youversion/platform-core": "workspace:*", "@youversion/platform-react-hooks": "workspace:*", + "better-result": "2.9.2", "class-variance-authority": "0.7.1", "clsx": "2.1.1", - "tailwind-merge": "3.3.1", "i18next": "^26.0.0", "radix-ui": "^1.4.3", "react-i18next": "^17.0.0", + "tailwind-merge": "3.3.1", "tw-animate-css": "1.4.0" }, "peerDependencies": { diff --git a/packages/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index c3d4810e..23388aa2 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -38,6 +38,7 @@ import { PersonIcon } from './icons/person'; import { ProfileAvatar } from './profile-avatar'; import { Button } from './ui/button'; import { Popover, PopoverClose, PopoverContent, PopoverTrigger } from './ui/popover'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; import { VerseActionPopover } from './verse-action-popover'; import { BibleTextView, getCleanVerseText, type FootnoteData } from './verse'; import { buildVerseReference, buildVerseShareText, joinVerseTexts } from '@/lib/verse-share'; @@ -504,40 +505,22 @@ function Content() { }, [book, chapter]); // ---- Verse selection + highlights ------------------------------------------ - // Selection is ephemeral; highlights persist to localStorage only for now - // (ADR-001) in the future `highlight` API shape: keyed by full passage_id USFM - // and scoped by versionId/bible_id (ADR-002). The reader DOM ref lets us anchor - // the popover and pull clean verse text for Copy / Share. + // Selection is ephemeral (ADR-007 in YPE-642). Highlights are server-only + // account data (ADR-001 in YPE-1034): fetched and written through + // useBibleReaderHighlights, dark-launched behind the internal HIGHLIGHTS_LIVE + // flag. The reader DOM ref lets us anchor the popover and pull clean verse + // text for Copy / Share. const readerRef = useRef(null); const [selectedVerses, setSelectedVerses] = useState([]); const [popoverOpen, setPopoverOpen] = useState(false); const [anchorElement, setAnchorElement] = useState(null); const lastSelectionRef = useRef([]); - const [highlightStore, setHighlightStore] = useState>({}); - - const highlightsStorageKey = `youversion-platform:highlights:${versionId}`; - - // Clear the store synchronously (during render) the moment the version key - // changes. The load effect below runs *after* paint, so without this the - // previous version's highlights would paint over the new text for one frame — - // their `${book}.${chapter}.N` keys collide with the new version's verses. - const [loadedHighlightsKey, setLoadedHighlightsKey] = useState(highlightsStorageKey); - if (loadedHighlightsKey !== highlightsStorageKey) { - setLoadedHighlightsKey(highlightsStorageKey); - setHighlightStore({}); - } - // Load this version's highlights when the version changes (client-only). - useEffect(() => { - let data: Record = {}; - try { - const raw = localStorage.getItem(highlightsStorageKey); - if (raw) data = JSON.parse(raw) as Record; - } catch { - // Ignore (unavailable or malformed storage). - } - setHighlightStore(data); - }, [highlightsStorageKey]); + const { + highlightedVerses, + apply: applyHighlight, + remove: removeHighlight, + } = useBibleReaderHighlights({ versionId, book, chapter }); // Navigating away (book/chapter/version) drops the selection — those verses no // longer exist on screen (ADR-007). @@ -548,18 +531,6 @@ function Content() { lastSelectionRef.current = []; }, [book, chapter, versionId]); - // Derive the visible chapter's highlights (verse number → hex) from the store. - const chapterPrefix = `${book}.${chapter}.`; - const highlightedVerses = useMemo(() => { - const map: Record = {}; - for (const [passageId, color] of Object.entries(highlightStore)) { - if (!passageId.startsWith(chapterPrefix)) continue; - const verseNum = parseInt(passageId.slice(chapterPrefix.length), 10); - if (verseNum) map[verseNum] = color; - } - return map; - }, [highlightStore, chapterPrefix]); - // Distinct colors present in the current selection → drives the X (remove) circles. const activeHighlights = useMemo( () => @@ -571,14 +542,6 @@ function Content() { [selectedVerses, highlightedVerses], ); - function persistHighlights(next: Record) { - try { - localStorage.setItem(highlightsStorageKey, JSON.stringify(next)); - } catch { - // Ignore (private mode / quota exceeded). - } - } - function closeAndClearSelection() { setPopoverOpen(false); setSelectedVerses([]); @@ -608,26 +571,16 @@ function Content() { } function handleHighlight(color: string) { - const next = { ...highlightStore }; - for (const verse of selectedVerses) { - next[`${book}.${chapter}.${verse}`] = color; - } - setHighlightStore(next); - persistHighlights(next); + applyHighlight(color, selectedVerses); closeAndClearSelection(); } function handleClearHighlight(color: string) { - const next = { ...highlightStore }; - for (const verse of selectedVerses) { - const passageId = `${book}.${chapter}.${verse}`; - if (next[passageId] === color) delete next[passageId]; - } - setHighlightStore(next); - persistHighlights(next); + removeHighlight(color, selectedVerses); // Multiple colors active → keep open so the user can remove others (AC 8a); - // last color removed → dismiss (AC 8). + // last color removed → dismiss (AC 8). `highlightedVerses` still holds the + // pre-removal snapshot here — the optimistic overlay lands next render. const hasRemaining = selectedVerses.some((verse) => { const current = highlightedVerses[verse]; return current && current !== color; diff --git a/packages/ui/src/components/use-bible-reader-highlights.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx new file mode 100644 index 00000000..80185e6b --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -0,0 +1,323 @@ +/** + * @vitest-environment jsdom + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import type { Collection, Highlight } from '@youversion/platform-core'; +import { useHighlights, YouVersionAuthContext } from '@youversion/platform-react-hooks'; +import type { YouVersionUserInfo } from '@youversion/platform-core'; +import type { ReactNode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; + +vi.mock('@youversion/platform-react-hooks', async () => { + const actual = await vi.importActual('@youversion/platform-react-hooks'); + return { + ...actual, + useHighlights: vi.fn(), + }; +}); + +const mockUserInfo = { id: 'user-1', name: 'Test User' } as unknown as YouVersionUserInfo; + +function makeCollection(data: Highlight[]): Collection { + return { data, next_page_token: null }; +} + +function mockUseHighlights( + overrides: Partial> = {}, +): ReturnType { + const value: ReturnType = { + highlights: makeCollection([]), + loading: false, + error: null, + refetch: vi.fn(), + createHighlight: vi.fn().mockResolvedValue({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }), + deleteHighlight: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; + vi.mocked(useHighlights).mockReturnValue(value); + return value; +} + +/** + * Auth wrapper whose signed-in state can be flipped between rerenders (the + * wrapper re-runs on `rerender()`, picking up the new value). + */ +let signedIn = true; +function AuthWrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} + +const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; + +beforeEach(() => { + vi.clearAllMocks(); + signedIn = true; + setHighlightsLive(true); +}); + +afterEach(() => { + setHighlightsLive(HIGHLIGHTS_LIVE); +}); + +describe('useBibleReaderHighlights — flag off (dark launch)', () => { + it('is fully inert: fetch disabled, empty map, writes are no-ops', () => { + setHighlightsLive(false); + const mocked = mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + // The fetch gate is `enabled: false` — useApiData skips the request entirely. + expect(vi.mocked(useHighlights)).toHaveBeenCalledWith( + { version_id: 111, passage_id: 'JHN.3' }, + { enabled: false }, + ); + // Even stale fetched data must not render while the flag is off. + expect(result.current.highlightedVerses).toEqual({}); + + act(() => { + result.current.apply('fffe00', [16, 17]); + result.current.remove('fffe00', [16]); + }); + expect(mocked.createHighlight).not.toHaveBeenCalled(); + expect(mocked.deleteHighlight).not.toHaveBeenCalled(); + expect(result.current.highlightedVerses).toEqual({}); + }); +}); + +describe('useBibleReaderHighlights — auth guarding', () => { + it('renders without crashing when no auth provider is mounted, treated as signed out', () => { + const mocked = mockUseHighlights(); + + // No wrapper: YouVersionAuthContext is null (useYVAuth would throw here). + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions)); + + expect(vi.mocked(useHighlights)).toHaveBeenCalledWith( + { version_id: 111, passage_id: 'JHN.3' }, + { enabled: false }, + ); + expect(result.current.highlightedVerses).toEqual({}); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(mocked.createHighlight).not.toHaveBeenCalled(); + }); + + it('clears rendered highlights immediately when the user signs out', () => { + mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + }); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Sign out: useApiData keeps its stale data, so the hook itself must gate. + signedIn = false; + rerender(); + expect(result.current.highlightedVerses).toEqual({}); + }); +}); + +describe('useBibleReaderHighlights — fetched highlights', () => { + it('maps per-verse USFMs for the current chapter into the verse map', () => { + mockUseHighlights({ + highlights: makeCollection([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: '5DFF79' }, // uppercase from server + { version_id: 111, passage_id: 'JHN.4.1', color: '00d6ff' }, // other chapter + { version_id: 999, passage_id: 'JHN.3.2', color: '00d6ff' }, // other version + ]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + expect(vi.mocked(useHighlights)).toHaveBeenCalledWith( + { version_id: 111, passage_id: 'JHN.3' }, + { enabled: true }, + ); + expect(result.current.highlightedVerses).toEqual({ + 16: 'fffe00', + 17: '5dff79', + }); + }); +}); + +describe('useBibleReaderHighlights — apply', () => { + it('applies optimistically and POSTs contiguous runs as range USFMs', async () => { + const mocked = mockUseHighlights(); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.apply('FFFE00', [16, 17, 18, 20]); + }); + + // Optimistic: rendered before any network round-trip settles. + expect(result.current.highlightedVerses).toEqual({ + 16: 'fffe00', + 17: 'fffe00', + 18: 'fffe00', + 20: 'fffe00', + }); + + await waitFor(() => { + expect(mocked.createHighlight).toHaveBeenCalledTimes(2); + }); + expect(mocked.createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16-18', + color: 'fffe00', // lowercased on the wire + }); + expect(mocked.createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.20', + color: 'fffe00', + }); + }); + + it('reverts the optimistic overlay and logs when the write fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(vi.fn()); + const mocked = mockUseHighlights({ + createHighlight: vi.fn().mockRejectedValue(new Error('network down')), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.apply('fffe00', [16, 17]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00', 17: 'fffe00' }); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({}); + }); + expect(mocked.createHighlight).toHaveBeenCalledTimes(1); + expect(consoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to apply highlight'), + expect.objectContaining({ passageId: 'JHN.3.16-17' }), + ); + consoleError.mockRestore(); + }); +}); + +describe('useBibleReaderHighlights — remove', () => { + it('removes optimistically and DELETEs only verses rendered in that color, as ranges', async () => { + const mocked = mockUseHighlights({ + highlights: makeCollection([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.18', color: '5dff79' }, // different color, stays + ]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.remove('fffe00', [16, 17, 18]); + }); + + // Optimistic: yellow verses gone immediately, green untouched. + expect(result.current.highlightedVerses).toEqual({ 18: '5dff79' }); + + await waitFor(() => { + expect(mocked.deleteHighlight).toHaveBeenCalledTimes(1); + }); + expect(mocked.deleteHighlight).toHaveBeenCalledWith('JHN.3.16-17', { version_id: 111 }); + }); + + it('reverts the optimistic removal and logs when the delete fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(vi.fn()); + mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + deleteHighlight: vi.fn().mockRejectedValue(new Error('network down')), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.remove('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({}); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); + expect(consoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to remove highlight'), + expect.objectContaining({ passageId: 'JHN.3.16' }), + ); + consoleError.mockRestore(); + }); + + it('is a no-op when no selected verse is rendered in the given color', () => { + const mocked = mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + }); + + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.remove('5dff79', [16]); + }); + expect(mocked.deleteHighlight).not.toHaveBeenCalled(); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); +}); + +describe('useBibleReaderHighlights — scope changes', () => { + it('drops the optimistic overlay when the chapter changes', () => { + const neverSettles = new Promise(vi.fn()); + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(neverSettles), + }); + + const { result, rerender } = renderHook( + (props: { versionId: number; book: string; chapter: string }) => + useBibleReaderHighlights(props), + { wrapper: AuthWrapper, initialProps: defaultOptions }, + ); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + rerender({ versionId: 111, book: 'JHN', chapter: '4' }); + expect(result.current.highlightedVerses).toEqual({}); + }); +}); diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts new file mode 100644 index 00000000..8ce9ebd9 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -0,0 +1,230 @@ +'use client'; + +import { isHighlightsLive } from '@/lib/feature-flags'; +import { buildPassageIds } from '@/lib/usfm-ranges'; +import { useHighlights, YouVersionAuthContext } from '@youversion/platform-react-hooks'; +import { Result } from 'better-result'; +import { useCallback, useContext, useMemo, useRef, useState } from 'react'; + +export type UseBibleReaderHighlightsOptions = { + versionId: number; + book: string; + chapter: string; +}; + +export type UseBibleReaderHighlightsReturn = { + /** Verse number → hex color (lowercase, no `#`) for the current chapter. */ + highlightedVerses: Record; + /** Highlights the given verses in `color`. Bridge-safe: primitives only. */ + apply: (color: string, verses: number[]) => void; + /** Clears the given verses that are currently highlighted in `color`. */ + remove: (color: string, verses: number[]) => void; +}; + +/** + * Optimistic per-verse overlay on top of the fetched highlights: a hex color + * means "optimistically applied", `null` means "optimistically removed". + */ +type HighlightOverlay = Record; + +/** + * The error type this hook's write boundary converts to. Core clients throw; + * we catch here (via better-result) so a failed write can never take the + * reader down — the failure is logged and the optimistic overlay reverted. + */ +class BibleReaderHighlightError extends Error { + readonly operation: 'apply' | 'remove'; + readonly passageId: string; + readonly cause: unknown; + + constructor(operation: 'apply' | 'remove', passageId: string, cause: unknown) { + super(`Highlight ${operation} failed for ${passageId}`); + this.name = 'BibleReaderHighlightError'; + this.operation = operation; + this.passageId = passageId; + this.cause = cause; + } +} + +function snapshotOverlay(overlay: HighlightOverlay, verses: number[]): HighlightOverlay { + const snapshot: HighlightOverlay = {}; + for (const verse of verses) { + if (verse in overlay) snapshot[verse] = overlay[verse]!; + } + return snapshot; +} + +/** + * BibleReader's seam onto the highlights API (YPE-1034, self-contained mode). + * + * Highlights are server-only account data: fetched per chapter through + * `useHighlights`, written as collapsed range USFMs, rendered through an + * in-memory optimistic overlay. There is no local persistence (ADR-001 in + * docs/adr/YPE-1034-highlights-server-only.md). + * + * Everything is gated on `isHighlightsLive() && isAuthenticated`: while the + * dark-launch flag is off, or the user has no session, the hook is inert — no + * fetches, no writes, an empty rendered map. Failures in this PR get a + * `console.error` and an overlay revert only; toasts and 401/403 handling are + * PR 2. + */ +export function useBibleReaderHighlights({ + versionId, + book, + chapter, +}: UseBibleReaderHighlightsOptions): UseBibleReaderHighlightsReturn { + // Read the auth context directly instead of `useYVAuth`, which throws when + // the consumer never mounted an auth provider. No provider and signed out + // are the same state here: no fetch, no writes, nothing rendered. + const authContext = useContext(YouVersionAuthContext); + const isAuthenticated = Boolean(authContext?.userInfo); + const live = isHighlightsLive() && isAuthenticated; + + const chapterUsfm = `${book}.${chapter}`; + const { highlights, createHighlight, deleteHighlight } = useHighlights( + { version_id: versionId, passage_id: chapterUsfm }, + { enabled: live }, + ); + + const [overlay, setOverlay] = useState({}); + + // Drop the optimistic overlay synchronously (during render) the moment the + // scope changes, so an in-flight overlay never paints over another + // chapter's or version's verses — their verse numbers collide. + const overlayScope = `${versionId}:${chapterUsfm}`; + const [loadedOverlayScope, setLoadedOverlayScope] = useState(overlayScope); + if (loadedOverlayScope !== overlayScope) { + setLoadedOverlayScope(overlayScope); + setOverlay({}); + } + + const highlightedVerses = useMemo(() => { + // `useApiData` keeps its last data when disabled, so gate on `live` here + // too: sign-out must un-render highlights immediately, not eventually. + if (!live) return {}; + + const map: Record = {}; + const versePrefix = `${chapterUsfm}.`; + // The API returns one highlight per verse (no ranges) — parse the verse + // number off the chapter prefix and normalize color case for rendering. + for (const highlight of highlights?.data ?? []) { + if (highlight.version_id !== versionId) continue; + if (!highlight.passage_id.startsWith(versePrefix)) continue; + const verse = parseInt(highlight.passage_id.slice(versePrefix.length), 10); + if (verse > 0) map[verse] = highlight.color.toLowerCase(); + } + for (const [verse, color] of Object.entries(overlay)) { + if (color === null) delete map[Number(verse)]; + else map[Number(verse)] = color; + } + return map; + }, [live, highlights, overlay, chapterUsfm, versionId]); + + // Refs so `apply` / `remove` can snapshot current state without re-memoizing + // on every overlay/fetch change. + const overlayRef = useRef(overlay); + overlayRef.current = overlay; + const highlightedVersesRef = useRef(highlightedVerses); + highlightedVersesRef.current = highlightedVerses; + + const patchOverlay = useCallback((verses: number[], value: string | null) => { + setOverlay((current) => { + const next = { ...current }; + for (const verse of verses) next[verse] = value; + return next; + }); + }, []); + + const revertOverlay = useCallback((verses: number[], snapshot: HighlightOverlay) => { + setOverlay((current) => { + const next = { ...current }; + for (const verse of verses) { + if (verse in snapshot) next[verse] = snapshot[verse]!; + else delete next[verse]; + } + return next; + }); + }, []); + + const apply = useCallback( + (color: string, verses: number[]) => { + if (!live || verses.length === 0) return; + + const normalizedColor = color.toLowerCase(); + const snapshot = snapshotOverlay(overlayRef.current, verses); + patchOverlay(verses, normalizedColor); + + void (async () => { + const results = await Promise.all( + buildPassageIds(book, chapter, verses).map((passageId) => + Result.tryPromise({ + try: () => + createHighlight({ + version_id: versionId, + passage_id: passageId, + color: normalizedColor, + }), + catch: (cause) => new BibleReaderHighlightError('apply', passageId, cause), + }), + ), + ); + + const failures = results.filter(Result.isError); + if (failures.length === 0) return; + for (const failure of failures) { + console.error( + `[YouVersion SDK] Failed to apply highlight (version ${versionId}, ` + + `passage ${failure.error.passageId}, color ${normalizedColor})`, + failure.error, + ); + } + // Partial failures revert the whole optimistic batch; the refetch that + // any successful create triggered repaints the server's truth. + revertOverlay(verses, snapshot); + })(); + }, + [live, book, chapter, versionId, createHighlight, patchOverlay, revertOverlay], + ); + + const remove = useCallback( + (color: string, verses: number[]) => { + if (!live || verses.length === 0) return; + + // Only clear verses currently rendered in this color — that's the + // popover's per-color X semantics. (The DELETE endpoint clears any color + // in the range, so the passage ids must not span other colors.) + const normalizedColor = color.toLowerCase(); + const rendered = highlightedVersesRef.current; + const targetVerses = verses.filter((verse) => rendered[verse] === normalizedColor); + if (targetVerses.length === 0) return; + + const snapshot = snapshotOverlay(overlayRef.current, targetVerses); + patchOverlay(targetVerses, null); + + void (async () => { + const results = await Promise.all( + buildPassageIds(book, chapter, targetVerses).map((passageId) => + Result.tryPromise({ + try: () => deleteHighlight(passageId, { version_id: versionId }), + catch: (cause) => new BibleReaderHighlightError('remove', passageId, cause), + }), + ), + ); + + const failures = results.filter(Result.isError); + if (failures.length === 0) return; + for (const failure of failures) { + console.error( + `[YouVersion SDK] Failed to remove highlight (version ${versionId}, ` + + `passage ${failure.error.passageId}, color ${normalizedColor})`, + failure.error, + ); + } + revertOverlay(targetVerses, snapshot); + })(); + }, + [live, book, chapter, versionId, deleteHighlight, patchOverlay, revertOverlay], + ); + + return { highlightedVerses, apply, remove }; +} diff --git a/packages/ui/src/lib/feature-flags.ts b/packages/ui/src/lib/feature-flags.ts new file mode 100644 index 00000000..7c76de52 --- /dev/null +++ b/packages/ui/src/lib/feature-flags.ts @@ -0,0 +1,31 @@ +/** + * Internal dark-launch flags for the UI package. + * + * NOT exported from the package entry point (`src/index.ts`) — these are + * SDK-internal switches, not consumer API. Flip the default here when a + * feature is ready to launch. + */ + +/** + * Dark-launch default for server-backed highlights in BibleReader (YPE-1034). + * While `false`, the reader's highlight color row is inert: no highlight + * fetches, no writes, nothing rendered from the API. + */ +export const HIGHLIGHTS_LIVE = false; + +let highlightsLive = HIGHLIGHTS_LIVE; + +/** Current state of the highlights dark-launch flag. */ +export function isHighlightsLive(): boolean { + return highlightsLive; +} + +/** + * Force the highlights flag on/off. + * + * @internal Test/Storybook-only. Never call from product code — launch by + * changing {@link HIGHLIGHTS_LIVE} instead. + */ +export function setHighlightsLive(value: boolean): void { + highlightsLive = value; +} diff --git a/packages/ui/src/lib/usfm-ranges.test.ts b/packages/ui/src/lib/usfm-ranges.test.ts new file mode 100644 index 00000000..86c00867 --- /dev/null +++ b/packages/ui/src/lib/usfm-ranges.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from 'vitest'; +import { buildPassageIds, collapseVerseRuns } from './usfm-ranges'; + +describe('collapseVerseRuns', () => { + it('returns an empty list for no verses', () => { + expect(collapseVerseRuns([])).toEqual([]); + }); + + it('collapses a fully contiguous selection into one run', () => { + expect(collapseVerseRuns([16, 17, 18])).toEqual([{ start: 16, end: 18 }]); + }); + + it('splits non-contiguous selections into separate runs', () => { + expect(collapseVerseRuns([1, 3, 4, 8])).toEqual([ + { start: 1, end: 1 }, + { start: 3, end: 4 }, + { start: 8, end: 8 }, + ]); + }); + + it('sorts and de-duplicates before collapsing', () => { + expect(collapseVerseRuns([18, 16, 17, 16])).toEqual([{ start: 16, end: 18 }]); + }); + + it('drops non-positive verse numbers', () => { + expect(collapseVerseRuns([0, -1, 2, 3])).toEqual([{ start: 2, end: 3 }]); + }); +}); + +describe('buildPassageIds', () => { + it('emits a single-verse USFM for a run of one', () => { + expect(buildPassageIds('JHN', '3', [16])).toEqual(['JHN.3.16']); + }); + + it('emits a range USFM for a contiguous run', () => { + expect(buildPassageIds('JHN', '3', [16, 17, 18])).toEqual(['JHN.3.16-18']); + }); + + it('emits one passage id per run for mixed selections', () => { + expect(buildPassageIds('JHN', '3', [16, 17, 18, 20])).toEqual(['JHN.3.16-18', 'JHN.3.20']); + }); + + it('returns an empty list for an empty selection', () => { + expect(buildPassageIds('JHN', '3', [])).toEqual([]); + }); +}); diff --git a/packages/ui/src/lib/usfm-ranges.ts b/packages/ui/src/lib/usfm-ranges.ts new file mode 100644 index 00000000..eb306fa4 --- /dev/null +++ b/packages/ui/src/lib/usfm-ranges.ts @@ -0,0 +1,41 @@ +/** + * Collapses verse selections into the range USFMs the highlights API speaks. + * + * The API stores highlights per verse but accepts contiguous ranges on the + * wire (`JHN.3.16-18`), so a 3-verse apply is one POST instead of three. The + * run-grouping mirrors `formatVerseNumbers` in verse-share.ts, but emits USFM + * passage ids instead of a human-readable reference fragment. + */ + +export type VerseRun = { start: number; end: number }; + +/** + * Groups a verse list into contiguous ascending runs: + * `[16,17,18] -> [{16,18}]`, `[1,3,4] -> [{1,1},{3,4}]`. + * De-duplicates and sorts; non-positive verse numbers are dropped. + */ +export function collapseVerseRuns(verses: number[]): VerseRun[] { + const sorted = [...new Set(verses)].filter((verse) => verse > 0).sort((a, b) => a - b); + const runs: VerseRun[] = []; + + for (const verse of sorted) { + const current = runs[runs.length - 1]; + if (current && verse === current.end + 1) { + current.end = verse; + } else { + runs.push({ start: verse, end: verse }); + } + } + + return runs; +} + +/** + * Builds one passage-id USFM per contiguous run: + * `("JHN", "3", [16,17,18,20]) -> ["JHN.3.16-18", "JHN.3.20"]`. + */ +export function buildPassageIds(book: string, chapter: string, verses: number[]): string[] { + return collapseVerseRuns(verses).map(({ start, end }) => + start === end ? `${book}.${chapter}.${start}` : `${book}.${chapter}.${start}-${end}`, + ); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 60563bb7..967ba791 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -255,6 +255,9 @@ importers: '@youversion/platform-react-hooks': specifier: workspace:* version: link:../hooks + better-result: + specifier: 2.9.2 + version: 2.9.2 class-variance-authority: specifier: 0.7.1 version: 0.7.1 @@ -3411,6 +3414,9 @@ packages: resolution: {integrity: sha512-pbnl5XzGBdrFU/wT4jqmJVPn2B6UHPBOhzMQkY/SPUPB6QtUXtmBHBIwCbXJol93mOpGMnQyP/+BB19q04xj7g==} engines: {node: '>=4'} + better-result@2.9.2: + resolution: {integrity: sha512-WIFoBPCdnTOdk9inkE1ZRvCZ4P0CpSkAiLlchC65N7n9DcjZ3NhqkBOlafzpOVnO8ixyi37kicmSJ3ENhPZl7Q==} + bidi-js@1.0.3: resolution: {integrity: sha512-RKshQI1R3YQ+n9YJz2QQ147P66ELpa1FQEg20Dk8oW9t2KgLbpDLLp9aGZ7y8WHSshDknG0bknqGw5/tyCs5tw==} @@ -9984,6 +9990,8 @@ snapshots: dependencies: is-windows: 1.0.2 + better-result@2.9.2: {} + bidi-js@1.0.3: dependencies: require-from-string: 2.0.2 From 19295d61d66a6681df4cfe7cf3f31b35a20f4c7e Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 13:47:38 -0500 Subject: [PATCH 3/7] fix(hooks): fetch on enabled flip, clear data on disable, latest-wins responses Adversarial review of the highlights wiring surfaced one blocker and two races, all rooted in useApiData: - The fetch effect ignored enabled transitions: a hook that mounted disabled (auth still resolving) never fetched once enabled flipped true, so a signed-in reader rendered zero highlights until a write or navigation. enabled now rides alongside the caller-supplied deps. - Disabling kept the last response, so stale account data could render across sign-out or a host-controlled auth user switch. Disabling now clears data and error. - refetch-initiated requests escaped cancellation: a stale refetch for a previous chapter resolving late could clobber the new chapter's data. All requests now go through a monotonic sequence; only the latest-issued request may commit state. In the BibleReader seam hook: - Successfully settled writes now hand their verses to a confirmed set whose overlay entries are dropped when the post-write refetch lands, so server truth wins again instead of the optimistic entry masking later remote changes until navigation. - Documented the two remaining apply/remove concurrency windows (in-flight POST vs DELETE ordering, snapshot revert vs concurrent write) at the write boundary; a real operation queue is deliberately PR 2. New tests: useApiData enabled transitions and stale-response handling, and a seam-hook integration test through the real useHighlights/useApiData path (module-level mocks had hidden the enabled-flip bug). Co-Authored-By: Claude Fable 5 --- .changeset/bible-reader-highlights-api.md | 4 + packages/hooks/src/useApiData.test.tsx | 175 ++++++++++++++++++ packages/hooks/src/useApiData.ts | 41 ++-- ...ble-reader-highlights.integration.test.tsx | 107 +++++++++++ .../use-bible-reader-highlights.test.tsx | 39 +++- .../components/use-bible-reader-highlights.ts | 58 +++++- 6 files changed, 405 insertions(+), 19 deletions(-) create mode 100644 packages/hooks/src/useApiData.test.tsx create mode 100644 packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx diff --git a/.changeset/bible-reader-highlights-api.md b/.changeset/bible-reader-highlights-api.md index e70f655b..2fbeb750 100644 --- a/.changeset/bible-reader-highlights-api.md +++ b/.changeset/bible-reader-highlights-api.md @@ -9,3 +9,7 @@ BibleReader highlights now wire to the highlights API behind an internal dark-la - A new internal `useBibleReaderHighlights` seam fetches the current chapter's highlights via `useHighlights`, renders them through an in-memory optimistic overlay (reverted on write failure), and collapses contiguous verse selections into range USFMs (e.g. `JHN.3.16-18`) on the wire. - Everything is gated on an internal `HIGHLIGHTS_LIVE` flag (currently off) plus an authenticated session: while off or signed out, the color row is inert — no fetches, no writes, no rendered highlights. Signing out un-renders highlights immediately. - `@youversion/platform-react-hooks` now exports `YouVersionAuthContext`, the no-throw alternative to `useYVAuth` for components that must tolerate a missing auth provider (its error message already advertised the export). +- `useApiData` (the fetch engine behind all data hooks, e.g. `useHighlights`, `usePassage`) fixes and behavior changes: + - An `enabled` false→true flip now triggers the fetch even when the caller's deps are unchanged — previously a hook that mounted disabled (e.g. waiting on auth) never fetched after being enabled. + - Disabling now clears `data` (and `error`) instead of keeping the last response, so stale account data cannot linger after sign-out or an auth user switch. + - Responses are now latest-wins across `refetch` too: a stale in-flight request (e.g. a refetch for a previous chapter) resolving after a newer fetch no longer overwrites its data. diff --git a/packages/hooks/src/useApiData.test.tsx b/packages/hooks/src/useApiData.test.tsx new file mode 100644 index 00000000..62570284 --- /dev/null +++ b/packages/hooks/src/useApiData.test.tsx @@ -0,0 +1,175 @@ +/** + * @vitest-environment jsdom + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; +import { useApiData, type UseApiDataOptions } from './useApiData'; + +type Deferred = { + promise: Promise; + resolve: (value: T) => void; + reject: (reason: unknown) => void; +}; + +function createDeferred(): Deferred { + let resolve!: (value: T) => void; + let reject!: (reason: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +describe('useApiData — enabled transitions', () => { + it('fetches when enabled flips from false to true with unchanged deps', async () => { + const fetchFn = vi.fn().mockResolvedValue('payload'); + + const { result, rerender } = renderHook( + ({ enabled }: { enabled: boolean }) => useApiData(fetchFn, ['stable-dep'], { enabled }), + { initialProps: { enabled: false } }, + ); + + // Disabled on mount (e.g. auth still resolving): no request goes out. + expect(fetchFn).not.toHaveBeenCalled(); + expect(result.current.loading).toBe(false); + expect(result.current.data).toBeNull(); + + // Auth resolves: enabled flips true while the caller's deps are unchanged. + rerender({ enabled: true }); + + await waitFor(() => { + expect(result.current.data).toBe('payload'); + }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it('clears data (without refetching) when enabled flips from true to false', async () => { + const fetchFn = vi.fn().mockResolvedValue('user-a-data'); + + const { result, rerender } = renderHook( + ({ enabled }: { enabled: boolean }) => useApiData(fetchFn, ['stable-dep'], { enabled }), + { initialProps: { enabled: true } }, + ); + + await waitFor(() => { + expect(result.current.data).toBe('user-a-data'); + }); + + // Sign-out (or auth switching users): stale account data must not linger. + rerender({ enabled: false }); + + expect(result.current.data).toBeNull(); + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(false); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); +}); + +describe('useApiData — stale responses (latest wins)', () => { + it('ignores a stale refetch response that resolves after a newer fetch', async () => { + const deferreds: Deferred[] = []; + const fetchFn = vi.fn(() => { + const deferred = createDeferred(); + deferreds.push(deferred); + return deferred.promise; + }); + + const { result, rerender } = renderHook( + ({ scope }: { scope: string }) => useApiData(fetchFn, [scope]), + { initialProps: { scope: 'JHN.3' } }, + ); + + // Initial fetch for JHN.3 resolves normally. + await act(async () => { + deferreds[0]!.resolve('JHN.3 data'); + await Promise.resolve(); + }); + expect(result.current.data).toBe('JHN.3 data'); + + // A refetch for JHN.3 goes out (e.g. after a highlight write)… + act(() => { + result.current.refetch(); + }); + expect(deferreds).toHaveLength(2); + + // …then the user navigates to JHN.4, whose fetch resolves first. + rerender({ scope: 'JHN.4' }); + expect(deferreds).toHaveLength(3); + await act(async () => { + deferreds[2]!.resolve('JHN.4 data'); + await Promise.resolve(); + }); + expect(result.current.data).toBe('JHN.4 data'); + + // The stale JHN.3 refetch finally lands — it must not clobber JHN.4. + await act(async () => { + deferreds[1]!.resolve('stale JHN.3 data'); + await Promise.resolve(); + }); + expect(result.current.data).toBe('JHN.4 data'); + expect(result.current.loading).toBe(false); + }); + + it('ignores a stale error from an invalidated request', async () => { + const deferreds: Deferred[] = []; + const fetchFn = vi.fn(() => { + const deferred = createDeferred(); + deferreds.push(deferred); + return deferred.promise; + }); + + const { result, rerender } = renderHook( + ({ scope }: { scope: string }) => useApiData(fetchFn, [scope]), + { initialProps: { scope: 'JHN.3' } }, + ); + + // Navigate away while the first request is still in flight. + rerender({ scope: 'JHN.4' }); + await act(async () => { + deferreds[1]!.resolve('JHN.4 data'); + await Promise.resolve(); + }); + expect(result.current.data).toBe('JHN.4 data'); + + // The abandoned JHN.3 request fails late — the error must not surface. + await act(async () => { + deferreds[0]!.reject(new Error('stale failure')); + await Promise.resolve(); + }); + expect(result.current.error).toBeNull(); + expect(result.current.data).toBe('JHN.4 data'); + }); +}); + +describe('useApiData — existing behavior', () => { + it('does not fetch when enabled is false for the whole lifetime', () => { + const fetchFn = vi.fn().mockResolvedValue('never'); + const options: UseApiDataOptions = { enabled: false }; + + const { result } = renderHook(() => useApiData(fetchFn, ['dep'], options)); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(result.current.loading).toBe(false); + expect(result.current.data).toBeNull(); + }); + + it('refetches on demand', async () => { + const fetchFn = vi.fn().mockResolvedValueOnce('first').mockResolvedValueOnce('second'); + + const { result } = renderHook(() => useApiData(fetchFn, ['dep'])); + + await waitFor(() => { + expect(result.current.data).toBe('first'); + }); + + act(() => { + result.current.refetch(); + }); + + await waitFor(() => { + expect(result.current.data).toBe('second'); + }); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/hooks/src/useApiData.ts b/packages/hooks/src/useApiData.ts index 91f95a32..2cdf1a96 100644 --- a/packages/hooks/src/useApiData.ts +++ b/packages/hooks/src/useApiData.ts @@ -1,6 +1,6 @@ 'use client'; -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; export type UseApiDataOptions = { enabled?: boolean; @@ -24,48 +24,63 @@ export function useApiData( const [loading, setLoading] = useState(true); const [error, setError] = useState(null); + // Monotonic sequence per issued request: only the latest-issued request may + // commit state. This covers refetch-initiated requests too, which a + // per-effect cancel closure would miss — a stale refetch (e.g. for a + // previous chapter) resolving after a newer fetch must not overwrite it. + const requestSeqRef = useRef(0); + const fetchData = useCallback(() => { + const requestSeq = ++requestSeqRef.current; + if (!enabled) { + // Disabling drops previously fetched data instead of keeping it: the + // usual reason to disable is that the data must no longer be shown + // (signed out, auth switched to a different user), and leaking stale + // account data across sessions is worse than a refetch on re-enable. + setData(null); setLoading(false); + setError(null); return; } - let canceled = false; - setLoading(true); setError(null); fetchFn() .then((result) => { - if (!canceled) { + if (requestSeq === requestSeqRef.current) { setData(result); } }) .catch((err) => { - if (!canceled) { + if (requestSeq === requestSeqRef.current) { setError(err as Error); } }) .finally(() => { - if (!canceled) { + if (requestSeq === requestSeqRef.current) { setLoading(false); } }); - - return () => { - canceled = true; - }; }, [fetchFn, enabled]); const refetch = useCallback(() => { fetchData(); }, [fetchData]); + // `enabled` rides alongside the caller-supplied deps so a false→true flip + // (e.g. auth resolving after mount, with the caller's deps unchanged) + // actually triggers the fetch. useEffect(() => { - const cleanup = fetchData(); - return cleanup; + fetchData(); + return () => { + // Invalidate any in-flight request (effect- or refetch-initiated) when + // the deps change or the component unmounts. + requestSeqRef.current++; + }; // @eslint-disable-next-line react-hooks/exhaustive-deps - }, deps); + }, [...deps, enabled]); return { data, loading, error, refetch }; } diff --git a/packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx new file mode 100644 index 00000000..e14a7c7c --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx @@ -0,0 +1,107 @@ +/** + * @vitest-environment jsdom + * + * Integration coverage for the seam hook through the REAL `useHighlights` and + * `useApiData` (nothing from the hooks package is module-mocked; only the core + * client's network method is stubbed). This exists because wholesale-mocking + * `useHighlights` hid a real bug: `useApiData`'s fetch effect didn't re-run + * when `enabled` flipped false→true, so a signed-in session resolving after + * mount never fetched highlights at all. + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import { HighlightsClient, type YouVersionUserInfo } from '@youversion/platform-core'; +import { YouVersionAuthContext, YouVersionContext } from '@youversion/platform-react-hooks'; +import type { ReactNode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; + +const mockUserInfo = { id: 'user-1', name: 'Test User' } as unknown as YouVersionUserInfo; + +let signedIn = false; + +function Providers({ children }: { children: ReactNode }) { + return ( + + + {children} + + + ); +} + +const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; + +beforeEach(() => { + vi.restoreAllMocks(); + signedIn = false; + setHighlightsLive(true); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setHighlightsLive(HIGHLIGHTS_LIVE); +}); + +describe('useBibleReaderHighlights — real useHighlights/useApiData', () => { + it('fetches and renders highlights when auth resolves after mount (enabled false→true)', async () => { + const getHighlights = vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue({ + data: [ + { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.17', color: '5dff79' }, + ], + next_page_token: null, + }); + + // Mount signed out — mirrors YouVersionAuthProvider, which initializes + // userInfo to null and resolves the session asynchronously. + const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: Providers, + }); + + expect(getHighlights).not.toHaveBeenCalled(); + expect(result.current.highlightedVerses).toEqual({}); + + // The session resolves: only `enabled` flips — no other dep changes. + signedIn = true; + rerender(); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00', 17: '5dff79' }); + }); + expect(getHighlights).toHaveBeenCalledTimes(1); + expect(getHighlights).toHaveBeenCalledWith({ version_id: 111, passage_id: 'JHN.3' }); + }); + + it('un-renders highlights immediately on sign-out and does not refetch', async () => { + const getHighlights = vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue({ + data: [{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }], + next_page_token: null, + }); + + signedIn = true; + const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: Providers, + }); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); + + signedIn = false; + rerender(); + + expect(result.current.highlightedVerses).toEqual({}); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/ui/src/components/use-bible-reader-highlights.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx index 80185e6b..d6c165d1 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -134,7 +134,8 @@ describe('useBibleReaderHighlights — auth guarding', () => { }); expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); - // Sign out: useApiData keeps its stale data, so the hook itself must gate. + // Sign out: the mock still returns fetched data (like a not-yet-cleared + // cache would), so this pins the hook's own render gate. signedIn = false; rerender(); expect(result.current.highlightedVerses).toEqual({}); @@ -229,6 +230,42 @@ describe('useBibleReaderHighlights — apply', () => { }); }); +describe('useBibleReaderHighlights — overlay confirmation', () => { + it('drops the optimistic entry once the post-write refetch lands, so server truth wins', async () => { + const mocked = mockUseHighlights(); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Let the write settle successfully. + await waitFor(() => { + expect(mocked.createHighlight).toHaveBeenCalledTimes(1); + }); + await act(async () => { + await Promise.resolve(); + }); + + // The post-write refetch lands with different server truth for that verse + // (e.g. another device re-colored it between our POST and the GET). + mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: '00d6ff' }]), + }); + rerender(); + + // Without confirmation-clearing, the stale overlay entry would keep + // rendering fffe00 until navigation. + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: '00d6ff' }); + }); + }); +}); + describe('useBibleReaderHighlights — remove', () => { it('removes optimistically and DELETEs only verses rendered in that color, as ranges', async () => { const mocked = mockUseHighlights({ diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 8ce9ebd9..c63de3a8 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -4,7 +4,7 @@ import { isHighlightsLive } from '@/lib/feature-flags'; import { buildPassageIds } from '@/lib/usfm-ranges'; import { useHighlights, YouVersionAuthContext } from '@youversion/platform-react-hooks'; import { Result } from 'better-result'; -import { useCallback, useContext, useMemo, useRef, useState } from 'react'; +import { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; export type UseBibleReaderHighlightsOptions = { versionId: number; @@ -99,8 +99,9 @@ export function useBibleReaderHighlights({ } const highlightedVerses = useMemo(() => { - // `useApiData` keeps its last data when disabled, so gate on `live` here - // too: sign-out must un-render highlights immediately, not eventually. + // Gate on `live` here too (useApiData also clears data when disabled): + // sign-out or flag-off must render nothing this very render, including + // any optimistic overlay entries still in state. if (!live) return {}; const map: Record = {}; @@ -127,6 +128,30 @@ export function useBibleReaderHighlights({ const highlightedVersesRef = useRef(highlightedVerses); highlightedVersesRef.current = highlightedVerses; + // Verses whose write settled successfully and are awaiting the post-write + // refetch. Once fresh data lands, their overlay entries are dropped so the + // server's truth wins again — otherwise a successful write's overlay entry + // would mask every later server-side change to that verse (another device, + // another tab) until navigation. + const confirmedVersesRef = useRef>(new Set()); + + useEffect(() => { + if (confirmedVersesRef.current.size === 0) return; + const confirmed = confirmedVersesRef.current; + confirmedVersesRef.current = new Set(); + setOverlay((current) => { + let changed = false; + const next = { ...current }; + for (const verse of confirmed) { + if (verse in next) { + delete next[verse]; + changed = true; + } + } + return changed ? next : current; + }); + }, [highlights]); + const patchOverlay = useCallback((verses: number[], value: string | null) => { setOverlay((current) => { const next = { ...current }; @@ -146,6 +171,21 @@ export function useBibleReaderHighlights({ }); }, []); + // Known concurrency windows at this apply/remove boundary. Deliberately not + // closed in PR 1 — a real per-verse operation queue is PR 2 territory: + // + // - Apply then remove the same verse while the POST is still in flight: the + // DELETE can reach the server before the create commits, leaving a + // server-side highlight that renders as removed until the next refetch or + // navigation repaints it. + // - Two overlapping writes touching the same verses: the loser's + // snapshot-based failure revert can transiently clobber the winner's + // optimistic entry (rendering converges once the post-write refetch + // lands, since useApiData is latest-wins). + // + // The confirmed-verse clearing above narrows both windows — a settled + // write's overlay entry stops shadowing server truth as soon as fresh data + // arrives — but does not close them. const apply = useCallback( (color: string, verses: number[]) => { if (!live || verses.length === 0) return; @@ -170,7 +210,12 @@ export function useBibleReaderHighlights({ ); const failures = results.filter(Result.isError); - if (failures.length === 0) return; + if (failures.length === 0) { + // Hand the verses to the confirmed set: the refetch createHighlight + // triggered will clear their overlay entries when its data lands. + for (const verse of verses) confirmedVersesRef.current.add(verse); + return; + } for (const failure of failures) { console.error( `[YouVersion SDK] Failed to apply highlight (version ${versionId}, ` + @@ -212,7 +257,10 @@ export function useBibleReaderHighlights({ ); const failures = results.filter(Result.isError); - if (failures.length === 0) return; + if (failures.length === 0) { + for (const verse of targetVerses) confirmedVersesRef.current.add(verse); + return; + } for (const failure of failures) { console.error( `[YouVersion SDK] Failed to remove highlight (version ${versionId}, ` + From 7f66a99a787197f1093e696fb82abc7a5199df8f Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 13:49:17 -0500 Subject: [PATCH 4/7] chore(hooks): drop inert lint-disable comment Co-Authored-By: Claude Fable 5 --- packages/hooks/src/useApiData.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/hooks/src/useApiData.ts b/packages/hooks/src/useApiData.ts index 2cdf1a96..63624e70 100644 --- a/packages/hooks/src/useApiData.ts +++ b/packages/hooks/src/useApiData.ts @@ -79,7 +79,6 @@ export function useApiData( // the deps change or the component unmounts. requestSeqRef.current++; }; - // @eslint-disable-next-line react-hooks/exhaustive-deps }, [...deps, enabled]); return { data, loading, error, refetch }; From 65f61b26fdfe1e9e08eb61e23f9efc9f48a63aad Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Mon, 20 Jul 2026 15:57:42 -0500 Subject: [PATCH 5/7] fix(ui): scope-gate confirmed highlight verses and drop non-null overlay assertions Cross-scope contamination: confirmedVersesRef enrolled a settled write's verses regardless of whether the user had since navigated to another chapter/version, letting the drain effect erase the new scope's optimistic overlay entry (verse numbers collide across scopes). Capture the overlay scope at write start and only enroll on settle if it still matches the current scope ref; also clear the confirmed set on scope change alongside the existing render-time overlay reset. Also replace two non-null assertions (`overlay[verse]!`, `snapshot[verse]!`) with `as string | null` casts: null means "optimistically removed" in HighlightOverlay, so `!` stripped a legitimate value. Co-Authored-By: Claude Fable 5 --- .../use-bible-reader-highlights.test.tsx | 56 +++++++++++++++++++ .../components/use-bible-reader-highlights.ts | 56 ++++++++++++++----- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/packages/ui/src/components/use-bible-reader-highlights.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx index d6c165d1..88ff02bb 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -357,4 +357,60 @@ describe('useBibleReaderHighlights — scope changes', () => { rerender({ versionId: 111, book: 'JHN', chapter: '4' }); expect(result.current.highlightedVerses).toEqual({}); }); + + it("does not drain the new scope's optimistic overlay when a previous scope's write settles", async () => { + // Chapter 3's create is deferred so it can settle AFTER we navigate away. + let resolvePrevWrite: (value: unknown) => void = vi.fn(); + const prevWrite = new Promise((resolve) => { + resolvePrevWrite = resolve; + }); + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(prevWrite), + }); + + const { result, rerender } = renderHook( + (props: { versionId: number; book: string; chapter: string }) => + useBibleReaderHighlights(props), + { wrapper: AuthWrapper, initialProps: defaultOptions }, + ); + + // Highlight JHN.3.16 — write is in flight (deferred, has not settled). + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Navigate to JHN.4: the overlay resets. Give this chapter its own + // never-settling write so its optimistic entry survives on its own terms. + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(new Promise(vi.fn())), + }); + rerender({ versionId: 111, book: 'JHN', chapter: '4' }); + expect(result.current.highlightedVerses).toEqual({}); + + // Highlight JHN.4.16 — same verse number, different chapter. + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Chapter 3's stale write finally settles. It must NOT enroll verse 16 in + // the confirmed set, because we have since left its scope. + await act(async () => { + resolvePrevWrite({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); + await Promise.resolve(); + await Promise.resolve(); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // A refetch lands for JHN.4 (new `highlights` identity fires the drain + // effect). A leaked cross-scope confirmation would erase JHN.4.16 here. + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(new Promise(vi.fn())), + highlights: makeCollection([]), + }); + rerender({ versionId: 111, book: 'JHN', chapter: '4' }); + + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); }); diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index c63de3a8..adca292e 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -49,7 +49,7 @@ class BibleReaderHighlightError extends Error { function snapshotOverlay(overlay: HighlightOverlay, verses: number[]): HighlightOverlay { const snapshot: HighlightOverlay = {}; for (const verse of verses) { - if (verse in overlay) snapshot[verse] = overlay[verse]!; + if (verse in overlay) snapshot[verse] = overlay[verse] as string | null; } return snapshot; } @@ -88,14 +88,36 @@ export function useBibleReaderHighlights({ const [overlay, setOverlay] = useState({}); - // Drop the optimistic overlay synchronously (during render) the moment the - // scope changes, so an in-flight overlay never paints over another - // chapter's or version's verses — their verse numbers collide. + // Verses whose write settled successfully and are awaiting the post-write + // refetch. Once fresh data lands, the drain effect below drops their overlay + // entries so the server's truth wins again — otherwise a successful write's + // overlay entry would mask every later server-side change to that verse + // (another device, another tab) until navigation. + // + // The set is scoped to the current version+chapter: verse numbers only mean + // something within one scope. Two things keep it honest: (1) a write enrolls + // its verses here only if the scope captured at write start still matches + // when the write settles (see `apply` / `remove`), and (2) the set is cleared + // on scope change alongside the overlay reset below. Without that gate a slow + // write from the previous chapter could enroll a verse number that now + // belongs to the new chapter's optimistic entry, and the drain effect would + // erase it. Same-scope write races are documented at the apply/remove + // boundary and deferred to PR 2. + const confirmedVersesRef = useRef>(new Set()); + + // Drop the optimistic overlay (and the now-meaningless confirmed set) + // synchronously during render the moment the scope changes, so an in-flight + // overlay never paints over another chapter's or version's verses — their + // verse numbers collide. `overlayScopeRef` mirrors the current scope so the + // async write callbacks can compare it at settle time. const overlayScope = `${versionId}:${chapterUsfm}`; + const overlayScopeRef = useRef(overlayScope); + overlayScopeRef.current = overlayScope; const [loadedOverlayScope, setLoadedOverlayScope] = useState(overlayScope); if (loadedOverlayScope !== overlayScope) { setLoadedOverlayScope(overlayScope); setOverlay({}); + confirmedVersesRef.current = new Set(); } const highlightedVerses = useMemo(() => { @@ -128,13 +150,8 @@ export function useBibleReaderHighlights({ const highlightedVersesRef = useRef(highlightedVerses); highlightedVersesRef.current = highlightedVerses; - // Verses whose write settled successfully and are awaiting the post-write - // refetch. Once fresh data lands, their overlay entries are dropped so the - // server's truth wins again — otherwise a successful write's overlay entry - // would mask every later server-side change to that verse (another device, - // another tab) until navigation. - const confirmedVersesRef = useRef>(new Set()); - + // When the post-write refetch lands, drain the confirmed verses' overlay + // entries so the server's truth wins again (see `confirmedVersesRef` above). useEffect(() => { if (confirmedVersesRef.current.size === 0) return; const confirmed = confirmedVersesRef.current; @@ -164,7 +181,7 @@ export function useBibleReaderHighlights({ setOverlay((current) => { const next = { ...current }; for (const verse of verses) { - if (verse in snapshot) next[verse] = snapshot[verse]!; + if (verse in snapshot) next[verse] = snapshot[verse] as string | null; else delete next[verse]; } return next; @@ -191,6 +208,7 @@ export function useBibleReaderHighlights({ if (!live || verses.length === 0) return; const normalizedColor = color.toLowerCase(); + const scopeAtWrite = overlayScopeRef.current; const snapshot = snapshotOverlay(overlayRef.current, verses); patchOverlay(verses, normalizedColor); @@ -212,8 +230,12 @@ export function useBibleReaderHighlights({ const failures = results.filter(Result.isError); if (failures.length === 0) { // Hand the verses to the confirmed set: the refetch createHighlight - // triggered will clear their overlay entries when its data lands. - for (const verse of verses) confirmedVersesRef.current.add(verse); + // triggered will clear their overlay entries when its data lands. Only + // enroll if we're still in the scope that issued the write — otherwise + // these verse numbers now belong to a different chapter's overlay. + if (overlayScopeRef.current === scopeAtWrite) { + for (const verse of verses) confirmedVersesRef.current.add(verse); + } return; } for (const failure of failures) { @@ -243,6 +265,7 @@ export function useBibleReaderHighlights({ const targetVerses = verses.filter((verse) => rendered[verse] === normalizedColor); if (targetVerses.length === 0) return; + const scopeAtWrite = overlayScopeRef.current; const snapshot = snapshotOverlay(overlayRef.current, targetVerses); patchOverlay(targetVerses, null); @@ -258,7 +281,10 @@ export function useBibleReaderHighlights({ const failures = results.filter(Result.isError); if (failures.length === 0) { - for (const verse of targetVerses) confirmedVersesRef.current.add(verse); + // Only enroll if still in the scope that issued the write (see `apply`). + if (overlayScopeRef.current === scopeAtWrite) { + for (const verse of targetVerses) confirmedVersesRef.current.add(verse); + } return; } for (const failure of failures) { From c636c5056de77e3dd2ee1f79a295bbe640ca408e Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Mon, 20 Jul 2026 16:07:20 -0500 Subject: [PATCH 6/7] fix(ui): scope-gate failure-path overlay reverts The success path already skipped enrolling verses in confirmedVersesRef when the write's captured scope no longer matched. The failure path had the same gap: after navigating chapters, a stale write's revertOverlay ran against a snapshot that predates the reset, so its absent entries would `delete` the new chapter's optimistic verses (verse numbers collide across scopes). Gate both revertOverlay calls on `overlayScopeRef.current === scopeAtWrite`, matching the success path. console.error logging stays unconditional. Update the invariant comment to cover both settle paths. Co-Authored-By: Claude Fable 5 --- .../use-bible-reader-highlights.test.tsx | 56 +++++++++++++++++++ .../components/use-bible-reader-highlights.ts | 25 +++++---- 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/packages/ui/src/components/use-bible-reader-highlights.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx index 88ff02bb..a2e914f1 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -413,4 +413,60 @@ describe('useBibleReaderHighlights — scope changes', () => { expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); }); + + it("does not revert the new scope's optimistic overlay when a previous scope's write fails", async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(vi.fn()); + + // Chapter 3's create is deferred so it can fail AFTER we navigate away. + let rejectPrevWrite: (reason?: unknown) => void = vi.fn(); + const prevWrite = new Promise((_resolve, reject) => { + rejectPrevWrite = reject; + }); + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(prevWrite), + }); + + const { result, rerender } = renderHook( + (props: { versionId: number; book: string; chapter: string }) => + useBibleReaderHighlights(props), + { wrapper: AuthWrapper, initialProps: defaultOptions }, + ); + + // Highlight JHN.3.16 — write is in flight (deferred, will fail later). + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Navigate to JHN.4: the overlay resets. This chapter gets its own + // never-settling write so its optimistic entry survives on its own terms. + mockUseHighlights({ + createHighlight: vi.fn().mockReturnValue(new Promise(vi.fn())), + }); + rerender({ versionId: 111, book: 'JHN', chapter: '4' }); + expect(result.current.highlightedVerses).toEqual({}); + + // Highlight JHN.4.16 — same verse number, different chapter. + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // Chapter 3's stale write finally fails. Its snapshot ({}) lacks verse 16, + // so an ungated revert would `delete` JHN.4.16's optimistic entry. The + // scope gate skips the revert since we've left that scope. + await act(async () => { + rejectPrevWrite(new Error('network down')); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + // The failure is still logged unconditionally. + expect(consoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to apply highlight'), + expect.objectContaining({ passageId: 'JHN.3.16' }), + ); + consoleError.mockRestore(); + }); }); diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index adca292e..76617795 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -95,13 +95,14 @@ export function useBibleReaderHighlights({ // (another device, another tab) until navigation. // // The set is scoped to the current version+chapter: verse numbers only mean - // something within one scope. Two things keep it honest: (1) a write enrolls - // its verses here only if the scope captured at write start still matches - // when the write settles (see `apply` / `remove`), and (2) the set is cleared - // on scope change alongside the overlay reset below. Without that gate a slow - // write from the previous chapter could enroll a verse number that now - // belongs to the new chapter's optimistic entry, and the drain effect would - // erase it. Same-scope write races are documented at the apply/remove + // something within one scope. Both settle paths capture the scope at write + // start (`scopeAtWrite`) and act only if it still matches `overlayScopeRef` + // when they run — a success enrolls its verses here, a failure reverts the + // optimistic overlay — and the set is also cleared on scope change alongside + // the overlay reset below. Without those guards a slow write from the + // previous chapter could touch a verse number that now belongs to the new + // chapter's optimistic entry (enroll-then-drain, or a snapshot-absent revert + // that deletes it). Same-scope write races are documented at the apply/remove // boundary and deferred to PR 2. const confirmedVersesRef = useRef>(new Set()); @@ -246,8 +247,11 @@ export function useBibleReaderHighlights({ ); } // Partial failures revert the whole optimistic batch; the refetch that - // any successful create triggered repaints the server's truth. - revertOverlay(verses, snapshot); + // any successful create triggered repaints the server's truth. Gate on + // scope like the success path: if we've since navigated away, the reset + // already wiped this overlay and the snapshot's absent entries would + // otherwise `delete` the new scope's optimistic verses. + if (overlayScopeRef.current === scopeAtWrite) revertOverlay(verses, snapshot); })(); }, [live, book, chapter, versionId, createHighlight, patchOverlay, revertOverlay], @@ -294,7 +298,8 @@ export function useBibleReaderHighlights({ failure.error, ); } - revertOverlay(targetVerses, snapshot); + // Gate on scope like the success path (see `apply`). + if (overlayScopeRef.current === scopeAtWrite) revertOverlay(targetVerses, snapshot); })(); }, [live, book, chapter, versionId, deleteHighlight, patchOverlay, revertOverlay], From a36263a6ed49d5e0cf48af98834172cbbda6b75a Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Tue, 21 Jul 2026 09:31:08 -0500 Subject: [PATCH 7/7] refactor: apply non-blocking review suggestions from PR 283 - use Promise.withResolvers in useApiData tests - collapse feature-flags.ts to minimal shape - trim CONTEXT.md glossary to implemented terms Co-Authored-By: Claude Fable 5 --- CONTEXT.md | 44 +++----------------------- packages/hooks/src/useApiData.test.tsx | 24 +++----------- packages/hooks/tsconfig.json | 1 + packages/ui/src/lib/feature-flags.ts | 28 +++------------- 4 files changed, 15 insertions(+), 82 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index f6702d7b..8ea84bc0 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -39,45 +39,11 @@ the verse action popover. Never persisted; cleared on navigation. ## Self-contained mode -The default `BibleReader` posture (YPE-1034): the reader fetches and writes -highlights itself through the SDK's own auth session. The color row is -always offered; tapping a color when the user lacks a session or the -highlights permission enters the highlight auth flow (July 9 2026 sync, -superseding the earlier hide-when-signed-out idea). - -## Highlights permission - -The per-app grant that authorizes highlight reads/writes for a user. It is -**not an OIDC scope**: it travels as `requested_permissions[]=highlights` -alongside `scope` at authorize time (PR #280) and is granted via a data -exchange consent. Permissions are open-ended strings, never enums (more -arrive later, e.g. verse notes). The **server is the source of truth** for -whether it is granted; any client-side permission cache is optimistic only. - -## Pending highlight - -The user's stashed highlight intent (verses + color + timestamp) while the -highlight auth flow is in flight. Survives a redirect round-trip via -sessionStorage; expires stale (~10 min) so an abandoned round-trip can -never apply a highlight during a later sign-in. Discarded on decline, -cancel, or failure. - -## Highlight auth flow - -The state machine (see the React Web auth state machine doc) that turns a -color tap into an applied highlight across two user paths: one-fell-swoop -(sign-in requesting the highlights permission together) and just-in-time -(already signed in, permission confirm dialog → data exchange grant). -Cancellation at any point keeps the verse selection intact and discards -only the pending highlight. - -## Controlled mode - -A planned `BibleReader` posture (YPE-3705): the host application supplies -rendered highlights as data and receives highlight-intent events, and the -reader makes no highlight network calls. The host owns auth, persistence, -and conflict rules. Used by native hosts (RN Expo SDK) embedding the web -reader. +The `BibleReader` posture (YPE-1034): the reader fetches and writes +highlights itself through the SDK's own auth session. Highlight behavior is +gated on the internal `HIGHLIGHTS_LIVE` dark-launch flag and an +authenticated user — while the flag is off or the user has no session, the +reader is inert: no fetches, no writes, nothing rendered from the API. ## Verse action popover diff --git a/packages/hooks/src/useApiData.test.tsx b/packages/hooks/src/useApiData.test.tsx index 62570284..cb21a189 100644 --- a/packages/hooks/src/useApiData.test.tsx +++ b/packages/hooks/src/useApiData.test.tsx @@ -5,22 +5,6 @@ import { act, renderHook, waitFor } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; import { useApiData, type UseApiDataOptions } from './useApiData'; -type Deferred = { - promise: Promise; - resolve: (value: T) => void; - reject: (reason: unknown) => void; -}; - -function createDeferred(): Deferred { - let resolve!: (value: T) => void; - let reject!: (reason: unknown) => void; - const promise = new Promise((res, rej) => { - resolve = res; - reject = rej; - }); - return { promise, resolve, reject }; -} - describe('useApiData — enabled transitions', () => { it('fetches when enabled flips from false to true with unchanged deps', async () => { const fetchFn = vi.fn().mockResolvedValue('payload'); @@ -68,9 +52,9 @@ describe('useApiData — enabled transitions', () => { describe('useApiData — stale responses (latest wins)', () => { it('ignores a stale refetch response that resolves after a newer fetch', async () => { - const deferreds: Deferred[] = []; + const deferreds: PromiseWithResolvers[] = []; const fetchFn = vi.fn(() => { - const deferred = createDeferred(); + const deferred = Promise.withResolvers(); deferreds.push(deferred); return deferred.promise; }); @@ -112,9 +96,9 @@ describe('useApiData — stale responses (latest wins)', () => { }); it('ignores a stale error from an invalidated request', async () => { - const deferreds: Deferred[] = []; + const deferreds: PromiseWithResolvers[] = []; const fetchFn = vi.fn(() => { - const deferred = createDeferred(); + const deferred = Promise.withResolvers(); deferreds.push(deferred); return deferred.promise; }); diff --git a/packages/hooks/tsconfig.json b/packages/hooks/tsconfig.json index 815796ab..c2233e27 100644 --- a/packages/hooks/tsconfig.json +++ b/packages/hooks/tsconfig.json @@ -1,6 +1,7 @@ { "extends": "@internal/tsconfig/react.json", "compilerOptions": { + "lib": ["ES2024", "DOM", "DOM.Iterable"], "outDir": "dist", "rootDir": ".", "baseUrl": ".", diff --git a/packages/ui/src/lib/feature-flags.ts b/packages/ui/src/lib/feature-flags.ts index 7c76de52..beb74389 100644 --- a/packages/ui/src/lib/feature-flags.ts +++ b/packages/ui/src/lib/feature-flags.ts @@ -1,31 +1,13 @@ -/** - * Internal dark-launch flags for the UI package. - * - * NOT exported from the package entry point (`src/index.ts`) — these are - * SDK-internal switches, not consumer API. Flip the default here when a - * feature is ready to launch. - */ - /** * Dark-launch default for server-backed highlights in BibleReader (YPE-1034). - * While `false`, the reader's highlight color row is inert: no highlight - * fetches, no writes, nothing rendered from the API. + * While `false`, the reader's highlight color row is inert: no fetches, no + * writes, nothing rendered from the API. Flip here to launch. */ export const HIGHLIGHTS_LIVE = false; let highlightsLive = HIGHLIGHTS_LIVE; -/** Current state of the highlights dark-launch flag. */ -export function isHighlightsLive(): boolean { - return highlightsLive; -} +export const isHighlightsLive = (): boolean => highlightsLive; -/** - * Force the highlights flag on/off. - * - * @internal Test/Storybook-only. Never call from product code — launch by - * changing {@link HIGHLIGHTS_LIVE} instead. - */ -export function setHighlightsLive(value: boolean): void { - highlightsLive = value; -} +/** @internal Test/Storybook only — launch by changing HIGHLIGHTS_LIVE. */ +export const setHighlightsLive = (value: boolean): void => void (highlightsLive = value);