diff --git a/.changeset/bible-reader-highlights-api.md b/.changeset/bible-reader-highlights-api.md new file mode 100644 index 00000000..2fbeb750 --- /dev/null +++ b/.changeset/bible-reader-highlights-api.md @@ -0,0 +1,15 @@ +--- +'@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). +- `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/CONTEXT.md b/CONTEXT.md new file mode 100644 index 00000000..8ea84bc0 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,51 @@ +# 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 `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 + +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. 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/hooks/src/useApiData.test.tsx b/packages/hooks/src/useApiData.test.tsx new file mode 100644 index 00000000..cb21a189 --- /dev/null +++ b/packages/hooks/src/useApiData.test.tsx @@ -0,0 +1,159 @@ +/** + * @vitest-environment jsdom + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; +import { useApiData, type UseApiDataOptions } from './useApiData'; + +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: PromiseWithResolvers[] = []; + const fetchFn = vi.fn(() => { + const deferred = Promise.withResolvers(); + 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: PromiseWithResolvers[] = []; + const fetchFn = vi.fn(() => { + const deferred = Promise.withResolvers(); + 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..63624e70 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,62 @@ 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; - // @eslint-disable-next-line react-hooks/exhaustive-deps - }, deps); + fetchData(); + return () => { + // Invalidate any in-flight request (effect- or refetch-initiated) when + // the deps change or the component unmounts. + requestSeqRef.current++; + }; + }, [...deps, enabled]); return { data, loading, error, refetch }; } 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/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.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 new file mode 100644 index 00000000..a2e914f1 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -0,0 +1,472 @@ +/** + * @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: 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({}); + }); +}); + +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 — 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({ + 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({}); + }); + + 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' }); + }); + + 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 new file mode 100644 index 00000000..76617795 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -0,0 +1,309 @@ +'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, useEffect, 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] as string | null; + } + 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({}); + + // 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. 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()); + + // 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(() => { + // 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 = {}; + 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; + + // 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; + 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 }; + 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] as string | null; + else delete next[verse]; + } + return next; + }); + }, []); + + // 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; + + const normalizedColor = color.toLowerCase(); + const scopeAtWrite = overlayScopeRef.current; + 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) { + // Hand the verses to the confirmed set: the refetch createHighlight + // 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) { + 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. 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], + ); + + 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 scopeAtWrite = overlayScopeRef.current; + 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) { + // 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) { + console.error( + `[YouVersion SDK] Failed to remove highlight (version ${versionId}, ` + + `passage ${failure.error.passageId}, color ${normalizedColor})`, + failure.error, + ); + } + // Gate on scope like the success path (see `apply`). + if (overlayScopeRef.current === scopeAtWrite) 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..beb74389 --- /dev/null +++ b/packages/ui/src/lib/feature-flags.ts @@ -0,0 +1,13 @@ +/** + * Dark-launch default for server-backed highlights in BibleReader (YPE-1034). + * 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; + +export const isHighlightsLive = (): boolean => highlightsLive; + +/** @internal Test/Storybook only — launch by changing HIGHLIGHTS_LIVE. */ +export const setHighlightsLive = (value: boolean): void => 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