diff --git a/.changeset/fix-highlights-jam-issues.md b/.changeset/fix-highlights-jam-issues.md new file mode 100644 index 00000000..53c12932 --- /dev/null +++ b/.changeset/fix-highlights-jam-issues.md @@ -0,0 +1,15 @@ +--- +'@youversion/platform-core': patch +'@youversion/platform-react-hooks': minor +'@youversion/platform-react-ui': patch +--- + +Fix four highlights-stack bugs surfaced by a staging session, plus a fill fade-in. + +- **Invisible primary button (ui):** the `default` button variant paired `bg-background` with `text-primary-foreground`, resolving to white-on-white in the light theme (the highlight permission dialog's Continue button was invisible). It now uses the standard `bg-primary` / `text-primary-foreground` pairing. `YouVersionAuthButton` pins `bg-background` explicitly so its neutral brand surface is unchanged. +- **Optimistic overlay dropped before the write is visible (ui):** after a successful highlight write the seam hook dropped the optimistic overlay as soon as ANY refetch landed, trusting it as server truth. Under read-after-write lag (staging slowness, prod read replicas) that GET often did not yet reflect the write, so a highlight flickered out and back on apply, or a removed highlight reappeared. The overlay is now retired only once a fetch actually reflects the write (apply → the verse shows the written color; remove → the verse no longer shows the removed color); until then the overlay wins. Reset paths (chapter/version change, sign-out) release any write the server never converges on. +- **Refetch coalescing (hooks behavior change):** `useHighlights.createHighlight` / `deleteHighlight` no longer refetch on each call. Direct consumers of `useHighlights` must now call `refetch()` themselves after their mutations settle. The sole in-repo consumer (`useBibleReaderHighlights`) now issues a single refetch after each batch settles — success or failure — so highlighting verses `[2,3,5]` fires two POSTs but only one GET (previously two). Batches with partial failures settle per sub-write: succeeded writes reconcile to server truth, only the failed writes' verses revert. +- **DELETE by range is unsupported (core + ui):** range passage-ids (e.g. `JHN.1.2-3`) returned a non-2xx from staging and threw. The remove path now sends one DELETE per verse (`JHN.1.2`, `JHN.1.3`); POST/apply still collapses to ranges, which works. The `HighlightsClient.deleteHighlight` docstring no longer claims range delete is supported (marked unverified pending API-team confirmation). Large removals issue more DELETEs but still coalesce to a single refetch. +- **Highlight fade-in (core styles):** verse highlight fills now transition their background color (~250ms, disabled under `prefers-reduced-motion`) instead of popping in, matching the Bible app. The selection underline and layout are untouched. + +Known/accepted trade-off (pending product sign-off): the optimistic overlay holds the locally-written value until a fetch reflects that write. A concurrent change from another device to the same verse therefore renders stale until navigation or the next write on this client. This is the deliberate cost of eliminating the flicker/ghost bugs above; documented in code comments in `useBibleReaderHighlights`. diff --git a/.changeset/highlight-auth-flow.md b/.changeset/highlight-auth-flow.md new file mode 100644 index 00000000..da007500 --- /dev/null +++ b/.changeset/highlight-auth-flow.md @@ -0,0 +1,11 @@ +--- +'@youversion/platform-core': patch +'@youversion/platform-react-hooks': patch +'@youversion/platform-react-ui': patch +--- + +Add the highlight auth flow: a color tap in BibleReader without a session or the `highlights` permission now stashes the intent and runs the two-path grant flow (YPE-1034, still behind the internal `HIGHLIGHTS_LIVE` flag). + +- **Core**: new `DataExchangeClient.updateToken` (`POST /data-exchange/token`, 201 → `{ token }`, Zod-validated) plus `buildDataExchangeUrl` / `parseDataExchangeCallback` / `handleDataExchangeCallback` for the hosted just-in-time grant. Sign-in and data-exchange callbacks now parse `granted_permissions` and seed an optimistic permission cache on `YouVersionPlatformConfiguration` (`grantedPermissions`, `hasPermission`, `saveGrantedPermissions`, `removeGrantedPermission`); the cache is cleared on sign-out and a 401/403 invalidates it (server truth wins). +- **Hooks**: new `useHighlightAuthActions` exposing the one-fell-swoop sign-in (requesting `highlights`), the just-in-time data-exchange redirect, the permission-cache reads/invalidation, and the data-exchange return handler. +- **UI**: `useBibleReaderHighlights` now runs the state machine — pending highlights persist to `sessionStorage` (~10-minute expiry) to survive the redirect round-trip and apply automatically on a granted return; a just-in-time permission confirm dialog (`HighlightPermissionDialog`, copy matched to the native SDK) gates the data-exchange grant. Write failures route by status: 401/403 invalidates the cache, keeps the pending highlight, and re-prompts; 5xx/network reverts the optimistic overlay and discards. Apply/remove writes are serialized through a FIFO queue with per-verse ownership so overlapping operations settle to the last-issued state. Copy/share-only behavior when no auth provider is configured is unchanged. diff --git a/.changeset/xstate-highlights-flow.md b/.changeset/xstate-highlights-flow.md new file mode 100644 index 00000000..68d41edb --- /dev/null +++ b/.changeset/xstate-highlights-flow.md @@ -0,0 +1,12 @@ +--- +'@youversion/platform-react-ui': patch +--- + +Rewrite the BibleReader highlights flow as an explicit xstate v5 statechart (YPE-1034 / PR-288, still behind the internal `HIGHLIGHTS_LIVE` flag). `useBibleReaderHighlights` is now a thin adapter over `bibleReaderHighlightsMachine`; the machine (authored with `setup()` and named guards/actions/actors, so it is Stately-visualizable) owns the whole flow — optimistic overlay, serialized writes with per-verse ownership, reconcile, the auth flow, and the resume-on-return path. A mermaid statechart lives in `docs/highlight-flow-statechart.md`. All previously-shipped invariants are preserved. + +Behavior changes: + +- **Sign-in dialog first (signed out):** a signed-out color tap now opens a `SignInDialog` (introducing the app, with an optional integrator prompt) instead of redirecting to OAuth immediately. Confirm launches the sign-in redirect requesting `highlights`; decline/dismiss discards the pending highlight and keeps the verse selection. The dialog's `appName` comes from `YouVersionPlatformConfiguration.appName` (falling back to "This app") and its prompt from `signInPromptMessage`. The hook return gains `signInDialogOpen`, `confirmSignInDialog`, and `cancelSignInDialog`. +- **Flag-off hides the highlights UI:** when `HIGHLIGHTS_LIVE` is off, `VerseActionPopover` hides the color row and the remove (checkmark) circles entirely via a new `highlightsEnabled` prop — only Copy / Share remain. Previously the row still rendered while taps were inert. +- **"Vapor" fix:** a deleted highlight no longer briefly reappears when a stale read-replica fetch lands after the delete settled. The reconcile step no longer retires remove-overlay entries; a removed verse is held until a reset path (scope change, sign-out, or a newer write). Apply-side convergence is unchanged. This matches the already-accepted trade-off (a concurrent same-verse edit from another device renders stale until navigation or the next write). +- **Remove-failure no longer re-prompts:** a 401/403 on a remove invalidates the permission cache but no longer opens the permission dialog (there is no pending highlight to resume), resolving a documented deferred wart. diff --git a/CONTEXT.md b/CONTEXT.md index 8ea84bc0..1c3b95f1 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -49,3 +49,28 @@ reader is inert: no fetches, no writes, nothing rendered from the API. The floating action bar (YPE-642) that appears over a verse selection, offering highlight colors, copy, and share. + +## Highlights permission + +The data-exchange permission (`highlights`) an app must hold before it can +read or write a user's highlights. Permissions are open-ended strings, not +enums — more arrive later — and the server reports the ones a user granted +back to the app via `granted_permissions` on the sign-in and data-exchange +callbacks (YPE-1034). + +## Highlight auth flow + +The flow (YPE-1034) that turns a color tap into an applied highlight when the +reader is not yet authorized. A tap forks: authorized writes optimistically; +signed out opens the sign-in dialog; signed in without the **highlights +permission** opens the just-in-time permission dialog. Consent routes through +a full-page data-exchange redirect and resumes on return. + +## Pending highlight + +A user's stashed highlight intent (verses, color, scope) held in +`sessionStorage` while the **highlight auth flow** is in flight (YPE-1034). It +survives the full-page redirect round-trip and expires (~10 min) so an +abandoned round-trip can never silently apply a highlight during a much later +sign-in. It is intent, not highlight data (highlights stay server-only, +ADR-001); discarded on decline, cancel, failure, or successful apply. diff --git a/docs/highlight-flow-statechart.md b/docs/highlight-flow-statechart.md new file mode 100644 index 00000000..77264641 --- /dev/null +++ b/docs/highlight-flow-statechart.md @@ -0,0 +1,101 @@ +# BibleReader highlights flow — statechart (PR-288) + +The BibleReader highlights flow is an [xstate v5](https://stately.ai/docs) state +machine: [`packages/ui/src/components/bible-reader-highlights-machine.ts`](../packages/ui/src/components/bible-reader-highlights-machine.ts). +`useBibleReaderHighlights` is a thin adapter that feeds React-owned inputs (auth, +the `HIGHLIGHTS_LIVE` flag, the fetched highlights, the scope) into the machine +as events and reads back the dialog states + the rendered verse map. + +The machine is authored with `setup()` and named guards/actions/actors so it is +statically analyzable — paste the source into the [Stately visualizer](https://stately.ai/viz) +to explore it interactively. + +## States and events + +- **`booting`** → routes to `disabled` or `enabled` from the initial input. +- **`disabled`** — the flag is off **or** no auth provider is mounted. Fully + inert: no fetch, no writes, no dialogs. A color tap resolves to `noop`. +- **`enabled`** — a parallel state with two independent regions: + - **`flow`** — the auth / dialog flow. + - `resuming` consumes the data-exchange return exactly once, then routes on + the pending highlight + auth + permission. + - `awaitingAuth` waits for the authenticated flip after a redirect return. + - `idle` is interactive. + - `signInDialog` / `permissionDialog` are the two consent dialogs. + - **`writer`** — serialized optimistic writes. `idle → writing → checkQueue`, + processing one queued operation at a time so a DELETE can never overtake an + in-flight POST for the same verse. + +`TAP_COLOR` forks in `flow`: authorized (`applied`) → optimistic write; signed +out → `signInDialog`; signed in without the permission → `permissionDialog`. +Both dialog paths stash a pending highlight (10-min `sessionStorage` TTL) so the +intent survives the full-page redirect and resumes on a granted return. + +The stash is a **list**, not a single slot: a fresh tap replaces it, but a queued +optimistic write that loses permission (401/403) _appends_ its intent with +verse-level last-wins. Two writes queued in different colors can each 401, and +both intents must survive the re-grant — a single slot would let the second +overwrite the first. On resume, `applyPendingHighlight` re-applies every live +entry (each to its own scope, first-to-last); entries never overlap on verses. + +```mermaid +stateDiagram-v2 + [*] --> booting + booting --> disabled: flag off / no provider + booting --> enabled: flag on & provider + + disabled --> enabled: AUTH_CHANGED (enabled) + enabled --> disabled: AUTH_CHANGED (disabled) + + state disabled { + note right of disabled + TAP_COLOR → outcome "noop" + no fetch / writes / dialogs + end note + } + + state enabled { + -- + state flow { + [*] --> resuming + resuming --> idle: no pending + resuming --> awaitingAuth: pending & not authed + resuming --> idle: pending & authed & permission / applyPending + resuming --> permissionDialog: pending & authed & no permission + + awaitingAuth --> idle: authed & permission / applyPending + awaitingAuth --> permissionDialog: authed & no permission + + idle --> idle: TAP_COLOR authorized / optimistic write + idle --> signInDialog: TAP_COLOR signed out / stash pending + idle --> permissionDialog: TAP_COLOR no permission / stash pending + + signInDialog --> idle: CONFIRM_SIGN_IN / start sign-in redirect + signInDialog --> idle: DECLINE_SIGN_IN / clear pending + + permissionDialog --> idle: CONFIRM_PERMISSION / start data-exchange + permissionDialog --> idle: CANCEL_PERMISSION / clear pending + + idle --> permissionDialog: PERMISSION_LOST (401/403 on write) + } + -- + state writer { + [*] --> w_idle + w_idle --> writing: queue has work + writing --> checkQueue: processWrite done / settle + shift + checkQueue --> writing: queue has work + checkQueue --> w_idle: queue empty + } + } +``` + +_(`ENQUEUE`, `AUTH_CHANGED`, `HIGHLIGHTS_UPDATED`, and `SCOPE_CHANGED` are handled +without leaving the current state: they update context and let the `always` +guards re-route. `HIGHLIGHTS_UPDATED` also runs the overlay reconcile.)_ + +## The "vapor" fix + +See the "vapor" fix block comment at the top of +`packages/ui/src/components/bible-reader-highlights-machine.ts` for the root +cause, fix, and trade-off — that comment sits next to `reconcileOverlay` and is +the single source of truth for this rationale. diff --git a/packages/core/src/Users.ts b/packages/core/src/Users.ts index cf1678c3..98846ddb 100644 --- a/packages/core/src/Users.ts +++ b/packages/core/src/Users.ts @@ -3,6 +3,7 @@ import { YouVersionUserInfo } from './YouVersionUserInfo'; import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; import { SignInWithYouVersionPKCEAuthorizationRequestBuilder } from './SignInWithYouVersionPKCE'; import { SignInWithYouVersionResult } from './SignInWithYouVersionResult'; +import { parseGrantedPermissions } from './permissions'; export class YouVersionAPIUsers { /** @@ -121,6 +122,12 @@ export class YouVersionAPIUsers { token_type: string; }; + // Parse the data-exchange permissions the server granted. The server + // echoes them as `granted_permissions` on the callback URL (comma- or + // space-separated, param may repeat). Used below to seed the optimistic + // permission cache — the single source of truth for granted permissions. + const grantedPermissions = parseGrantedPermissions(urlParams); + // Extract user info from ID token const result = this.extractSignInResult(tokens); @@ -133,7 +140,8 @@ export class YouVersionAPIUsers { ); // Persist the decoded user profile so it survives reloads without - // retaining the ID token itself. + // retaining the ID token itself. This must happen before the permission + // cache is seeded below, since grants are scoped to the current user. YouVersionPlatformConfiguration.saveUserInfo({ id: result.yvpUserId, name: result.name, @@ -141,6 +149,13 @@ export class YouVersionAPIUsers { avatar_url: result.profilePicture, }); + // Persist the granted permissions into the optimistic permission cache so + // a one-fell-swoop sign-in that requested `highlights` can apply a pending + // highlight on return without a probe round-trip. + if (grantedPermissions.length > 0) { + YouVersionPlatformConfiguration.saveGrantedPermissions(grantedPermissions); + } + // Clean up localStorage localStorage.removeItem('youversion-auth-code-verifier'); localStorage.removeItem('youversion-auth-redirect-uri'); diff --git a/packages/core/src/YouVersionPlatformConfiguration.ts b/packages/core/src/YouVersionPlatformConfiguration.ts index 1e46f94f..2222dba1 100644 --- a/packages/core/src/YouVersionPlatformConfiguration.ts +++ b/packages/core/src/YouVersionPlatformConfiguration.ts @@ -15,6 +15,8 @@ export class YouVersionPlatformConfiguration { private static _apiHost: string = 'api.youversion.com'; private static _refreshTokenKey: string | null = null; private static _expiryDateKey: string | null = null; + private static _signInPromptMessage: string | undefined = undefined; + private static _appName: string | undefined = undefined; private static getOrSetInstallationId(): string { if (typeof window === 'undefined') { @@ -73,6 +75,97 @@ export class YouVersionPlatformConfiguration { public static clearAuthTokens(): void { this.saveAuthData(null, null, null); this.saveUserInfo(null); + this.clearGrantedPermissions(); + } + + /** + * Optimistic cache of the data-exchange permissions the server told us are + * granted (seeded from `granted_permissions` on the sign-in / data-exchange + * callbacks). It is optimistic only: the server is the source of truth, and a + * 401/403 on a permissioned request invalidates the relevant entry via + * {@link removeGrantedPermission}. + * + * The cache is scoped to the signed-in user: it is persisted as + * `{ userId, permissions }` and only read back when `userId` matches the + * current {@link storedUserInfo}. This prevents one user's grants from leaking + * to a later user who signs in without a {@link clearAuthTokens} in between. + */ + private static readonly grantedPermissionsKey = 'youversion-platform:granted-permissions'; + + /** The id of the user the cache is scoped to, or `null` when signed out. */ + private static get currentUserId(): string | null { + return this.storedUserInfo?.id ?? null; + } + + /** + * Reads the stored `{ userId, permissions }` entry, or `null` when absent, + * malformed, or in the legacy bare-array format (which is treated as absent). + */ + private static readStoredGrants(): { userId: string; permissions: string[] } | null { + if (typeof localStorage === 'undefined') return null; + const raw = localStorage.getItem(this.grantedPermissionsKey); + if (!raw) return null; + try { + const parsed: unknown = JSON.parse(raw); + if (typeof parsed !== 'object' || parsed === null) return null; + const record = parsed as Record; + // Reject the legacy bare-array format and any other malformed shape. + if (typeof record.userId !== 'string' || !Array.isArray(record.permissions)) { + return null; + } + return { + userId: record.userId, + permissions: record.permissions.filter( + (entry): entry is string => typeof entry === 'string', + ), + }; + } catch { + return null; + } + } + + private static writeStoredGrants(userId: string, permissions: string[]): void { + localStorage.setItem(this.grantedPermissionsKey, JSON.stringify({ userId, permissions })); + } + + public static get grantedPermissions(): string[] { + const userId = this.currentUserId; + if (!userId) return []; + const stored = this.readStoredGrants(); + if (stored?.userId !== userId) return []; + return stored.permissions; + } + + /** + * Merges `permissions` into the cache (union) when the cached entry belongs to + * the current user; a different (or absent) owner is replaced wholesale. + * No-ops when signed out, since grants must be scoped to a user. + */ + public static saveGrantedPermissions(permissions: string[]): void { + if (typeof localStorage === 'undefined') return; + const userId = this.currentUserId; + if (!userId) return; + const merged = new Set([...this.grantedPermissions, ...permissions]); + this.writeStoredGrants(userId, [...merged]); + } + + /** Drops a single permission from the cache — used to honor a server 401/403. */ + public static removeGrantedPermission(permission: string): void { + if (typeof localStorage === 'undefined') return; + const userId = this.currentUserId; + if (!userId) return; + const next = this.grantedPermissions.filter((entry) => entry !== permission); + this.writeStoredGrants(userId, next); + } + + public static clearGrantedPermissions(): void { + if (typeof localStorage === 'undefined') return; + localStorage.removeItem(this.grantedPermissionsKey); + } + + /** Optimistic check against the permission cache. Server 401/403 still wins. */ + public static hasPermission(permission: string): boolean { + return this.grantedPermissions.includes(permission); } public static get accessToken(): string | null { @@ -149,4 +242,29 @@ export class YouVersionPlatformConfiguration { static set expiryDateKey(value: string) { this._expiryDateKey = value; } + + /** + * The integrator's own pitch line shown in the sign-in dialog. Optional; not + * persisted (it is supplied by configuration on each app load). + */ + static get signInPromptMessage(): string | undefined { + return this._signInPromptMessage; + } + + static set signInPromptMessage(value: string | undefined) { + this._signInPromptMessage = value; + } + + /** + * The integrator's display name used in the sign-in dialog copy (e.g. + * "{appName} wants to connect to your YouVersion Bible App account"). Optional; + * not persisted (it is supplied by configuration on each app load). + */ + static get appName(): string | undefined { + return this._appName; + } + + static set appName(value: string | undefined) { + this._appName = value; + } } diff --git a/packages/core/src/__tests__/YouVersionPlatformConfiguration.test.ts b/packages/core/src/__tests__/YouVersionPlatformConfiguration.test.ts index 9a38cb0c..a248c7cc 100644 --- a/packages/core/src/__tests__/YouVersionPlatformConfiguration.test.ts +++ b/packages/core/src/__tests__/YouVersionPlatformConfiguration.test.ts @@ -344,4 +344,30 @@ describe('YouVersionPlatformConfiguration', () => { expect(firstId).toBe(mockUUID); }); }); + + describe('signInPromptMessage', () => { + it('should default to undefined and get/set the value', () => { + expect(YouVersionPlatformConfiguration.signInPromptMessage).toBeUndefined(); + + YouVersionPlatformConfiguration.signInPromptMessage = 'Save your highlights across devices.'; + expect(YouVersionPlatformConfiguration.signInPromptMessage).toBe( + 'Save your highlights across devices.', + ); + + YouVersionPlatformConfiguration.signInPromptMessage = undefined; + expect(YouVersionPlatformConfiguration.signInPromptMessage).toBeUndefined(); + }); + }); + + describe('appName', () => { + it('should default to undefined and get/set the value', () => { + expect(YouVersionPlatformConfiguration.appName).toBeUndefined(); + + YouVersionPlatformConfiguration.appName = 'Acme Bible'; + expect(YouVersionPlatformConfiguration.appName).toBe('Acme Bible'); + + YouVersionPlatformConfiguration.appName = undefined; + expect(YouVersionPlatformConfiguration.appName).toBeUndefined(); + }); + }); }); diff --git a/packages/core/src/__tests__/data-exchange.test.ts b/packages/core/src/__tests__/data-exchange.test.ts new file mode 100644 index 00000000..4ae0e357 --- /dev/null +++ b/packages/core/src/__tests__/data-exchange.test.ts @@ -0,0 +1,161 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { http, HttpResponse } from 'msw'; +import { ApiClient } from '../client'; +import { + DataExchangeClient, + buildDataExchangeUrl, + parseDataExchangeCallback, + handleDataExchangeCallback, +} from '../data-exchange'; +import { server } from './setup'; + +const apiHost = process.env.YVP_API_HOST; + +describe('DataExchangeClient.updateToken', () => { + let client: DataExchangeClient; + + beforeEach(() => { + client = new DataExchangeClient( + new ApiClient({ apiHost, appKey: 'test-app', installationId: 'test-installation' }), + ); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + }); + + it('POSTs requested_permissions with app-key query + Bearer auth and returns the token', async () => { + let seenAuth: string | null = null; + let seenBody: unknown = null; + let seenUrl = ''; + server.use( + http.post(`https://${apiHost}/data-exchange/token`, async ({ request }) => { + seenAuth = request.headers.get('Authorization'); + seenBody = await request.json(); + seenUrl = request.url; + return HttpResponse.json({ token: 'dx-token-123' }, { status: 201 }); + }), + ); + + const token = await client.updateToken(['highlights'], 'my-access-token'); + + expect(token).toBe('dx-token-123'); + expect(seenAuth).toBe('Bearer my-access-token'); + expect(seenBody).toEqual({ requested_permissions: ['highlights'] }); + expect(seenUrl).toContain('app-key=test-app'); + }); + + it('throws (401 = not permitted) when the server rejects', async () => { + server.use( + http.post( + `https://${apiHost}/data-exchange/token`, + () => new HttpResponse(null, { status: 401 }), + ), + ); + + await expect(client.updateToken(['highlights'], 'tok')).rejects.toThrow(); + }); + + it('throws when the response fails schema validation', async () => { + server.use( + http.post(`https://${apiHost}/data-exchange/token`, () => + HttpResponse.json({ not_a_token: true }, { status: 201 }), + ), + ); + + await expect(client.updateToken(['highlights'], 'tok')).rejects.toThrow( + /Unexpected data exchange token response/, + ); + }); +}); + +describe('buildDataExchangeUrl', () => { + it('builds the hosted consent URL with token + both app-key params', () => { + const url = new URL(buildDataExchangeUrl('tok-9', 'app-42', 'api.example.com')); + expect(url.origin + url.pathname).toBe('https://api.example.com/data-exchange'); + expect(url.searchParams.get('token')).toBe('tok-9'); + expect(url.searchParams.get('app_key')).toBe('app-42'); + expect(url.searchParams.get('x-yvp-app-key')).toBe('app-42'); + }); +}); + +describe('parseDataExchangeCallback', () => { + it('returns null when there is no data_exchange_status', () => { + expect(parseDataExchangeCallback('?state=abc&code=1')).toBeNull(); + }); + + it('parses a granted return with granted_permissions', () => { + expect( + parseDataExchangeCallback('?data_exchange_status=granted&granted_permissions=highlights'), + ).toEqual({ status: 'granted', grantedPermissions: ['highlights'] }); + }); + + it('maps cancel verbatim and treats anything else as failure', () => { + expect(parseDataExchangeCallback('?data_exchange_status=cancel')).toEqual({ + status: 'cancel', + grantedPermissions: [], + }); + expect(parseDataExchangeCallback('?data_exchange_status=weird')).toEqual({ + status: 'failure', + grantedPermissions: [], + }); + // Present but empty value → failure. + expect(parseDataExchangeCallback('?data_exchange_status=')).toEqual({ + status: 'failure', + grantedPermissions: [], + }); + }); +}); + +describe('handleDataExchangeCallback URL cleanup', () => { + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + function stubLocation(href: string): ReturnType { + const replaceState = vi.fn(); + const url = new URL(href); + vi.stubGlobal('window', { + location: { href, search: url.search }, + history: { replaceState }, + }); + return replaceState; + } + + it('strips only the data-exchange params, preserving app params and the hash', () => { + const replaceState = stubLocation( + 'https://app.example.com/read?tab=notes&ref=abc&data_exchange_status=granted&granted_permissions=highlights#section', + ); + + const result = handleDataExchangeCallback(); + expect(result).toEqual({ status: 'granted', grantedPermissions: ['highlights'] }); + + expect(replaceState).toHaveBeenCalledTimes(1); + const cleaned = new URL(replaceState.mock.calls[0]![2] as string); + expect(cleaned.searchParams.get('tab')).toBe('notes'); + expect(cleaned.searchParams.get('ref')).toBe('abc'); + expect(cleaned.searchParams.has('data_exchange_status')).toBe(false); + expect(cleaned.searchParams.has('granted_permissions')).toBe(false); + expect(cleaned.hash).toBe('#section'); + }); + + it('leaves no dangling "?" when only exchange params were present', () => { + const replaceState = stubLocation( + 'https://app.example.com/read?data_exchange_status=granted&granted_permissions=highlights', + ); + + handleDataExchangeCallback(); + + const cleanedUrl = replaceState.mock.calls[0]![2] as string; + expect(cleanedUrl).toBe('https://app.example.com/read'); + expect(cleanedUrl).not.toContain('?'); + }); + + it('returns null and does not touch history when the URL is not a data-exchange return', () => { + const replaceState = stubLocation('https://app.example.com/read?tab=notes'); + expect(handleDataExchangeCallback()).toBeNull(); + expect(replaceState).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/core/src/__tests__/permissions.test.ts b/packages/core/src/__tests__/permissions.test.ts new file mode 100644 index 00000000..52a5ed6b --- /dev/null +++ b/packages/core/src/__tests__/permissions.test.ts @@ -0,0 +1,125 @@ +/** + * @vitest-environment jsdom + */ +import { describe, it, expect, beforeEach } from 'vitest'; +import { parseGrantedPermissions } from '../permissions'; +import { YouVersionPlatformConfiguration } from '../YouVersionPlatformConfiguration'; + +describe('parseGrantedPermissions', () => { + it('parses a single value', () => { + const params = new URLSearchParams('granted_permissions=highlights'); + expect(parseGrantedPermissions(params)).toEqual(['highlights']); + }); + + it('splits comma- and space-separated values and de-duplicates', () => { + const params = new URLSearchParams('granted_permissions=highlights,votd%20bibles'); + expect(parseGrantedPermissions(params)).toEqual(['highlights', 'votd', 'bibles']); + }); + + it('unions repeated params', () => { + const params = new URLSearchParams( + 'granted_permissions=highlights&granted_permissions=votd,highlights', + ); + expect(parseGrantedPermissions(params)).toEqual(['highlights', 'votd']); + }); + + it('returns [] when the param is absent', () => { + expect(parseGrantedPermissions(new URLSearchParams('state=x'))).toEqual([]); + }); +}); + +describe('YouVersionPlatformConfiguration permission cache', () => { + const storageKey = 'youversion-platform:granted-permissions'; + + beforeEach(() => { + localStorage.clear(); + // The cache is scoped to the signed-in user; establish one for the cases + // that exercise reads/writes for a single user. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a' }); + }); + + it('starts empty and merges granted permissions without duplicates', () => { + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights', 'votd']); + + expect(YouVersionPlatformConfiguration.grantedPermissions.sort()).toEqual([ + 'highlights', + 'votd', + ]); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(true); + }); + + it('removeGrantedPermission honors a server 401/403 invalidation', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights', 'votd']); + YouVersionPlatformConfiguration.removeGrantedPermission('highlights'); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + expect(YouVersionPlatformConfiguration.hasPermission('votd')).toBe(true); + }); + + it('clearAuthTokens also clears the permission cache', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + YouVersionPlatformConfiguration.clearAuthTokens(); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + }); + + it('tolerates malformed stored JSON', () => { + localStorage.setItem(storageKey, '{not json'); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + }); + + it('ignores the legacy bare-array format', () => { + localStorage.setItem(storageKey, JSON.stringify(['highlights'])); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + }); + + it('returns [] when signed out even if an entry is stored', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual(['highlights']); + + // Drop the user without clearing the cache entry. + YouVersionPlatformConfiguration.saveUserInfo(null); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + }); + + describe('user scoping (security)', () => { + it('does not leak grants to a different user who signs in without clearAuthTokens', () => { + // User A grants highlights. + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(true); + + // User B signs in (new session) without clearAuthTokens first. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-b' }); + + // B must not inherit A's grant. + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + }); + + it("a callback without a granted_permissions echo does not resurrect the prior user's grants", () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-b' }); + + // A `granted` return with no permissions echoed → nothing is saved. + // B's cache stays empty rather than falling back to A's entry. + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + }); + + it('a different user replaces the entry wholesale on the next save', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights', 'votd']); + + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-b' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['bibles']); + + // Only B's grant is present; A's are gone. + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual(['bibles']); + + // And A cannot read them back either. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a' }); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + }); + }); +}); diff --git a/packages/core/src/data-exchange.ts b/packages/core/src/data-exchange.ts new file mode 100644 index 00000000..9b7a816a --- /dev/null +++ b/packages/core/src/data-exchange.ts @@ -0,0 +1,148 @@ +import type { ApiClient } from './client'; +import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; +import { parseGrantedPermissions } from './permissions'; +import { DataExchangeTokenResponseSchema } from './schemas/data-exchange'; + +/** + * Data exchange is YouVersion's just-in-time permission grant flow: a signed-in + * user who has not yet granted an app a data-exchange permission (e.g. + * `highlights`) is sent to a hosted consent page, and returns with the grant. + * + * The browser flow is: + * 1. {@link DataExchangeClient.updateToken} mints a short-lived token + * (`POST /data-exchange/token`). + * 2. The app full-page redirects to {@link buildDataExchangeUrl}. + * 3. On return, the hosted page appends `data_exchange_status` and + * `granted_permissions`, parsed by {@link parseDataExchangeCallback}. + * + * Mirrors the Swift SDK's `YouVersionAPI.DataExchange` contract. + */ + +export class DataExchangeClient { + private client: ApiClient; + + constructor(client: ApiClient) { + this.client = client; + } + + /** + * Reads the auth token from the argument or the ambient platform + * configuration, mirroring {@link HighlightsClient}. Server-side callers + * (no `localStorage`) must pass `lat` explicitly. + */ + private getAuthToken(lat?: string): string { + if (lat) return lat; + const token = + typeof localStorage === 'undefined' ? null : YouVersionPlatformConfiguration.accessToken; + if (!token) { + throw new Error( + 'Authentication required. Please provide a token or sign in before requesting a data exchange.', + ); + } + return token; + } + + /** + * Mints a short-lived data-exchange token for the given permissions. + * + * `POST https:///data-exchange/token?app-key=` with + * `Authorization: Bearer ` and body + * `{"requested_permissions": [...]}`. Expects `201 { "token": "..." }`. + * + * @throws when no app key or access token is available, or the server + * responds with a non-2xx status (401 = not permitted), or the response + * fails schema validation. + */ + async updateToken(permissions: string[], lat?: string): Promise { + const appKey = this.client.config.appKey; + if (!appKey) { + throw new Error('App key is required to request a data exchange token.'); + } + + const response = await this.client.post( + `/data-exchange/token`, + { requested_permissions: permissions }, + { 'app-key': appKey }, + { Authorization: `Bearer ${this.getAuthToken(lat)}` }, + ); + + const parsed = DataExchangeTokenResponseSchema.safeParse(response); + if (!parsed.success) { + throw new Error(`Unexpected data exchange token response: ${parsed.error.message}`); + } + return parsed.data.token; + } +} + +/** + * Builds the hosted data-exchange consent URL the app full-page redirects to. + * Matches the Swift SDK: `token`, `app_key`, and `x-yvp-app-key` query params. + */ +export function buildDataExchangeUrl( + token: string, + appKey: string, + apiHost: string = YouVersionPlatformConfiguration.apiHost, +): string { + const url = new URL(`https://${apiHost}/data-exchange`); + url.searchParams.set('token', token); + url.searchParams.set('app_key', appKey); + url.searchParams.set('x-yvp-app-key', appKey); + return url.toString(); +} + +/** + * The query params the data-exchange flow appends to the callback URL (parsed by + * {@link parseDataExchangeCallback}). {@link handleDataExchangeCallback} strips + * exactly these on cleanup, leaving any unrelated app params untouched. + */ +const DATA_EXCHANGE_CALLBACK_PARAMS = ['data_exchange_status', 'granted_permissions'] as const; + +export type DataExchangeStatus = 'granted' | 'cancel' | 'failure'; + +export type DataExchangeCallbackResult = { + status: DataExchangeStatus; + grantedPermissions: string[]; +}; + +/** + * Parses a data-exchange return from a URL query string. Pure — no DOM. Returns + * `null` when the query has no `data_exchange_status` (i.e. not a data-exchange + * return). `data_exchange_status` maps `granted`/`cancel` verbatim; anything + * else (including a missing value) is treated as `failure`. + */ +export function parseDataExchangeCallback(search: string): DataExchangeCallbackResult | null { + const params = new URLSearchParams(search); + if (!params.has('data_exchange_status')) return null; + const raw = params.get('data_exchange_status'); + const status: DataExchangeStatus = + raw === 'granted' ? 'granted' : raw === 'cancel' ? 'cancel' : 'failure'; + return { status, grantedPermissions: parseGrantedPermissions(params) }; +} + +/** + * Browser entry point for handling a data-exchange return on page load. Reads + * `window.location.search`; on a `granted` return it reconciles the permission + * cache with the server-reported `granted_permissions`, then strips the query + * params (mirroring the sign-in callback cleanup). Returns the parsed result, or + * `null` when the current URL is not a data-exchange return. + * + * Cleanup surgically removes only the data-exchange params, preserving any + * unrelated app query params and the hash fragment. + */ +export function handleDataExchangeCallback(): DataExchangeCallbackResult | null { + if (typeof window === 'undefined') return null; + const result = parseDataExchangeCallback(window.location.search); + if (!result) return null; + + if (result.status === 'granted' && result.grantedPermissions.length > 0) { + YouVersionPlatformConfiguration.saveGrantedPermissions(result.grantedPermissions); + } + + const cleanUrl = new URL(window.location.href); + for (const param of DATA_EXCHANGE_CALLBACK_PARAMS) { + cleanUrl.searchParams.delete(param); + } + window.history.replaceState({}, '', cleanUrl.toString()); + + return result; +} diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index 4e7f2d3b..c56a29b9 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -202,7 +202,13 @@ export class HighlightsClient { /** * Clears highlights for a passage. * Requires OAuth with write_highlights scope. - * @param passageId The passage identifier (USFM format, e.g., "MAT.1.1" or "MAT.1.1-5"). + * + * UNVERIFIED / likely unsupported: passing a verse RANGE (e.g. "MAT.1.1-5") + * returned a non-2xx from staging (observed with `JHN.1.2-3`), even though the + * API stores highlights per verse and POST accepts ranges. Until the API team + * confirms range delete, callers should send one DELETE per verse passage-id + * (e.g. "MAT.1.1"). See YPE-1034. + * @param passageId The passage identifier (single-verse USFM, e.g., "MAT.1.1"). * @param options Query parameters; `version_id` is required by the API (sent as `bible_id`). * @param lat Optional long access token. If not provided, retrieves from YouVersionPlatformConfiguration. * @returns Promise that resolves when highlights are deleted (204 response). diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 884b741b..0979c693 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -7,6 +7,15 @@ export { type GetHighlightsOptions, type DeleteHighlightOptions, } from './highlights'; +export { + DataExchangeClient, + buildDataExchangeUrl, + parseDataExchangeCallback, + handleDataExchangeCallback, + type DataExchangeStatus, + type DataExchangeCallbackResult, +} from './data-exchange'; +export { parseGrantedPermissions } from './permissions'; export * from './StorageStrategy'; export * from './Users'; export * from './YouVersionUserInfo'; diff --git a/packages/core/src/permissions.ts b/packages/core/src/permissions.ts new file mode 100644 index 00000000..e4304bda --- /dev/null +++ b/packages/core/src/permissions.ts @@ -0,0 +1,26 @@ +/** + * YouVersion data-exchange permissions are open-ended strings (not enums): the + * `highlights` permission ships now, more (e.g. verse notes) arrive later. The + * server communicates granted permissions back to the app on the sign-in and + * data-exchange callbacks via `granted_permissions` query param(s). + * + * These helpers are pure (no DOM/storage), so they can be unit-tested and reused + * by both the sign-in callback handler and the data-exchange callback handler. + */ + +/** + * Parses `granted_permissions` from a set of callback query params. + * + * The param may repeat and each value may pack several permissions separated by + * a comma or whitespace (mirrors the Swift SDK's split on `,`/` `). Returns a + * de-duplicated list, order-preserving on first appearance. + */ +export function parseGrantedPermissions(params: URLSearchParams): string[] { + const seen = new Set(); + for (const value of params.getAll('granted_permissions')) { + for (const part of value.split(/[,\s]+/)) { + if (part) seen.add(part); + } + } + return [...seen]; +} diff --git a/packages/core/src/schemas/data-exchange.ts b/packages/core/src/schemas/data-exchange.ts new file mode 100644 index 00000000..aa2ee6ee --- /dev/null +++ b/packages/core/src/schemas/data-exchange.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; + +/** + * Response of `POST /data-exchange/token`: a short-lived data-exchange token + * minted for the requested permissions (see {@link DataExchangeClient}). + */ +export const DataExchangeTokenResponseSchema = z.object({ + token: z.string().min(1), +}); + +export type DataExchangeTokenResponse = z.infer; diff --git a/packages/core/src/schemas/index.ts b/packages/core/src/schemas/index.ts index 063f9f18..c6c4991d 100644 --- a/packages/core/src/schemas/index.ts +++ b/packages/core/src/schemas/index.ts @@ -3,6 +3,7 @@ export * from './bible-index'; export * from './book'; export * from './chapter'; export * from './collection'; +export * from './data-exchange'; export * from './font'; export * from './highlight'; export * from './language'; diff --git a/packages/core/src/styles/bible-reader.css b/packages/core/src/styles/bible-reader.css index cc0c289b..9c34a632 100644 --- a/packages/core/src/styles/bible-reader.css +++ b/packages/core/src/styles/bible-reader.css @@ -123,6 +123,20 @@ display: inline; } + /* Fade highlight fills in/out instead of popping. The fill is painted as an + inline `background-color` on `.yv-v[v]` (see verse.tsx); transitioning only + that property animates apply/remove without touching the selection + underline (text-decoration) and without any layout shift. */ + & .yv-v { + transition: background-color 250ms ease; + } + + @media (prefers-reduced-motion: reduce) { + & .yv-v { + transition: none; + } + } + /* Only show pointer cursor when verses are selectable */ &[data-selectable='true'] .yv-v, &[data-selectable='true'] .verse { diff --git a/packages/hooks/src/index.ts b/packages/hooks/src/index.ts index c6b39d73..0d44d571 100644 --- a/packages/hooks/src/index.ts +++ b/packages/hooks/src/index.ts @@ -17,6 +17,7 @@ export * from './useBibleClient'; export * from './usePassage'; export * from './useVOTD'; export * from './useHighlights'; +export * from './useHighlightAuthActions'; export * from './useLanguages'; export * from './useLanguage'; export * from './useTheme'; diff --git a/packages/hooks/src/useHighlightAuthActions.test.tsx b/packages/hooks/src/useHighlightAuthActions.test.tsx new file mode 100644 index 00000000..00b683d2 --- /dev/null +++ b/packages/hooks/src/useHighlightAuthActions.test.tsx @@ -0,0 +1,85 @@ +import { renderHook } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ReactNode } from 'react'; +import { + DataExchangeClient, + YouVersionAPIUsers, + YouVersionPlatformConfiguration, +} from '@youversion/platform-core'; +import { YouVersionContext } from './context'; +import { YouVersionAuthContext } from './context/YouVersionAuthContext'; +import { useHighlightAuthActions } from './useHighlightAuthActions'; + +function wrapper({ children }: { children: ReactNode }) { + return ( + + + {children} + + + ); +} + +describe('useHighlightAuthActions', () => { + beforeEach(() => { + localStorage.clear(); + // A writable location stub so href assignment doesn't hit jsdom navigation. + Object.defineProperty(window, 'location', { + value: { href: 'https://host.example/read' }, + writable: true, + configurable: true, + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('reads and invalidates the highlights permission cache', () => { + // The permission cache is scoped to the signed-in user, so establish one. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1' }); + const { result } = renderHook(() => useHighlightAuthActions(), { wrapper }); + + expect(result.current.hasHighlightsPermission()).toBe(false); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + expect(result.current.hasHighlightsPermission()).toBe(true); + + result.current.invalidateHighlightsPermission(); + expect(result.current.hasHighlightsPermission()).toBe(false); + }); + + it('starts sign-in requesting profile + highlights via the configured redirect', async () => { + const signIn = vi.spyOn(YouVersionAPIUsers, 'signIn').mockResolvedValue(undefined); + const { result } = renderHook(() => useHighlightAuthActions(), { wrapper }); + + await result.current.startSignInForHighlights(); + + expect(signIn).toHaveBeenCalledWith( + 'https://host.example/callback', + ['profile'], + ['highlights'], + ); + }); + + it('mints a data-exchange token then redirects to the hosted consent page', async () => { + const updateToken = vi + .spyOn(DataExchangeClient.prototype, 'updateToken') + .mockResolvedValue('dx-token'); + const { result } = renderHook(() => useHighlightAuthActions(), { wrapper }); + + await result.current.startDataExchangeForHighlights(); + + expect(updateToken).toHaveBeenCalledWith(['highlights']); + expect(window.location.href).toContain('https://api.example.com/data-exchange'); + expect(window.location.href).toContain('token=dx-token'); + expect(window.location.href).toContain('app_key=app-1'); + }); +}); diff --git a/packages/hooks/src/useHighlightAuthActions.ts b/packages/hooks/src/useHighlightAuthActions.ts new file mode 100644 index 00000000..79f846bc --- /dev/null +++ b/packages/hooks/src/useHighlightAuthActions.ts @@ -0,0 +1,111 @@ +'use client'; + +import { useCallback, useContext, useMemo } from 'react'; +import { + ApiClient, + DataExchangeClient, + buildDataExchangeUrl, + handleDataExchangeCallback, + SignInWithYouVersionPermission, + YouVersionAPIUsers, + YouVersionPlatformConfiguration, + type DataExchangeCallbackResult, +} from '@youversion/platform-core'; +import { YouVersionContext } from './context'; +import { YouVersionAuthContext } from './context/YouVersionAuthContext'; + +const HIGHLIGHTS_PERMISSION = SignInWithYouVersionPermission.highlights; + +/** Optimistic cache read; a 401/403 on a write is still the ultimate check. */ +const hasHighlightsPermission = () => + YouVersionPlatformConfiguration.hasPermission(HIGHLIGHTS_PERMISSION); + +/** Drops the cached `highlights` grant so the next attempt re-prompts. */ +const invalidateHighlightsPermission = () => + YouVersionPlatformConfiguration.removeGrantedPermission(HIGHLIGHTS_PERMISSION); + +/** + * Reconciles the permission cache from a data-exchange return and cleans the + * URL. Returns the parsed return (or `null` when this load is not one). + */ +const consumeDataExchangeReturn = () => handleDataExchangeCallback(); + +/** + * The two-path auth actions the highlight auth flow (YPE-1034) needs, plus the + * permission-cache reads. Kept UI-agnostic: it returns primitives the seam hook + * (`useBibleReaderHighlights`) orchestrates; it renders nothing. + * + * Reads `YouVersionAuthContext` defensively (via `useContext`, not `useYVAuth`) + * so it never throws when no auth provider is mounted — the seam hook treats "no + * provider" and "signed out" the same and simply never invokes the redirects. + */ +export function useHighlightAuthActions(): { + /** Optimistic cache read; a 401/403 on a write is still the ultimate check. */ + hasHighlightsPermission: () => boolean; + /** Drops the cached `highlights` grant so the next attempt re-prompts. */ + invalidateHighlightsPermission: () => void; + /** + * Reconciles the permission cache from a data-exchange return and cleans the + * URL. Returns the parsed return (or `null` when this load is not one). + */ + consumeDataExchangeReturn: () => DataExchangeCallbackResult | null; + /** + * One-fell-swoop: full-page redirect to sign-in, requesting the `highlights` + * permission alongside `profile`. Returns to `redirectUrl` (or the auth + * provider's configured `redirectUri`, falling back to the current URL). + */ + startSignInForHighlights: (redirectUrl?: string) => Promise; + /** + * Just-in-time: mint a data-exchange token then full-page redirect to the + * hosted consent page for the `highlights` permission. + */ + startDataExchangeForHighlights: () => Promise; +} { + const context = useContext(YouVersionContext); + const authContext = useContext(YouVersionAuthContext); + const redirectUri = authContext?.redirectUri; + + const dataExchangeClient = useMemo(() => { + if (!context?.appKey) return null; + return new DataExchangeClient( + new ApiClient({ + appKey: context.appKey, + apiHost: context.apiHost, + installationId: context.installationId, + additionalHeaders: context.additionalHeaders, + }), + ); + }, [context?.appKey, context?.apiHost, context?.installationId, context?.additionalHeaders]); + + const startSignInForHighlights = useCallback( + async (redirectUrl?: string) => { + const url = + redirectUrl ?? + redirectUri ?? + (typeof window !== 'undefined' ? window.location.href : undefined); + if (!url) { + throw new Error('A redirect URL is required to start sign-in for highlights.'); + } + await YouVersionAPIUsers.signIn(url, ['profile'], [HIGHLIGHTS_PERMISSION]); + }, + [redirectUri], + ); + + const startDataExchangeForHighlights = useCallback(async () => { + if (!dataExchangeClient || !context?.appKey) { + throw new Error('YouVersion context is required to start a data exchange.'); + } + const token = await dataExchangeClient.updateToken([HIGHLIGHTS_PERMISSION]); + if (typeof window !== 'undefined') { + window.location.href = buildDataExchangeUrl(token, context.appKey, context.apiHost); + } + }, [dataExchangeClient, context?.appKey, context?.apiHost]); + + return { + hasHighlightsPermission, + invalidateHighlightsPermission, + consumeDataExchangeReturn, + startSignInForHighlights, + startDataExchangeForHighlights, + }; +} diff --git a/packages/hooks/src/useHighlights.test.tsx b/packages/hooks/src/useHighlights.test.tsx index 73b1dc64..ed65c255 100644 --- a/packages/hooks/src/useHighlights.test.tsx +++ b/packages/hooks/src/useHighlights.test.tsx @@ -218,13 +218,15 @@ describe('useHighlights', () => { }); describe('createHighlight mutation', () => { - it('should create highlight and refetch', async () => { + it('should create highlight WITHOUT auto-refetching (callers coalesce refetches)', async () => { const wrapper = createYVWrapper(); const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); }); + // The mount fetch. + expect(mockGetHighlights).toHaveBeenCalledTimes(1); const createData: CreateHighlight = { version_id: 111, @@ -241,6 +243,13 @@ describe('useHighlights', () => { const created = await createPromise; expect(created).toEqual(mockHighlight); + // No implicit GET after the write — the mount fetch is still the only one. + // A batching caller issues one refetch() after the whole batch settles. + await Promise.resolve(); + expect(mockGetHighlights).toHaveBeenCalledTimes(1); + + // An explicit refetch still works and issues exactly one GET. + result.current.refetch(); await waitFor(() => { expect(mockGetHighlights).toHaveBeenCalledTimes(2); }); @@ -272,13 +281,14 @@ describe('useHighlights', () => { }); describe('deleteHighlight mutation', () => { - it('should delete highlight and refetch', async () => { + it('should delete highlight WITHOUT auto-refetching (callers coalesce refetches)', async () => { const wrapper = createYVWrapper(); const { result } = renderHook(() => useHighlights(defaultOptions), { wrapper }); await waitFor(() => { expect(result.current.loading).toBe(false); }); + expect(mockGetHighlights).toHaveBeenCalledTimes(1); const deletePromise = result.current.deleteHighlight('MAT.1.1', { version_id: 111 }); @@ -288,9 +298,9 @@ describe('useHighlights', () => { await deletePromise; - await waitFor(() => { - expect(mockGetHighlights).toHaveBeenCalledTimes(2); - }); + // No implicit GET after the delete. + await Promise.resolve(); + expect(mockGetHighlights).toHaveBeenCalledTimes(1); }); it('should handle delete error', async () => { diff --git a/packages/hooks/src/useHighlights.ts b/packages/hooks/src/useHighlights.ts index 9dac8eb3..2dc62819 100644 --- a/packages/hooks/src/useHighlights.ts +++ b/packages/hooks/src/useHighlights.ts @@ -21,7 +21,17 @@ export function useHighlights( loading: boolean; error: Error | null; refetch: () => void; + /** + * Creates a highlight. Intentionally does NOT auto-refetch: a single logical + * apply can fan out into several writes, so callers must call `refetch()` once + * after their write batch settles. See the NOTE in the hook body for the why. + */ createHighlight: (data: CreateHighlight) => Promise; + /** + * Deletes a highlight. Intentionally does NOT auto-refetch: callers must call + * `refetch()` once after their write batch settles. See the NOTE in the hook + * body for the why. + */ deleteHighlight: (passageId: string, deleteOptions: DeleteHighlightOptions) => Promise; } { const context = useContext(YouVersionContext); @@ -54,21 +64,21 @@ export function useHighlights( }, ); + // NOTE: these mutations intentionally do NOT auto-refetch. A single logical + // apply/remove can fan out into several writes (one per contiguous run for + // apply, one per verse for delete); auto-refetching per call would issue a + // GET per write. Callers coalesce instead — issue the batch, then `refetch()` + // once after it settles. The seam hook (`useBibleReaderHighlights`) is the + // sole consumer and does exactly that. const createHighlight = useCallback( - async (data: CreateHighlight): Promise => { - const result = await highlightsClient.createHighlight(data); - refetch(); - return result; - }, - [highlightsClient, refetch], + (data: CreateHighlight): Promise => highlightsClient.createHighlight(data), + [highlightsClient], ); const deleteHighlight = useCallback( - async (passageId: string, deleteOptions: DeleteHighlightOptions): Promise => { - await highlightsClient.deleteHighlight(passageId, deleteOptions); - refetch(); - }, - [highlightsClient, refetch], + (passageId: string, deleteOptions: DeleteHighlightOptions): Promise => + highlightsClient.deleteHighlight(passageId, deleteOptions), + [highlightsClient], ); return { diff --git a/packages/ui/package.json b/packages/ui/package.json index ef6778dc..2577c163 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -51,6 +51,7 @@ "@radix-ui/react-slot": "1.2.3", "@radix-ui/react-tabs": "1.1.13", "@radix-ui/react-use-controllable-state": "^1.2.2", + "@xstate/react": "^6.1.0", "@youversion/platform-core": "workspace:*", "@youversion/platform-react-hooks": "workspace:*", "better-result": "2.9.2", @@ -60,7 +61,8 @@ "radix-ui": "^1.4.3", "react-i18next": "^17.0.0", "tailwind-merge": "3.3.1", - "tw-animate-css": "1.4.0" + "tw-animate-css": "1.4.0", + "xstate": "5.32.5" }, "peerDependencies": { "react": ">=19.1.0 <20.0.0", diff --git a/packages/ui/src/components/YouVersionAuthButton.tsx b/packages/ui/src/components/YouVersionAuthButton.tsx index 019a4e43..1ae3bb5d 100644 --- a/packages/ui/src/components/YouVersionAuthButton.tsx +++ b/packages/ui/src/components/YouVersionAuthButton.tsx @@ -198,6 +198,11 @@ export const YouVersionAuthButton = React.forwardRef; resolve: (value?: unknown) => void }; +function deferred(): Deferred { + let resolve!: (value?: unknown) => void; + const promise = new Promise((res) => { + resolve = res as (value?: unknown) => void; + }); + return { promise, resolve }; +} + +function makeServices(overrides: Partial = {}): { + ref: HighlightServicesRef; + refetch: ReturnType; +} { + const refetch = vi.fn(); + const services: HighlightServices = { + createHighlight: vi.fn().mockResolvedValue(undefined), + deleteHighlight: vi.fn().mockResolvedValue(undefined), + refetch, + hasHighlightsPermission: () => true, + invalidateHighlightsPermission: vi.fn(), + consumeDataExchangeReturn: () => null, + startSignInForHighlights: vi.fn().mockResolvedValue(undefined), + startDataExchangeForHighlights: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; + return { ref: { current: services }, refetch }; +} + +const scopeJHN3: HighlightScope = { versionId: 111, book: 'JHN', chapter: '3' }; +const scopeJHN4: HighlightScope = { versionId: 111, book: 'JHN', chapter: '4' }; + +function startMachine(ref: HighlightServicesRef, scope: HighlightScope = scopeJHN3) { + const actor = createActor(bibleReaderHighlightsMachine, { + input: { services: ref, scope, flagOn: true, hasAuthProvider: true, isAuthenticated: true }, + }); + actor.start(); + return actor; +} + +afterEach(() => { + vi.restoreAllMocks(); + localStorage.clear(); + sessionStorage.clear(); +}); + +describe('bibleReaderHighlightsMachine — writeIntent lifecycle', () => { + it('releases a verse writeIntent entry once its write settles', async () => { + const { ref, refetch } = makeServices(); + const actor = startMachine(ref); + + actor.send({ type: 'TAP_COLOR', color: 'FFFE00', verses: [16] }); + // Claimed synchronously before the write settles. + expect(actor.getSnapshot().context.writeIntent.get(16)).toBeDefined(); + + await vi.waitFor(() => expect(refetch).toHaveBeenCalled()); + + const ctx = actor.getSnapshot().context; + expect(ctx.writeIntent.has(16)).toBe(false); + expect(ctx.reconcile.get(16)).toEqual({ op: 'apply', color: 'fffe00' }); + actor.stop(); + }); + + it('does not let an old-scope write pollute the new scope after SCOPE_CHANGED', async () => { + const pending = deferred(); + const createHighlight = vi.fn().mockReturnValue(pending.promise); + const { ref, refetch } = makeServices({ createHighlight }); + const actor = startMachine(ref); + + actor.send({ type: 'TAP_COLOR', color: 'FFFE00', verses: [16] }); + await vi.waitFor(() => expect(createHighlight).toHaveBeenCalled()); + expect(actor.getSnapshot().context.overlay).toEqual({ 16: 'fffe00' }); + + // Navigate to a new chapter while the old-scope write is still in flight. + actor.send({ type: 'SCOPE_CHANGED', scope: scopeJHN4 }); + expect(actor.getSnapshot().context.overlay).toEqual({}); + expect(actor.getSnapshot().context.writeIntent.size).toBe(0); + + // The old-scope (JHN.3) write settles after the scope change. + pending.resolve(undefined); + await vi.waitFor(() => expect(refetch).toHaveBeenCalled()); + + // Verse 16 in the NEW scope must be untouched by the JHN.3 write. + const ctx = actor.getSnapshot().context; + expect(ctx.reconcile.size).toBe(0); + expect(ctx.overlay).toEqual({}); + expect(ctx.writeIntent.size).toBe(0); + actor.stop(); + }); + + it("preserves a newer write's claim when an older write settles on the same verse", async () => { + const first = deferred(); + const second = deferred(); + const queued = [first, second]; + let callIndex = 0; + const createHighlight = vi.fn().mockImplementation(() => queued[callIndex++]!.promise); + const { ref, refetch } = makeServices({ createHighlight }); + const actor = startMachine(ref); + + // Write A: apply red to verse 16 (goes in flight). + actor.send({ type: 'TAP_COLOR', color: 'FF0000', verses: [16] }); + await vi.waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(1)); + const claimA = actor.getSnapshot().context.writeIntent.get(16); + expect(claimA).toBeDefined(); + + // Write B: re-claim verse 16 with green while A is still in flight (queued). + actor.send({ type: 'TAP_COLOR', color: '00FF00', verses: [16] }); + const claimB = actor.getSnapshot().context.writeIntent.get(16); + expect(claimB).toBeDefined(); + expect(claimB).not.toBe(claimA); + expect(actor.getSnapshot().context.overlay).toEqual({ 16: '00ff00' }); + + // A settles: it must not delete B's claim or reconcile verse 16. + first.resolve(undefined); + await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(1)); + const afterA = actor.getSnapshot().context; + expect(afterA.writeIntent.get(16)).toBe(claimB); + expect(afterA.reconcile.has(16)).toBe(false); + expect(afterA.overlay).toEqual({ 16: '00ff00' }); + + // B settles: it cleans up its own claim and registers its reconcile entry. + await vi.waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(2)); + second.resolve(undefined); + await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); + const afterB = actor.getSnapshot().context; + expect(afterB.writeIntent.has(16)).toBe(false); + expect(afterB.reconcile.get(16)).toEqual({ op: 'apply', color: '00ff00' }); + actor.stop(); + }); +}); + +describe('bibleReaderHighlightsMachine — pending stash on lost permission', () => { + it('preserves BOTH intents when two queued apply writes each lose permission', async () => { + // The exact review scenario: tap color A on verses 1-3, then color B on 4-6 + // before the first write settles; both 401. A single-slot stash would let the + // second settle overwrite the first, losing color A after the re-grant. + const createHighlight = vi.fn().mockRejectedValue(httpError(401)); + const { ref, refetch } = makeServices({ createHighlight }); + const actor = startMachine(ref); + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + + // Both taps issued before either write settles: A is writing, B is queued. + actor.send({ type: 'TAP_COLOR', color: 'AAAAAA', verses: [1, 2, 3] }); + actor.send({ type: 'TAP_COLOR', color: 'BBBBBB', verses: [4, 5, 6] }); + + await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); + + const stash = peekPendingHighlights(); + expect(stash).toHaveLength(2); + // Verse-level ordering deterministic: first-queued (A) first. + expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'aaaaaa' }); + expect(stash[1]).toMatchObject({ verses: [4, 5, 6], color: 'bbbbbb' }); + actor.stop(); + }); + + it('keeps a sibling’s stashed intent when a later write fails with a network/5xx error', async () => { + // Write 1 (color A) 401s and stashes; write 2 (color B) then fails 5xx. The + // 5xx path must NOT clear the stash — write 2 never stashed anything of its + // own, and write 1's intent must survive to resume after the re-grant. + const createHighlight = vi + .fn() + .mockRejectedValueOnce(httpError(401)) + .mockRejectedValueOnce(httpError(500)); + const { ref, refetch } = makeServices({ createHighlight }); + const actor = startMachine(ref); + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + + actor.send({ type: 'TAP_COLOR', color: 'AAAAAA', verses: [1, 2, 3] }); + actor.send({ type: 'TAP_COLOR', color: 'BBBBBB', verses: [4, 5, 6] }); + + await vi.waitFor(() => expect(refetch).toHaveBeenCalledTimes(2)); + + const stash = peekPendingHighlights(); + expect(stash).toHaveLength(1); + expect(stash[0]).toMatchObject({ verses: [1, 2, 3], color: 'aaaaaa' }); + // The 5xx write's verses were never stashed. + expect(stash.some((entry) => entry.verses.includes(4))).toBe(false); + actor.stop(); + }); + + it('re-applies every stashed entry after a restart with permission granted', async () => { + // Two entries survived a data-exchange redirect; on the granted return the + // machine restarts and must resume ALL of them, not just the last. + const now = Date.now(); + appendPendingHighlight( + { + verses: [1, 2, 3], + color: 'aaaaaa', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: now, + }, + now, + ); + appendPendingHighlight( + { + verses: [4, 5, 6], + color: 'bbbbbb', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: now, + }, + now, + ); + const createHighlight = vi.fn().mockResolvedValue(undefined); + const { ref } = makeServices({ createHighlight }); + const actor = startMachine(ref); + + await vi.waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(2)); + + const passages = createHighlight.mock.calls.map( + (call) => (call[0] as { passage_id: string }).passage_id, + ); + expect(passages).toEqual(['JHN.3.1-3', 'JHN.3.4-6']); + // Both colors painted in the same-scope overlay. + expect(actor.getSnapshot().context.overlay).toEqual({ + 1: 'aaaaaa', + 2: 'aaaaaa', + 3: 'aaaaaa', + 4: 'bbbbbb', + 5: 'bbbbbb', + 6: 'bbbbbb', + }); + // Pending consumed once resumed. + expect(readPendingHighlights()).toEqual([]); + actor.stop(); + }); +}); diff --git a/packages/ui/src/components/bible-reader-highlights-machine.ts b/packages/ui/src/components/bible-reader-highlights-machine.ts new file mode 100644 index 00000000..1901b00d --- /dev/null +++ b/packages/ui/src/components/bible-reader-highlights-machine.ts @@ -0,0 +1,801 @@ +/** + * BibleReader highlights flow as an xstate v5 statechart (YPE-1034, PR-288). + * + * This machine is the single source of truth for the highlight auth flow and the + * optimistic write path. It replaces the hand-rolled effect/ref orchestration in + * `useBibleReaderHighlights`, which is now a thin adapter (see that file). The + * machine is authored with `setup()` and NAMED guards/actions/actors so it stays + * statically analyzable / Stately-visualizable — no anonymous inline logic where + * a named implementation is possible. + * + * Statechart shape (see docs/highlight-flow-statechart.md for the mermaid diagram): + * + * booting ─(always)─▶ disabled | enabled + * disabled ── flag off OR no auth provider: fully inert (no fetch, writes, + * dialogs). Color taps resolve to `noop`. + * enabled ── parallel { flow, writer } + * flow region (the auth/dialog flow) + * resuming ─ consume the data-exchange return once, then route on the + * pending highlight + auth + permission. + * idle · signInDialog · permissionDialog · awaitingAuth + * writer region (serialized optimistic writes) + * idle ─(queue has work)─▶ writing ─(actor done)─▶ checkQueue ─▶ … + * + * External inputs (fetched highlights, auth flags, scope) are delivered as + * events by the adapter; the machine never reaches into React. + * + * ── The "vapor" fix (PR-288) ────────────────────────────────────────────── + * Reported on staging: a deleted highlight reappears for a split second, then + * disappears. Root cause (confirmed): the reconcile step retired a REMOVE + * overlay entry as soon as ANY fetch reflected the removal; a later response + * from a stale read replica that still contained the highlight then had nothing + * suppressing it, so the verse repainted until the next fetch cleared it. + * + * Fix (this machine): `reconcileOverlay` NEVER retires remove-overlay entries. + * A removed verse's optimistic `null` overlay is held until a reset path + * (scope change, sign-out, or a newer write re-claiming the verse). Apply + * entries still retire on reflection, because the existing convergence tests + * assert that after a fetch reflects an apply, later server-side changes to that + * verse render (other-device convergence). Removes have no such convergence + * test, and holding them is exactly the PR's already-accepted trade-off: + * "a concurrent same-verse edit from another device renders stale until + * navigation or the next write on this client." This is fix direction (a) from + * the brief, applied only to the remove side (removes can never resurrect), + * leaving the tested apply-convergence behavior untouched. + */ +import { collapseVerseRuns, formatPassageId, type VerseRun } from '@/lib/usfm-ranges'; +import { + appendPendingHighlight, + clearPendingHighlight, + peekPendingHighlights, + readPendingHighlights, + stashPendingHighlight, + type PendingHighlight, +} from '@/lib/pending-highlight'; +import { Result } from 'better-result'; +import { assign, enqueueActions, fromPromise, setup, type DoneActorEvent } from 'xstate'; + +// ── Domain types ──────────────────────────────────────────────────────────── + +export type HighlightScope = { versionId: number; book: string; chapter: string }; + +/** + * The data-exchange return statuses. Mirrors core's `DataExchangeStatus` (UI + * must not import core directly — see the package boundary rules). + */ +type DataExchangeStatus = 'granted' | 'cancel' | 'failure'; + +/** Verse number → hex color for the current scope, parsed from the fetch. */ +export type ServerColors = Record; + +/** hex color means "optimistically applied", `null` means "optimistically removed". */ +export type HighlightOverlay = Record; + +type ReconcileEntry = { op: 'apply' | 'remove'; color: string }; + +/** One serialized unit of work on the write queue. */ +type WriteOp = { + kind: 'apply' | 'remove'; + color: string; + verses: number[]; + scope: HighlightScope; + /** Per-verse ownership token; a failed write only reverts verses it still owns. */ + token: object; + /** Whether the optimistic overlay was painted for this op (owns its verses). */ + paint: boolean; + /** + * Whether a 401/403 should keep the pending highlight + re-prompt. `true` for a + * user-initiated apply; `false` for a remove (invalidate only — no re-prompt, + * fixing the old deferred wart) and for a resume-applied pending highlight. + */ + reprompt: boolean; +}; + +type WriteResult = { + op: WriteOp; + failures: BibleReaderHighlightError[]; + failedVerses: number[]; + succeededVerses: number[]; +}; + +/** + * The live SDK service functions the actors/actions call. Passed in once via + * `input` as a stable React ref so the machine spawn never changes yet always + * sees the current closures. + */ +export type HighlightServices = { + createHighlight: (data: { + version_id: number; + passage_id: string; + color: string; + }) => Promise; + deleteHighlight: (passageId: string, options: { version_id: number }) => Promise; + refetch: () => void; + hasHighlightsPermission: () => boolean; + invalidateHighlightsPermission: () => void; + consumeDataExchangeReturn: () => { status: DataExchangeStatus } | null; + startSignInForHighlights: () => Promise; + startDataExchangeForHighlights: () => Promise; +}; + +export type HighlightServicesRef = { current: HighlightServices }; + +export type HighlightsMachineInput = { + services: HighlightServicesRef; + scope: HighlightScope; + flagOn: boolean; + hasAuthProvider: boolean; + isAuthenticated: boolean; +}; + +export type TapOutcome = 'applied' | 'flow' | 'noop'; + +type HighlightsContext = { + services: HighlightServicesRef; + scope: HighlightScope; + flagOn: boolean; + hasAuthProvider: boolean; + isAuthenticated: boolean; + serverColors: ServerColors; + overlay: HighlightOverlay; + reconcile: Map; + writeIntent: Map; + queue: WriteOp[]; + dataExchangeConsumed: boolean; + /** Set by the TAP_COLOR handlers so the adapter can read the synchronous outcome. */ + lastTapOutcome: TapOutcome; +}; + +export type HighlightsEvent = + | { type: 'TAP_COLOR'; color: string; verses: number[] } + | { type: 'REMOVE'; color: string; verses: number[] } + | { + type: 'AUTH_CHANGED'; + flagOn: boolean; + hasAuthProvider: boolean; + isAuthenticated: boolean; + } + | { type: 'HIGHLIGHTS_UPDATED'; serverColors: ServerColors } + | { type: 'SCOPE_CHANGED'; scope: HighlightScope } + | { type: 'CONFIRM_SIGN_IN' } + | { type: 'DECLINE_SIGN_IN' } + | { type: 'CONFIRM_PERMISSION' } + | { type: 'CANCEL_PERMISSION' } + | { type: 'ENQUEUE'; op: WriteOp } + | { type: 'PERMISSION_LOST' }; + +// ── Error boundary + helpers (module-level, reused by the write actor) ──────── + +/** + * The error type the 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. + */ +export 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; + } +} + +/** Expands a contiguous run back into its verse numbers: `{2,4} -> [2,3,4]`. */ +function versesInRun(run: VerseRun): number[] { + const verses: number[] = []; + for (let verse = run.start; verse <= run.end; verse++) verses.push(verse); + return verses; +} + +/** Pulls an HTTP status off a thrown ApiClient error (possibly wrapped). */ +function extractStatus(error: unknown): number | undefined { + if (error instanceof BibleReaderHighlightError) return extractStatus(error.cause); + if (typeof error === 'object' && error !== null && 'status' in error) { + const status = (error as { status?: unknown }).status; + return typeof status === 'number' ? status : undefined; + } + return undefined; +} + +/** + * A 401/403 means the app lost (or never had) the highlights permission. + * + * DEFERRED (accepted for dark launch): a 401 from an *expired token* (not a + * missing permission) also lands here and misroutes to the permission re-prompt + * instead of a token refresh / re-auth. Follow-up: distinguish auth-expiry from + * permission-denied at this boundary. + */ +function isPermissionError(error: unknown): boolean { + const status = extractStatus(error); + return status === 401 || status === 403; +} + +function logWriteFailures(failures: BibleReaderHighlightError[], color: string): void { + for (const failure of failures) { + console.error( + `[YouVersion SDK] Failed to ${failure.operation} highlight ` + + `(passage ${failure.passageId}, color ${color})`, + failure, + ); + } +} + +function opToPending(op: WriteOp): PendingHighlight { + return { + verses: op.verses, + color: op.color, + versionId: op.scope.versionId, + book: op.scope.book, + chapter: op.scope.chapter, + timestamp: Date.now(), + }; +} + +export function scopesEqual(a: HighlightScope, b: HighlightScope): boolean { + return a.versionId === b.versionId && a.book === b.book && a.chapter === b.chapter; +} + +/** + * Claims a set of verses for a write: stamps each with the op's ownership + * `token`, drops any pending reconciliation (a newer write supersedes it), and + * paints the optimistic overlay (`color` for an apply, `null` for a remove). + * Shared by every write entry point so the claim shape stays in one place. + */ +function claimVerses( + context: Pick, + verses: number[], + token: object, + color: string | null, +): Pick { + const writeIntent = new Map(context.writeIntent); + const reconcile = new Map(context.reconcile); + const overlay = { ...context.overlay }; + for (const verse of verses) { + writeIntent.set(verse, token); + reconcile.delete(verse); + overlay[verse] = color; + } + return { writeIntent, reconcile, overlay }; +} + +/** The rendered verse→color map: server truth with the optimistic overlay applied. */ +export function selectHighlightedVerses( + serverColors: ServerColors, + overlay: HighlightOverlay, +): Record { + const map: Record = { ...serverColors }; + for (const [verse, color] of Object.entries(overlay)) { + if (color === null) delete map[Number(verse)]; + else map[Number(verse)] = color; + } + return map; +} + +// ── The write actor ────────────────────────────────────────────────────────── + +/** + * Performs one queued write. Apply POSTs collapse contiguous runs to range + * USFMs (one POST per run); remove DELETEs are PER VERSE (range DELETE is + * unsupported server-side). Every sub-write is caught with better-result so the + * actor always RESOLVES with a per-sub-write settlement — a batch can partially + * succeed and the two halves need opposite treatment in `settleWrite`. + */ +const processWrite = fromPromise( + async ({ input }) => { + const svc = input.services.current; + const { op } = input; + const failures: BibleReaderHighlightError[] = []; + const failedVerses: number[] = []; + const succeededVerses: number[] = []; + + if (op.kind === 'apply') { + const runs = collapseVerseRuns(op.verses); + const results = await Promise.all( + runs.map((run) => + Result.tryPromise({ + try: () => + svc.createHighlight({ + version_id: op.scope.versionId, + passage_id: formatPassageId(op.scope.book, op.scope.chapter, run), + color: op.color, + }), + catch: (cause) => + new BibleReaderHighlightError( + 'apply', + formatPassageId(op.scope.book, op.scope.chapter, run), + cause, + ), + }), + ), + ); + runs.forEach((run, index) => { + const result = results[index]; + if (result === undefined) return; // unreachable: 1:1 with runs + const runVerses = versesInRun(run); + if (Result.isError(result)) { + failures.push(result.error); + failedVerses.push(...runVerses); + } else { + succeededVerses.push(...runVerses); + } + }); + } else { + const results = await Promise.all( + op.verses.map((verse) => { + const passageId = `${op.scope.book}.${op.scope.chapter}.${verse}`; + return Result.tryPromise({ + try: () => svc.deleteHighlight(passageId, { version_id: op.scope.versionId }), + catch: (cause) => new BibleReaderHighlightError('remove', passageId, cause), + }); + }), + ); + op.verses.forEach((verse, index) => { + const result = results[index]; + if (result === undefined) return; // unreachable: 1:1 with verses + if (Result.isError(result)) { + failures.push(result.error); + failedVerses.push(verse); + } else { + succeededVerses.push(verse); + } + }); + } + + return { op, failures, failedVerses, succeededVerses }; + }, +); + +// ── Machine ─────────────────────────────────────────────────────────────────── + +export const bibleReaderHighlightsMachine = setup({ + types: { + context: {} as HighlightsContext, + events: {} as HighlightsEvent, + input: {} as HighlightsMachineInput, + }, + actors: { processWrite }, + guards: { + isEnabledNow: ({ context }) => context.flagOn && context.hasAuthProvider, + isDisabledNow: ({ context }) => !context.flagOn || !context.hasAuthProvider, + scopeIsDifferent: ({ context, event }) => + event.type === 'SCOPE_CHANGED' && !scopesEqual(context.scope, event.scope), + queueHasWork: ({ context }) => context.queue.length > 0, + + // ── TAP_COLOR fork ── + tapInert: ({ event }) => event.type === 'TAP_COLOR' && event.verses.length === 0, + tapCanWrite: ({ context, event }) => + event.type === 'TAP_COLOR' && + event.verses.length > 0 && + context.isAuthenticated && + context.services.current.hasHighlightsPermission(), + tapNeedsSignIn: ({ context, event }) => + event.type === 'TAP_COLOR' && event.verses.length > 0 && !context.isAuthenticated, + + // ── resume fork ── + // Guards must be pure, so they PEEK (never clear expired/malformed entries); + // the clear-on-expiry/consume side effect stays in the actions/consume paths. + noPending: () => peekPendingHighlights().length === 0, + pendingNotAuthed: ({ context }) => + peekPendingHighlights().length > 0 && !context.isAuthenticated, + pendingAuthedHasPermission: ({ context }) => + peekPendingHighlights().length > 0 && + context.isAuthenticated && + context.services.current.hasHighlightsPermission(), + pendingAuthedNoPermission: ({ context }) => + peekPendingHighlights().length > 0 && + context.isAuthenticated && + !context.services.current.hasHighlightsPermission(), + }, + actions: { + setOutcomeNoop: assign({ lastTapOutcome: () => 'noop' as TapOutcome }), + + assignAuth: assign(({ event }) => { + if (event.type !== 'AUTH_CHANGED') return {}; + return { + flagOn: event.flagOn, + hasAuthProvider: event.hasAuthProvider, + isAuthenticated: event.isAuthenticated, + }; + }), + + /** Sign-out / going disabled must not leave a stale overlay to resurface later. */ + resetWriteStateIfSignedOut: assign(({ event }) => { + if (event.type !== 'AUTH_CHANGED' || event.isAuthenticated) return {}; + return { + overlay: {}, + reconcile: new Map(), + writeIntent: new Map(), + queue: [], + }; + }), + + resetForScopeChange: assign(({ event }) => { + if (event.type !== 'SCOPE_CHANGED') return {}; + // Verse numbers collide across scopes: drop the overlay + reconcile + // expectations synchronously, and clear writeIntent so a stale-scope write + // settling after this change can't pass its `writeIntent.get(verse) === + // token` ownership check and pollute the new scope's reconcile/overlay + // under a colliding verse number. In-flight writes carry their own scope + // and still refetch on settle; a resume-write re-paints via + // `applyPendingHighlight` (which sets its own intent) so nothing relies on + // the old intents surviving. This is also the escape hatch for a + // never-converging write — navigating away releases it. + return { + scope: event.scope, + overlay: {}, + reconcile: new Map(), + writeIntent: new Map(), + }; + }), + + /** + * Store the freshly fetched server truth and reconcile the overlay against + * it. Apply entries retire once the fetch reflects the written color; remove + * entries are HELD (the vapor fix — see the file header). + */ + reconcileOverlay: assign(({ context, event }) => { + if (event.type !== 'HIGHLIGHTS_UPDATED') return {}; + const serverColors = event.serverColors; + if (context.reconcile.size === 0) return { serverColors }; + + const overlay = { ...context.overlay }; + const reconcile = new Map(context.reconcile); + let changed = false; + for (const [verse, entry] of context.reconcile) { + if (entry.op !== 'apply') continue; // remove entries never retire (vapor fix) + if (serverColors[verse] === entry.color) { + reconcile.delete(verse); + if (verse in overlay) { + delete overlay[verse]; + changed = true; + } + } + } + return changed ? { serverColors, overlay, reconcile } : { serverColors, reconcile }; + }), + + consumeDataExchangeOnce: assign(({ context }) => { + if (context.dataExchangeConsumed) return {}; + const returned = context.services.current.consumeDataExchangeReturn(); + // Act on a decline/failure the moment the return is consumed — BEFORE the + // auth gate. The shipped auth provider hydrates asynchronously, so this + // runs unauthenticated on the first pass after a redirect return; a decline + // means the intent is dead regardless of who ends up signed in. + if (returned && returned.status !== 'granted') { + clearPendingHighlight(); + } + return { dataExchangeConsumed: true }; + }), + + /** Optimistically paint + claim + enqueue a user apply (TAP_COLOR authorized path). */ + startApplyWrite: enqueueActions(({ enqueue, context, event }) => { + if (event.type !== 'TAP_COLOR') return; + const color = event.color.toLowerCase(); + const verses = event.verses; + const token = {}; + enqueue.assign(({ context: current }) => ({ + ...claimVerses(current, verses, token, color), + lastTapOutcome: 'applied' as TapOutcome, + })); + const op: WriteOp = { + kind: 'apply', + color, + verses, + scope: context.scope, + token, + paint: true, + reprompt: true, + }; + enqueue.raise({ type: 'ENQUEUE', op }); + }), + + /** + * A tap that must enter the auth flow (signed out → sign-in dialog; signed in + * without permission → permission dialog): stash the intent so it survives a + * redirect round-trip / resumes after the grant, and report the `flow` + * outcome so the caller keeps the verse selection. + */ + stashPendingTap: enqueueActions(({ enqueue, context, event }) => { + if (event.type !== 'TAP_COLOR') return; + stashPendingHighlight({ + verses: event.verses, + color: event.color.toLowerCase(), + versionId: context.scope.versionId, + book: context.scope.book, + chapter: context.scope.chapter, + timestamp: Date.now(), + }); + enqueue.assign({ lastTapOutcome: () => 'flow' as TapOutcome }); + }), + + /** Remove: only clear verses currently rendered in the given color. */ + startRemoveWrite: enqueueActions(({ enqueue, context, event }) => { + if (event.type !== 'REMOVE') return; + if (!context.isAuthenticated) return; // nothing rendered to remove when signed out + const color = event.color.toLowerCase(); + const rendered = selectHighlightedVerses(context.serverColors, context.overlay); + const targetVerses = event.verses.filter((verse) => rendered[verse] === color); + if (targetVerses.length === 0) return; + + const token = {}; + enqueue.assign(({ context: current }) => claimVerses(current, targetVerses, token, null)); + const op: WriteOp = { + kind: 'remove', + color, + verses: targetVerses, + scope: context.scope, + token, + paint: true, + reprompt: false, + }; + enqueue.raise({ type: 'ENQUEUE', op }); + }), + + /** + * Apply every pending highlight after a granted return. Each entry writes to + * its OWN scope even if the user returned on a different chapter; an entry + * only paints the overlay when its scope matches what is on screen. Entries + * are enqueued first-to-last so verse-level ordering is deterministic; they + * never overlap on verses (append's last-wins merge guarantees it). + */ + applyPendingHighlight: enqueueActions(({ enqueue, context }) => { + const pendings = readPendingHighlights(); + if (pendings.length === 0) return; + clearPendingHighlight(); + for (const pending of pendings) { + const scope: HighlightScope = { + versionId: pending.versionId, + book: pending.book, + chapter: pending.chapter, + }; + const paint = scopesEqual(scope, context.scope); + const token = {}; + if (paint) { + enqueue.assign(({ context: current }) => + claimVerses(current, pending.verses, token, pending.color), + ); + } + const op: WriteOp = { + kind: 'apply', + color: pending.color, + verses: pending.verses, + scope, + token, + paint, + // A resume-write failure only logs + reverts (the pending intent was + // already consumed). Documented deferred follow-up: route resume-write + // failures through the standard apply failure handling. + reprompt: false, + }; + enqueue.raise({ type: 'ENQUEUE', op }); + } + }), + + enqueueOp: assign(({ context, event }) => { + if (event.type !== 'ENQUEUE') return {}; + return { queue: [...context.queue, event.op] }; + }), + + shiftQueue: assign(({ context }) => ({ queue: context.queue.slice(1) })), + + /** + * Per-sub-write settlement of a finished batch: succeeded verses register for + * reconciliation (overlay holds until a fetch reflects the write), failed + * verses revert (only if still owned by this op's token), exactly one refetch + * fires, and failures route by status (401/403 → invalidate + maybe re-prompt; + * network/5xx → discard pending). + */ + settleWrite: enqueueActions(({ enqueue, event }) => { + // Wired only to the processWrite `onDone`; the done event is the actor's + // `DoneActorEvent`, which is not part of the machine's public event union, + // so bridge through `unknown` to read its `output`. + const { op, failures, failedVerses, succeededVerses } = ( + event as unknown as DoneActorEvent + ).output; + + enqueue.assign(({ context: current }) => { + const overlay = { ...current.overlay }; + const reconcile = new Map(current.reconcile); + const writeIntent = new Map(current.writeIntent); + for (const verse of succeededVerses) { + if (current.writeIntent.get(verse) === op.token) { + reconcile.set(verse, { op: op.kind, color: op.color }); + // Settled writes release their claim so intents can't accumulate + // until sign-out. A newer op has already re-claimed the verse (its + // token differs), so the guard leaves that fresher claim intact. + writeIntent.delete(verse); + } + } + for (const verse of failedVerses) { + if (current.writeIntent.get(verse) === op.token) { + if (verse in overlay) delete overlay[verse]; + writeIntent.delete(verse); + } + } + return { overlay, reconcile, writeIntent }; + }); + + // Exactly one GET per settled batch, success or failure — this is what + // reconciles partial successes to server truth now that the hooks layer no + // longer auto-refetches per write. + enqueue(({ context: current }) => current.services.current.refetch()); + + if (failures.length === 0) return; + + enqueue(() => logWriteFailures(failures, op.color)); + + const permissionDenied = failures.some(isPermissionError); + if (permissionDenied) { + // Server says the permission is gone (or never applied): invalidate the + // optimistic cache. Server truth wins. + enqueue(({ context: current }) => + current.services.current.invalidateHighlightsPermission(), + ); + if (op.kind === 'apply' && op.reprompt) { + // Keep this highlight pending and re-prompt the just-in-time dialog. + // Append (not replace): a sibling batch that already lost permission may + // hold a different color/verses, and both intents must survive the grant. + enqueue(() => appendPendingHighlight(opToPending(op))); + enqueue.raise({ type: 'PERMISSION_LOST' }); + } + // Remove failures invalidate the cache but never re-prompt (the old + // deferred wart is fixed here): with no pending highlight, a re-prompt's + // post-grant resume was a no-op. The next apply re-enters the flow. + } + // Network / 5xx must NOT clear the stash: a user apply that reaches the + // queue via `startApplyWrite` never stashed anything of its own, so there + // is nothing here to drop — and any entries present belong to a sibling + // batch that lost permission moments earlier and must survive the grant. + // (Tap-flow stashes can't linger past this point either: they are followed + // by a full-page redirect, and dialog cancel/decline run `clearPending`.) + }), + + // ── Dialog side effects (fire-and-forget redirects, matching the hook) ── + startSignIn: ({ context }) => { + void context.services.current.startSignInForHighlights().catch((error: unknown) => { + console.error('[YouVersion SDK] Failed to start sign-in for highlights', error); + clearPendingHighlight(); + }); + }, + startDataExchange: ({ context }) => { + void context.services.current.startDataExchangeForHighlights().catch((error: unknown) => { + console.error('[YouVersion SDK] Failed to start data exchange for highlights', error); + clearPendingHighlight(); + }); + }, + clearPending: () => clearPendingHighlight(), + }, +}).createMachine({ + id: 'bibleReaderHighlights', + context: ({ input }) => ({ + services: input.services, + scope: input.scope, + flagOn: input.flagOn, + hasAuthProvider: input.hasAuthProvider, + isAuthenticated: input.isAuthenticated, + serverColors: {}, + overlay: {}, + reconcile: new Map(), + writeIntent: new Map(), + queue: [], + dataExchangeConsumed: false, + lastTapOutcome: 'noop', + }), + // These three inputs update context regardless of the active state; state + // moves are driven by `always` guards that read the freshly-assigned context. + on: { + AUTH_CHANGED: { actions: ['assignAuth', 'resetWriteStateIfSignedOut'] }, + HIGHLIGHTS_UPDATED: { actions: 'reconcileOverlay' }, + SCOPE_CHANGED: { guard: 'scopeIsDifferent', actions: 'resetForScopeChange' }, + }, + initial: 'booting', + states: { + booting: { + always: [{ guard: 'isEnabledNow', target: 'enabled' }, { target: 'disabled' }], + }, + + disabled: { + // Flag off or no auth provider: fully inert. Taps resolve to noop; removes + // are ignored. Copy/share (owned by the caller) are unaffected. + always: [{ guard: 'isEnabledNow', target: 'enabled' }], + on: { + TAP_COLOR: { actions: 'setOutcomeNoop' }, + REMOVE: {}, + }, + }, + + enabled: { + always: [{ guard: 'isDisabledNow', target: 'disabled' }], + type: 'parallel', + states: { + flow: { + initial: 'resuming', + on: { + // A write's 401/403 (raised by settleWrite) re-opens the JIT dialog. + PERMISSION_LOST: { target: '.permissionDialog' }, + // Taps are handled here so any flow substate resolves them. + TAP_COLOR: [ + { guard: 'tapInert', actions: 'setOutcomeNoop' }, + { guard: 'tapCanWrite', target: '.idle', actions: 'startApplyWrite' }, + { guard: 'tapNeedsSignIn', target: '.signInDialog', actions: 'stashPendingTap' }, + { target: '.permissionDialog', actions: 'stashPendingTap' }, + ], + REMOVE: { actions: 'startRemoveWrite' }, + }, + states: { + resuming: { + entry: 'consumeDataExchangeOnce', + always: [ + { guard: 'noPending', target: 'idle' }, + { guard: 'pendingNotAuthed', target: 'awaitingAuth' }, + { + guard: 'pendingAuthedHasPermission', + target: 'idle', + actions: 'applyPendingHighlight', + }, + { target: 'permissionDialog' }, + ], + }, + awaitingAuth: { + // Wait for the authenticated flip, then resolve the pending intent. + // No self-target: when still unauthenticated, none match and we stay. + always: [ + { guard: 'noPending', target: 'idle' }, + { + guard: 'pendingAuthedHasPermission', + target: 'idle', + actions: 'applyPendingHighlight', + }, + { guard: 'pendingAuthedNoPermission', target: 'permissionDialog' }, + ], + }, + idle: {}, + signInDialog: { + on: { + CONFIRM_SIGN_IN: { target: 'idle', actions: 'startSignIn' }, + DECLINE_SIGN_IN: { target: 'idle', actions: 'clearPending' }, + }, + }, + permissionDialog: { + on: { + CONFIRM_PERMISSION: { target: 'idle', actions: 'startDataExchange' }, + CANCEL_PERMISSION: { target: 'idle', actions: 'clearPending' }, + }, + }, + }, + }, + + writer: { + initial: 'idle', + on: { + // FIFO enqueue works in any writer state; `idle.always` starts it. + ENQUEUE: { actions: 'enqueueOp' }, + }, + states: { + idle: { + always: [{ guard: 'queueHasWork', target: 'writing' }], + }, + writing: { + invoke: { + src: 'processWrite', + input: ({ context }) => ({ services: context.services, op: context.queue[0]! }), + onDone: { target: 'checkQueue', actions: ['settleWrite', 'shiftQueue'] }, + // processWrite catches internally and never rejects, but keep the + // queue alive if it ever does. + onError: { target: 'checkQueue', actions: 'shiftQueue' }, + }, + }, + checkQueue: { + always: [{ guard: 'queueHasWork', target: 'writing' }, { target: 'idle' }], + }, + }, + }, + }, + }, + }, +}); diff --git a/packages/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index 23388aa2..80b5d43b 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -40,8 +40,12 @@ 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 { HighlightPermissionDialog } from './highlight-permission-dialog'; +import { SignInDialog } from './sign-in-dialog'; import { BibleTextView, getCleanVerseText, type FootnoteData } from './verse'; import { buildVerseReference, buildVerseShareText, joinVerseTexts } from '@/lib/verse-share'; +import { isHighlightsLive } from '@/lib/feature-flags'; +import { YouVersionPlatformConfiguration } from '@youversion/platform-core'; type BibleReaderContextType = { book: string; @@ -520,8 +524,23 @@ function Content() { highlightedVerses, apply: applyHighlight, remove: removeHighlight, + permissionDialogOpen, + onPermissionDialogOpenChange, + confirmPermissionDialog, + cancelPermissionDialog, + signInDialogOpen, + confirmSignInDialog, + cancelSignInDialog, } = useBibleReaderHighlights({ versionId, book, chapter }); + // The color row / clear-highlight affordances only render when the highlights + // feature is live (dark-launch flag). Copy / Share are always available. + const highlightsEnabled = isHighlightsLive(); + // Copy shown to the sign-in dialog. Falls back to a neutral label when the + // integrator hasn't set `YouVersionPlatformConfiguration.appName`. + const signInAppName = YouVersionPlatformConfiguration.appName ?? t('signInAppNameFallback'); + const signInPromptMessage = YouVersionPlatformConfiguration.signInPromptMessage; + // Navigating away (book/chapter/version) drops the selection — those verses no // longer exist on screen (ADR-007). useEffect(() => { @@ -571,7 +590,12 @@ function Content() { } function handleHighlight(color: string) { - applyHighlight(color, selectedVerses); + const outcome = applyHighlight(color, selectedVerses); + // Entering the auth flow (sign-in redirect or the permission confirm dialog) + // keeps the verse selection and popover intact so a cancel leaves the reader + // exactly where it was (YPE-1034 decision 7). An immediate apply — or an + // inert no-op — clears as before. + if (outcome === 'flow') return; closeAndClearSelection(); } @@ -735,6 +759,7 @@ function Content() { activeHighlights={activeHighlights} selectedVerses={selectedVerses} highlightedVerses={highlightedVerses} + highlightsEnabled={highlightsEnabled} anchorElement={anchorElement} scrollRoot={scrollContainerRef.current} onHighlight={handleHighlight} @@ -744,6 +769,26 @@ function Content() { theme={background} /> + + + { + if (!open) cancelSignInDialog(); + }} + appName={signInAppName} + promptMessage={signInPromptMessage} + onConfirm={confirmSignInDialog} + onDecline={cancelSignInDialog} + theme={background} + /> + {showLoadingOverlay ? (
void; + /** User accepted → start the data-exchange grant. */ + onConfirm: () => void; + /** User declined → discard the pending highlight. */ + onCancel: () => void; + theme?: 'light' | 'dark'; +}; + +/** + * Just-in-time permission confirm dialog for the highlight auth flow + * (YPE-1034). Copy is verbatim from the Swift SDK (`dataExchange.highlights.*`). + * Accepting starts the data-exchange grant; declining/dismissing discards only + * the pending highlight and leaves the verse selection intact. + */ +export const HighlightPermissionDialog: FC = ({ + open, + onOpenChange, + onConfirm, + onCancel, + theme = 'light', +}) => { + const { t } = useTranslation(undefined, { i18n }); + + return ( + + +
+ + {t('dataExchangeHighlightsQuestion')} + + + {t('dataExchangeHighlightsExplanation')} + +
+ +
+ + +
+ + + ); +}; diff --git a/packages/ui/src/components/sign-in-dialog.test.tsx b/packages/ui/src/components/sign-in-dialog.test.tsx new file mode 100644 index 00000000..984106a7 --- /dev/null +++ b/packages/ui/src/components/sign-in-dialog.test.tsx @@ -0,0 +1,112 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent } from '@testing-library/react'; +import { SignInDialog } from './sign-in-dialog'; + +describe('SignInDialog', () => { + const defaultProps = { + open: true, + onOpenChange: vi.fn(), + appName: 'Acme Bible', + onConfirm: vi.fn(), + onDecline: vi.fn(), + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('Rendering', () => { + it('renders the eyebrow, logo, body, and both buttons', () => { + render(); + + // Eyebrow caption + expect(screen.getByText('INTRODUCING')).toBeTruthy(); + // YouVersion Platform logo (svg with role img) + expect(screen.getByRole('img', { name: 'YouVersion Platform' })).toBeTruthy(); + // Body paragraph + expect( + screen.getByText(/wants to connect to your YouVersion Bible App account/), + ).toBeTruthy(); + // Primary + secondary buttons + expect(screen.getByRole('button', { name: 'Yes Please' })).toBeTruthy(); + expect(screen.getByRole('button', { name: 'No Thanks' })).toBeTruthy(); + }); + + it('does not render content when open is false', () => { + render(); + expect(screen.queryByRole('dialog')).toBeNull(); + }); + }); + + describe('Integrator prompt message', () => { + it('is hidden when promptMessage is unset', () => { + render(); + expect(screen.queryByText(/Loving this reading plan/)).toBeNull(); + }); + + it('is shown when promptMessage is provided', () => { + render(); + expect(screen.getByText(/Loving this reading plan\?/)).toBeTruthy(); + }); + }); + + describe('appName interpolation', () => { + it('interpolates the appName into the body paragraph', () => { + render(); + expect( + screen.getByText( + 'Acme Bible wants to connect to your YouVersion Bible App account. This will allow them to see and interact with your Bible App activity. Would you like to proceed?', + ), + ).toBeTruthy(); + }); + }); + + describe('Callbacks', () => { + it('calls onConfirm when "Yes Please" is clicked', () => { + const onConfirm = vi.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'Yes Please' })); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); + + it('calls onDecline when "No Thanks" is clicked', () => { + const onDecline = vi.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: 'No Thanks' })); + expect(onDecline).toHaveBeenCalledTimes(1); + }); + }); + + describe('Accessibility', () => { + it('renders a dialog labelled by the eyebrow title', () => { + render(); + const dialog = screen.getByRole('dialog'); + expect(dialog).toBeTruthy(); + // Radix wires aria-labelledby to the DialogTitle ("INTRODUCING"). + expect(dialog).toHaveAccessibleName('INTRODUCING'); + }); + + it('has a described-by body paragraph', () => { + render(); + const dialog = screen.getByRole('dialog'); + expect(dialog).toHaveAccessibleDescription(/wants to connect to your YouVersion Bible App/); + }); + }); + + describe('Styling', () => { + it('has the data-yv-sdk scoping attribute', () => { + render(); + expect(screen.getByRole('dialog').getAttribute('data-yv-sdk')).not.toBeNull(); + }); + + it('defaults to the light theme', () => { + render(); + expect(screen.getByRole('dialog').getAttribute('data-yv-theme')).toBe('light'); + }); + + it('applies the dark theme attribute', () => { + render(); + expect(screen.getByRole('dialog').getAttribute('data-yv-theme')).toBe('dark'); + }); + }); +}); diff --git a/packages/ui/src/components/sign-in-dialog.tsx b/packages/ui/src/components/sign-in-dialog.tsx new file mode 100644 index 00000000..e065d905 --- /dev/null +++ b/packages/ui/src/components/sign-in-dialog.tsx @@ -0,0 +1,82 @@ +import type { FC } from 'react'; +import { useTranslation } from 'react-i18next'; +import i18n from '@/i18n'; +import { YouVersionLogo } from './icons/youversion-logo'; +import { Button } from './ui/button'; +import { Dialog, DialogContent, DialogDescription, DialogTitle } from './ui/dialog'; + +type SignInDialogProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + /** The integrating app's display name, interpolated into the body copy. */ + appName: string; + /** + * Optional pitch line supplied by the integrator (from + * `YouVersionPlatformConfiguration`, passed in by the wiring layer). Hidden + * entirely when unset. + */ + promptMessage?: string; + /** User tapped "Yes Please" → launch the OAuth sign-in flow. */ + onConfirm: () => void; + /** User tapped "No Thanks" / dismissed → abandon the sign-in flow. */ + onDecline: () => void; + theme?: 'light' | 'dark'; +}; + +/** + * "Sign in with YouVersion" introduction dialog (YPE-1034). Shown when a + * signed-out user taps a highlight color, before OAuth launches. Copy is + * verbatim from the Swift SDK's `SignInWithYouVersionView` (`signIn.*`). + * Presentational only — accepting/declining is delegated to the callbacks; the + * component performs no OAuth, network, or config reads. + */ +export const SignInDialog: FC = ({ + open, + onOpenChange, + appName, + promptMessage, + onConfirm, + onDecline, + theme = 'light', +}) => { + const { t } = useTranslation(undefined, { i18n }); + + return ( + + +
+ + {t('signInIntroducing')} + + +
+ + {promptMessage ? ( +

+ “{promptMessage}” +

+ ) : null} + + + {t('signInParagraph', { appName })} + + +
+ + +
+ + + ); +}; diff --git a/packages/ui/src/components/ui/button.test.tsx b/packages/ui/src/components/ui/button.test.tsx new file mode 100644 index 00000000..a2e7e140 --- /dev/null +++ b/packages/ui/src/components/ui/button.test.tsx @@ -0,0 +1,21 @@ +/** + * @vitest-environment jsdom + */ +import { render } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; +import { Button } from './button'; + +describe('Button — default variant colors', () => { + it('paints the primary surface, not background-on-foreground (regression: invisible button)', () => { + // The `default` variant must pair `bg-primary` with `text-primary-foreground`. + // The previous `bg-background` + `text-primary-foreground` pairing resolved to + // white-on-white in the light theme, so the Continue button on the highlight + // permission dialog was invisible. Guard against regressing to that pairing. + const { getByRole } = render(); + const className = getByRole('button').className; + + expect(className).toContain('yv:bg-primary'); + expect(className).toContain('yv:text-primary-foreground'); + expect(className).not.toContain('yv:bg-background'); + }); +}); diff --git a/packages/ui/src/components/ui/button.tsx b/packages/ui/src/components/ui/button.tsx index 9f1b0a41..ddab8de7 100644 --- a/packages/ui/src/components/ui/button.tsx +++ b/packages/ui/src/components/ui/button.tsx @@ -9,7 +9,7 @@ const buttonVariants = cva( { variants: { variant: { - default: 'yv:bg-background yv:text-primary-foreground yv:hover:bg-background/90', + default: 'yv:bg-primary yv:text-primary-foreground yv:hover:bg-primary/90', destructive: 'yv:bg-destructive yv:text-white yv:hover:bg-destructive/90 yv:focus-visible:ring-destructive/20 yv:dark:focus-visible:ring-destructive/40 yv:dark:bg-destructive/60', outline: diff --git a/packages/ui/src/components/ui/dialog.tsx b/packages/ui/src/components/ui/dialog.tsx new file mode 100644 index 00000000..82c442bf --- /dev/null +++ b/packages/ui/src/components/ui/dialog.tsx @@ -0,0 +1,57 @@ +import * as React from 'react'; +import * as DialogPrimitive from '@radix-ui/react-dialog'; + +import { cn } from '../../lib/utils'; + +const Dialog = DialogPrimitive.Root; +const DialogTitle = DialogPrimitive.Title; +const DialogDescription = DialogPrimitive.Description; + +type DialogContentProps = React.ComponentProps & { + theme?: 'light' | 'dark'; +}; + +/** + * Shared modal chrome for the SDK's dialogs: renders the portal + overlay and a + * centered card `Content` with the SDK's `data-yv-sdk` scope + theme attributes. + * `className` slots between the card layout and the enter/exit animations so a + * caller's alignment classes land where they always did (byte-equivalent + * markup); everything else (`onEscapeKeyDown`, etc.) forwards to `Content`. + */ +function DialogContent({ + className, + theme = 'light', + children, + ...props +}: DialogContentProps): React.ReactElement { + return ( + + + + {children} + + + ); +} + +export { Dialog, DialogContent, DialogTitle, DialogDescription }; diff --git a/packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx new file mode 100644 index 00000000..af7e61a7 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx @@ -0,0 +1,421 @@ +/** + * @vitest-environment jsdom + * + * Integration coverage for the highlight auth flow state machine (YPE-1034 PR2) + * through the REAL `useHighlights` + `useHighlightAuthActions` hooks. Nothing + * from the hooks package is module-mocked; only the core clients' network + * methods (`HighlightsClient` / `DataExchangeClient` prototypes) and the sign-in + * redirect are stubbed at the boundary — the same discipline that caught PR 1's + * worst bug. + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import { + DataExchangeClient, + HighlightsClient, + YouVersionAPIUsers, + YouVersionPlatformConfiguration, + 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 { readPendingHighlights, stashPendingHighlight } from '@/lib/pending-highlight'; +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 options = { versionId: 111, book: 'JHN', chapter: '3' }; + +function setLocation(href: string) { + const url = new URL(href); + Object.defineProperty(window, 'location', { + value: { href: url.href, search: url.search, origin: url.origin, pathname: url.pathname }, + writable: true, + configurable: true, + }); +} + +function httpError(status: number): Error { + return Object.assign(new Error(`HTTP ${status}`), { status }); +} + +beforeEach(() => { + vi.restoreAllMocks(); + localStorage.clear(); + sessionStorage.clear(); + signedIn = false; + setHighlightsLive(true); + // The permission cache is user-scoped: persist a matching userInfo (as the + // auth provider does at sign-in) so granted permissions are readable. This + // alone grants nothing — the cache stays empty until a grant lands. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + setLocation('https://host.example/read'); + vi.spyOn(window.history, 'replaceState').mockImplementation(vi.fn()); + vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue({ + data: [], + next_page_token: null, + }); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); +}); + +describe('highlight auth flow — one-fell-swoop (signed out)', () => { + it('color tap opens the sign-in dialog and stashes pending; confirm starts sign-in; granted return applies', async () => { + const signIn = vi.spyOn(YouVersionAPIUsers, 'signIn').mockResolvedValue(undefined); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); + + const { result, unmount } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + // NEW BEHAVIOR (PR-288): a signed-out color tap opens the sign-in dialog + // instead of redirecting immediately. It stashes the pending intent and does + // NOT launch OAuth until the user confirms. + act(() => { + expect(result.current.apply('FFFE00', [16])).toBe('flow'); + }); + + expect(result.current.signInDialogOpen).toBe(true); + const pending = readPendingHighlights()[0]; + expect(pending).toMatchObject({ verses: [16], color: 'fffe00', versionId: 111, chapter: '3' }); + expect(signIn).not.toHaveBeenCalled(); + expect(createHighlight).not.toHaveBeenCalled(); + + // Confirm → launch the full-page sign-in redirect requesting `highlights`. + act(() => { + result.current.confirmSignInDialog(); + }); + expect(signIn).toHaveBeenCalledWith( + 'https://host.example/callback', + ['profile'], + ['highlights'], + ); + + // The confirm triggers a full-page redirect; simulate the reload on the + // granted return — a fresh mount, now signed in with the permission granted + // and the pending highlight still in sessionStorage. + unmount(); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + signedIn = true; + renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + await waitFor(() => { + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }); + }); + // Pending consumed (the write is the proof it applied). + expect(readPendingHighlights()).toEqual([]); + }); +}); + +describe('highlight auth flow — sign-in dialog (signed out)', () => { + it('a color tap opens the sign-in dialog and stashes pending without launching OAuth', () => { + const signIn = vi.spyOn(YouVersionAPIUsers, 'signIn').mockResolvedValue(undefined); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + expect(result.current.apply('fffe00', [16])).toBe('flow'); + }); + + expect(result.current.signInDialogOpen).toBe(true); + expect(readPendingHighlights()[0]).toMatchObject({ verses: [16], color: 'fffe00' }); + expect(signIn).not.toHaveBeenCalled(); + }); + + it('declining the sign-in dialog discards the pending highlight and does not sign in', () => { + const signIn = vi.spyOn(YouVersionAPIUsers, 'signIn').mockResolvedValue(undefined); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.signInDialogOpen).toBe(true); + expect(readPendingHighlights()).not.toHaveLength(0); + + act(() => { + result.current.cancelSignInDialog(); + }); + expect(result.current.signInDialogOpen).toBe(false); + expect(readPendingHighlights()).toEqual([]); + expect(signIn).not.toHaveBeenCalled(); + }); +}); + +describe('highlight auth flow — just-in-time (signed in, no permission)', () => { + beforeEach(() => { + signedIn = true; + }); + + it('opens the confirm dialog and stashes pending; confirm starts data exchange', async () => { + const updateToken = vi + .spyOn(DataExchangeClient.prototype, 'updateToken') + .mockResolvedValue('dx-token'); + const createHighlight = vi.spyOn(HighlightsClient.prototype, 'createHighlight'); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + expect(result.current.apply('fffe00', [16])).toBe('flow'); + }); + + expect(result.current.permissionDialogOpen).toBe(true); + expect(readPendingHighlights()[0]).toMatchObject({ verses: [16], color: 'fffe00' }); + expect(createHighlight).not.toHaveBeenCalled(); + + await act(async () => { + result.current.confirmPermissionDialog(); + await Promise.resolve(); + }); + + expect(updateToken).toHaveBeenCalledWith(['highlights']); + await waitFor(() => { + expect(window.location.href).toContain('https://api.example.com/data-exchange'); + }); + // Pending survives the redirect so the resume effect can apply it on return. + expect(readPendingHighlights()).not.toHaveLength(0); + }); + + it('declining the dialog discards only the pending highlight', () => { + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(readPendingHighlights()).not.toHaveLength(0); + + act(() => { + result.current.cancelPermissionDialog(); + }); + expect(result.current.permissionDialogOpen).toBe(false); + expect(readPendingHighlights()).toEqual([]); + }); +}); + +describe('highlight auth flow — data-exchange return', () => { + it('applies the pending highlight on a granted return (async session hydration)', async () => { + signedIn = false; + setLocation( + 'https://host.example/read?data_exchange_status=granted&granted_permissions=highlights', + ); + // Pre-stash a pending highlight as the confirm path would have. + stashPendingHighlight({ + verses: [16], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: Date.now(), + }); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); + + const { rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + // The session resolves after mount, as the shipped provider does. + signedIn = true; + rerender(); + + await waitFor(() => { + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }); + }); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(true); + expect(readPendingHighlights()).toEqual([]); + }); + + it('discards pending on a cancelled return and does not re-open the dialog (async session hydration)', async () => { + // Mount signed OUT: the shipped YouVersionAuthProvider hydrates userInfo + // asynchronously, so the first effect run after a redirect return is always + // unauthenticated. The cancel discard must survive that flip — consuming + // the status on run 1 and acting on it on run 2 is the bug this pins. + signedIn = false; + setLocation('https://host.example/read?data_exchange_status=cancel'); + stashPendingHighlight({ + verses: [16], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: Date.now(), + }); + const createHighlight = vi.spyOn(HighlightsClient.prototype, 'createHighlight'); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + // The session resolves after mount. + signedIn = true; + rerender(); + + await waitFor(() => { + expect(readPendingHighlights()).toEqual([]); + }); + expect(result.current.permissionDialogOpen).toBe(false); + expect(createHighlight).not.toHaveBeenCalled(); + }); + + it('discards pending on a failure return and does not re-open the dialog (async session hydration)', async () => { + signedIn = false; + setLocation('https://host.example/read?data_exchange_status=something-unexpected'); + stashPendingHighlight({ + verses: [16], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: Date.now(), + }); + const createHighlight = vi.spyOn(HighlightsClient.prototype, 'createHighlight'); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + signedIn = true; + rerender(); + + await waitFor(() => { + expect(readPendingHighlights()).toEqual([]); + }); + expect(result.current.permissionDialogOpen).toBe(false); + expect(createHighlight).not.toHaveBeenCalled(); + }); +}); + +describe('highlight auth flow — write failure routing', () => { + beforeEach(() => { + signedIn = true; + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + }); + + it('401 invalidates the permission cache, keeps pending, and re-prompts', async () => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockRejectedValue(httpError(401)); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + expect(result.current.apply('fffe00', [16])).toBe('applied'); + }); + + await waitFor(() => { + expect(result.current.permissionDialogOpen).toBe(true); + }); + // Cache invalidated (server truth wins), overlay reverted, pending KEPT. + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + expect(result.current.highlightedVerses).toEqual({}); + expect(readPendingHighlights()[0]).toMatchObject({ verses: [16], color: 'fffe00' }); + }); + + it('5xx reverts the overlay, logs, and discards pending without re-prompting', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(vi.fn()); + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockRejectedValue(httpError(500)); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({}); + }); + expect(result.current.permissionDialogOpen).toBe(false); + expect(readPendingHighlights()).toEqual([]); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(true); + expect(consoleError).toHaveBeenCalledWith( + expect.stringContaining('Failed to apply highlight'), + expect.anything(), + ); + }); +}); + +describe('highlight auth flow — operation queue', () => { + beforeEach(() => { + signedIn = true; + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + }); + + it('serializes overlapping apply→remove so DELETE never overtakes the in-flight POST', async () => { + const order: string[] = []; + let releaseCreate!: () => void; + const createGate = new Promise((resolve) => { + releaseCreate = resolve; + }); + + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockImplementation(async () => { + order.push('create:start'); + await createGate; + order.push('create:end'); + return { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }; + }); + vi.spyOn(HighlightsClient.prototype, 'deleteHighlight').mockImplementation(async () => { + order.push('delete:start'); + await Promise.resolve(); + }); + + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + // Apply then immediately remove the same verse. + act(() => { + result.current.apply('fffe00', [16]); + }); + act(() => { + result.current.remove('fffe00', [16]); + }); + + // Optimistic: last-issued (remove) wins the visual state. + expect(result.current.highlightedVerses).toEqual({}); + + // The DELETE must not start until the POST has fully settled. + await waitFor(() => expect(order).toContain('create:start')); + expect(order).not.toContain('delete:start'); + + releaseCreate(); + + await waitFor(() => expect(order).toContain('delete:start')); + expect(order).toEqual(['create:start', 'create:end', 'delete:start']); + // Settles to the last-issued operation's state. + expect(result.current.highlightedVerses).toEqual({}); + }); +}); 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 index e14a7c7c..c17acb9b 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.integration.test.tsx @@ -9,13 +9,23 @@ * mount never fetched highlights at all. */ import { act, renderHook, waitFor } from '@testing-library/react'; -import { HighlightsClient, type YouVersionUserInfo } from '@youversion/platform-core'; +import { + HighlightsClient, + YouVersionPlatformConfiguration, + type Collection, + type Highlight, + 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'; +function collection(data: Highlight[]): Collection { + return { data, next_page_token: null }; +} + const mockUserInfo = { id: 'user-1', name: 'Test User' } as unknown as YouVersionUserInfo; let signedIn = false; @@ -48,6 +58,8 @@ beforeEach(() => { afterEach(() => { vi.restoreAllMocks(); setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); }); describe('useBibleReaderHighlights — real useHighlights/useApiData', () => { @@ -105,3 +117,331 @@ describe('useBibleReaderHighlights — real useHighlights/useApiData', () => { expect(getHighlights).toHaveBeenCalledTimes(1); }); }); + +/** + * Write-path coverage through the REAL `useHighlights` + `useApiData` chain + * (only `HighlightsClient.prototype` network methods are spied). Auth is + * mounted signed OUT and flipped, matching the async session hydration of the + * shipped provider — never a synchronously-signed-in mount. + */ +describe('useBibleReaderHighlights — write reconciliation (Fix 2/3/4)', () => { + function mountFlipped() { + localStorage.clear(); + // The permission cache is user-scoped: persist a matching userInfo so the + // seeded grant is readable (the auth provider does this at sign-in). + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + const view = renderHook(() => useBibleReaderHighlights(defaultOptions), { wrapper: Providers }); + signedIn = true; + view.rerender(); + return view; + } + + it('apply: overlay wins when the post-write GET does not yet reflect the write (lag)', async () => { + // Mount GET and the single post-write refetch both come back empty — the + // server hasn't caught up to our POST yet (read-after-write lag). + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValue(collection([])); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); + + const { result } = mountFlipped(); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(1)); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + await waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(1)); + // Exactly one coalesced refetch after the write. + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + // The GET is still empty, but the overlay wins — the highlight stays painted. + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); + + it('apply: once a GET reflects the write, the overlay retires and server truth renders', async () => { + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValueOnce(collection([])) + .mockResolvedValue( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); + + const { result } = mountFlipped(); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(1)); + + act(() => { + result.current.apply('fffe00', [16]); + }); + await waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + + // The refetch reflects the write; the verse renders (now from server truth). + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); + + // Distinguish retired-overlay from still-masking-overlay (both render + // fffe00 above): the server now reports verse 16 in a DIFFERENT color, and + // a write to another verse triggers the next fetch. If verse 16's entry + // were still pending, its overlay would keep masking with fffe00; retired, + // the server's color must render. + getHighlights.mockResolvedValue( + collection([ + { version_id: 111, passage_id: 'JHN.3.16', color: '00d6ff' }, + { version_id: 111, passage_id: 'JHN.3.20', color: 'fffe00' }, + ]), + ); + act(() => { + result.current.apply('fffe00', [20]); + }); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(3)); + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: '00d6ff', 20: 'fffe00' }); + }); + }); + + it('apply partial failure: succeeded run stays painted, failed run reverts, one refetch (regression)', async () => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + // Post-write GET reflects the run that succeeded server-side ([2,3]). + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValueOnce(collection([])) + .mockResolvedValue( + collection([ + { version_id: 111, passage_id: 'JHN.3.2', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.3', color: 'fffe00' }, + ]), + ); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockImplementation((data) => { + if (data.passage_id === 'JHN.3.5') return Promise.reject(new Error('network down')); + return Promise.resolve({ version_id: 111, passage_id: data.passage_id, color: data.color }); + }); + + const { result } = mountFlipped(); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(1)); + + act(() => { + result.current.apply('fffe00', [2, 3, 5]); + }); + // Optimistic: all three painted. + expect(result.current.highlightedVerses).toEqual({ 2: 'fffe00', 3: 'fffe00', 5: 'fffe00' }); + + await waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(2)); + // The refetch still fires despite the failure — exactly one for the batch. + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + + // Succeeded run [2,3] stays painted (reconciled to server truth); failed + // run [5] reverted. Before this fix the WHOLE batch reverted with no + // refetch, erasing the persisted 2-3 until the next navigation/write. + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 2: 'fffe00', 3: 'fffe00' }); + }); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(2); + }); + + it('remove partial failure: no ghosts — succeeded DELETEs stay un-painted, failed one reverts (regression)', async () => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + // Server starts with [2,3,4] green; the post-batch GET is a stale snapshot + // that still contains all three (read-after-write lag on the deletes). + const getHighlights = vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue( + collection([ + { version_id: 111, passage_id: 'JHN.3.2', color: '5dff79' }, + { version_id: 111, passage_id: 'JHN.3.3', color: '5dff79' }, + { version_id: 111, passage_id: 'JHN.3.4', color: '5dff79' }, + ]), + ); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockImplementation((passageId) => { + if (passageId === 'JHN.3.3') return Promise.reject(new Error('network down')); + return Promise.resolve(undefined); + }); + + const { result } = mountFlipped(); + await waitFor(() => + expect(result.current.highlightedVerses).toEqual({ + 2: '5dff79', + 3: '5dff79', + 4: '5dff79', + }), + ); + + act(() => { + result.current.remove('5dff79', [2, 3, 4]); + }); + expect(result.current.highlightedVerses).toEqual({}); + + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(3)); + // One refetch despite the failure. + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + + // Verses 2 and 4 were deleted server-side: their remove overlay holds + // against the stale snapshot (no ghosts). Verse 3's DELETE failed: it + // reverts and renders highlighted again from the fetched data. + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 3: '5dff79' }); + }); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(2); + }); + + it('apply total failure: everything reverts and the batch still refetches exactly once', async () => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValue(collection([])); + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockRejectedValue( + new Error('network down'), + ); + + const { result } = mountFlipped(); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(1)); + + act(() => { + result.current.apply('fffe00', [16, 17]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00', 17: 'fffe00' }); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({}); + }); + // The batch refetch fires on the all-fail path too — and only once. + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(2); + }); + + it('remove: overlay wins when the post-delete GET still contains the removed row (lag)', async () => { + // Server starts with the row and, due to lag, still returns it after DELETE. + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValue( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockResolvedValue(undefined); + + const { result } = mountFlipped(); + await waitFor(() => expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' })); + + act(() => { + result.current.remove('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({}); + + await waitFor(() => + expect(deleteHighlight).toHaveBeenCalledWith('JHN.3.16', { version_id: 111 }), + ); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + // The GET still contains the row, but the removal overlay wins — no resurrection. + expect(result.current.highlightedVerses).toEqual({}); + }); + + it('apply: a genuine write failure reverts the optimistic overlay', async () => { + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue(collection([])); + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockRejectedValue( + new Error('network down'), + ); + + const { result } = mountFlipped(); + await waitFor(() => { + // wait for the mount fetch to settle + expect(result.current.highlightedVerses).toEqual({}); + }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({}); + }); + }); + + it('apply of [2,3,5] POSTs two runs but issues exactly one refetch (Fix 3)', async () => { + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValue(collection([])); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.2', color: 'fffe00' }); + + const { result } = mountFlipped(); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(1)); + + act(() => { + result.current.apply('fffe00', [2, 3, 5]); + }); + + await waitFor(() => expect(createHighlight).toHaveBeenCalledTimes(2)); + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.2-3', + color: 'fffe00', + }); + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.5', + color: 'fffe00', + }); + + // One refetch for the whole batch → 2 GETs total (mount + 1). + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(2); + }); + + it('remove of a contiguous [2,3] issues per-verse DELETEs and one refetch (Fix 4 + 3)', async () => { + const getHighlights = vi.spyOn(HighlightsClient.prototype, 'getHighlights').mockResolvedValue( + collection([ + { version_id: 111, passage_id: 'JHN.3.2', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.3', color: 'fffe00' }, + ]), + ); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockResolvedValue(undefined); + + const { result } = mountFlipped(); + await waitFor(() => + expect(result.current.highlightedVerses).toEqual({ 2: 'fffe00', 3: 'fffe00' }), + ); + + act(() => { + result.current.remove('fffe00', [2, 3]); + }); + + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(2)); + // Per-verse passage ids, NOT the range `JHN.3.2-3`. + expect(deleteHighlight).toHaveBeenCalledWith('JHN.3.2', { version_id: 111 }); + expect(deleteHighlight).toHaveBeenCalledWith('JHN.3.3', { version_id: 111 }); + + // One coalesced refetch for the whole removal → 2 GETs total (mount + 1). + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + await act(async () => { + await Promise.resolve(); + }); + expect(getHighlights).toHaveBeenCalledTimes(2); + }); +}); 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 a2e914f1..644bef83 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -4,7 +4,10 @@ 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 { + YouVersionPlatformConfiguration, + 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'; @@ -68,8 +71,16 @@ const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; beforeEach(() => { vi.clearAllMocks(); + localStorage.clear(); + sessionStorage.clear(); signedIn = true; setHighlightsLive(true); + // The permission cache is user-scoped, so it only takes effect once a matching + // userInfo is persisted (the auth provider does this at sign-in). Seed both so + // the authorized-write path is exercised. The auth-flow branches (missing + // session/permission) have their own dedicated coverage. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); }); afterEach(() => { @@ -201,6 +212,8 @@ describe('useBibleReaderHighlights — apply', () => { passage_id: 'JHN.3.20', color: 'fffe00', }); + // Two POSTs (two runs) but a SINGLE coalesced refetch for the batch (Fix 3). + expect(mocked.refetch).toHaveBeenCalledTimes(1); }); it('reverts the optimistic overlay and logs when the write fails', async () => { @@ -230,8 +243,8 @@ describe('useBibleReaderHighlights — apply', () => { }); }); -describe('useBibleReaderHighlights — overlay confirmation', () => { - it('drops the optimistic entry once the post-write refetch lands, so server truth wins', async () => { +describe('useBibleReaderHighlights — overlay reconciliation (Fix 2)', () => { + it('holds the overlay until a fetch REFLECTS the write, then retires it to server truth', async () => { const mocked = mockUseHighlights(); const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { @@ -251,23 +264,111 @@ describe('useBibleReaderHighlights — overlay confirmation', () => { 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). + // The post-write GET lands but does NOT yet contain the write (read-after- + // write lag). The overlay must WIN — no flicker out and back. + mockUseHighlights({ highlights: makeCollection([]) }); + rerender(); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + + // A later GET reflects the write (verse present in the written color). + mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + }); + rerender(); + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); + + // Prove the overlay entry was retired: with the entry gone, server truth now + // drives the verse, so clearing it server-side un-paints it. + mockUseHighlights({ highlights: makeCollection([]) }); + rerender(); + await waitFor(() => { + expect(result.current.highlightedVerses).toEqual({}); + }); + }); + + it('holds the overlay when the server converges on a DIFFERENT color (overlay wins until navigation)', async () => { + const mocked = mockUseHighlights(); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + await waitFor(() => { + expect(mocked.createHighlight).toHaveBeenCalledTimes(1); + }); + await act(async () => { + await Promise.resolve(); + }); + + // A GET returns the verse in a color that is NOT what we wrote. That doesn't + // reflect our write, so the overlay is held rather than snapping to it. mockUseHighlights({ highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: '00d6ff' }]), }); rerender(); + await act(async () => { + await Promise.resolve(); + }); + expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' }); + }); +}); + +describe('useBibleReaderHighlights — vapor bug (removed highlight resurrection)', () => { + // Regression for the staging "vapor" report: a deleted highlight reappears for + // a split second, then disappears. Root cause: the reconcile step retired a + // REMOVE overlay entry as soon as any fetch reflected the removal; a later + // response from a stale read replica that still contained the highlight then + // had nothing suppressing it, so the verse repainted until the next fetch. + it('a stale fetch after a settled+reflected DELETE does not resurrect the removed highlight', async () => { + const mocked = 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' }); + + act(() => { + result.current.remove('fffe00', [16]); + }); + // Optimistic removal. + expect(result.current.highlightedVerses).toEqual({}); - // Without confirmation-clearing, the stale overlay entry would keep - // rendering fffe00 until navigation. await waitFor(() => { - expect(result.current.highlightedVerses).toEqual({ 16: '00d6ff' }); + expect(mocked.deleteHighlight).toHaveBeenCalledTimes(1); }); + await act(async () => { + await Promise.resolve(); + }); + + // Fetch A reflects the removal (server no longer shows the color). + mockUseHighlights({ highlights: makeCollection([]) }); + rerender(); + expect(result.current.highlightedVerses).toEqual({}); + + // Fetch B is a STALE read replica that still contains the removed highlight. + // The removal overlay must be HELD so the highlight does not resurrect. + mockUseHighlights({ + highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + }); + rerender(); + expect(result.current.highlightedVerses).toEqual({}); + + // Fetch C is consistent again (still removed) — no flicker at any point. + mockUseHighlights({ highlights: makeCollection([]) }); + rerender(); + expect(result.current.highlightedVerses).toEqual({}); }); }); describe('useBibleReaderHighlights — remove', () => { - it('removes optimistically and DELETEs only verses rendered in that color, as ranges', async () => { + it('removes optimistically and DELETEs one passage-id per verse (never a range)', async () => { const mocked = mockUseHighlights({ highlights: makeCollection([ { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, @@ -287,10 +388,15 @@ describe('useBibleReaderHighlights — remove', () => { // Optimistic: yellow verses gone immediately, green untouched. expect(result.current.highlightedVerses).toEqual({ 18: '5dff79' }); + // Contiguous [16,17] must NOT collapse to `JHN.3.16-17` — range delete is + // unsupported server-side (Fix 4). One DELETE per verse instead. await waitFor(() => { - expect(mocked.deleteHighlight).toHaveBeenCalledTimes(1); + expect(mocked.deleteHighlight).toHaveBeenCalledTimes(2); }); - expect(mocked.deleteHighlight).toHaveBeenCalledWith('JHN.3.16-17', { version_id: 111 }); + expect(mocked.deleteHighlight).toHaveBeenCalledWith('JHN.3.16', { version_id: 111 }); + expect(mocked.deleteHighlight).toHaveBeenCalledWith('JHN.3.17', { version_id: 111 }); + // The whole removal coalesces into a single refetch (Fix 3). + expect(mocked.refetch).toHaveBeenCalledTimes(1); }); it('reverts the optimistic removal and logs when the delete fails', async () => { diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 76617795..18219fa7 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -1,10 +1,21 @@ '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'; +import { + bibleReaderHighlightsMachine, + scopesEqual, + selectHighlightedVerses, + type HighlightScope, + type HighlightServices, + type ServerColors, +} from './bible-reader-highlights-machine'; +import { + useHighlightAuthActions, + useHighlights, + YouVersionAuthContext, +} from '@youversion/platform-react-hooks'; +import { useActorRef, useSelector } from '@xstate/react'; +import { useContext, useEffect, useMemo, useRef } from 'react'; export type UseBibleReaderHighlightsOptions = { versionId: number; @@ -15,295 +26,225 @@ export type UseBibleReaderHighlightsOptions = { 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; + /** + * Highlights the given verses in `color`. Bridge-safe: primitives only. + * When the user has a session and the highlights permission this writes + * optimistically (`'applied'`); otherwise it opens the sign-in dialog (signed + * out) or the just-in-time permission dialog (signed in, no permission) and + * returns `'flow'`. Returns `'noop'` when highlighting is inert (flag off, no + * verses, or no auth provider). The caller uses the outcome to decide whether + * to keep the verse selection: `'flow'` keeps it so cancelling the dialog + * leaves the selection and popover intact. + */ + apply: (color: string, verses: number[]) => 'applied' | 'flow' | 'noop'; /** Clears the given verses that are currently highlighted in `color`. */ remove: (color: string, verses: number[]) => void; + /** Whether the just-in-time permission confirm dialog is open. */ + permissionDialogOpen: boolean; + /** Controlled open-change for the permission confirm dialog. */ + onPermissionDialogOpenChange: (open: boolean) => void; + /** User accepted the dialog → start the data-exchange grant (full-page redirect). */ + confirmPermissionDialog: () => void; + /** User declined/dismissed the dialog → discard the pending highlight. */ + cancelPermissionDialog: () => void; + /** Whether the sign-in introduction dialog is open (signed-out color tap). */ + signInDialogOpen: boolean; + /** User accepted the sign-in dialog → start the sign-in redirect. */ + confirmSignInDialog: () => void; + /** User declined/dismissed the sign-in dialog → discard the pending highlight. */ + cancelSignInDialog: () => void; }; /** - * Optimistic per-verse overlay on top of the fetched highlights: a hex color - * means "optimistically applied", `null` means "optimistically removed". + * Parses the fetched highlights into a verse→color map for the current scope, + * exactly as `selectHighlightedVerses` expects the server side. */ -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 parseServerColors( + highlights: { data?: { version_id: number; passage_id: string; color: string }[] } | null, + versionId: number, + chapterUsfm: string, +): ServerColors { + const map: ServerColors = {}; + const versePrefix = `${chapterUsfm}.`; + 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(); } + return map; } -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; +function serverColorsEqual(a: ServerColors, b: ServerColors): boolean { + const aKeys = Object.keys(a); + if (aKeys.length !== Object.keys(b).length) return false; + for (const key of aKeys) { + if (a[Number(key)] !== b[Number(key)]) return false; } - return snapshot; + return true; } /** - * 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). + * BibleReader's seam onto the highlights API (YPE-1034, self-contained mode). A + * THIN adapter over `bibleReaderHighlightsMachine` (PR-288): it reads auth + + * flag + fetched highlights from React and feeds them to the machine as events, + * exposes the machine's dialog states + write commands, and derives the rendered + * verse map. All flow/write logic (optimistic overlay, serialized writes, + * per-verse ownership, reconcile, auth flow, the vapor fix) lives in the + * machine; see that file for the invariants and the statechart. * - * 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. + * Rendering and fetching are gated on `isHighlightsLive() && isAuthenticated`. + * With no auth provider the reader keeps the PR-1 posture: no fetch, no writes, + * and a color tap never enters the auth flow — copy/share still work. */ 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. + // Read the auth context directly instead of `useYVAuth`, which throws when no + // auth provider is mounted. const authContext = useContext(YouVersionAuthContext); + const hasAuthProvider = authContext !== null; const isAuthenticated = Boolean(authContext?.userInfo); - const live = isHighlightsLive() && isAuthenticated; + const flagOn = isHighlightsLive(); + const live = flagOn && isAuthenticated; + + const { + hasHighlightsPermission, + invalidateHighlightsPermission, + consumeDataExchangeReturn, + startSignInForHighlights, + startDataExchangeForHighlights, + } = useHighlightAuthActions(); const chapterUsfm = `${book}.${chapter}`; - const { highlights, createHighlight, deleteHighlight } = useHighlights( + const { highlights, createHighlight, deleteHighlight, refetch } = 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]); + // A stable ref bag of the live SDK service closures. Passed once to the machine + // via `input`; the machine reads `.current` at call time so it always sees the + // latest closures without re-spawning. The ref is initialized with the first + // render's services and then kept current each render (the "latest ref" + // pattern) so freshness holds without a null-hole or re-spawn. + const services: HighlightServices = { + createHighlight, + deleteHighlight, + refetch, + hasHighlightsPermission, + invalidateHighlightsPermission, + consumeDataExchangeReturn, + startSignInForHighlights, + startDataExchangeForHighlights, + }; + const servicesRef = useRef(services); + servicesRef.current = services; + + const scope: HighlightScope = useMemo( + () => ({ versionId, book, chapter }), + [versionId, book, chapter], + ); - // 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 actorRef = useActorRef(bibleReaderHighlightsMachine, { + input: { + services: servicesRef, + scope, + flagOn, + hasAuthProvider, + isAuthenticated, + }, + }); - // When the post-write refetch lands, drain the confirmed verses' overlay - // entries so the server's truth wins again (see `confirmedVersesRef` above). + // ── Feed React-owned inputs to the machine ────────────────────────────────── 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]); + actorRef.send({ type: 'AUTH_CHANGED', flagOn, hasAuthProvider, isAuthenticated }); + }, [actorRef, flagOn, hasAuthProvider, isAuthenticated]); - 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], + useEffect(() => { + actorRef.send({ type: 'SCOPE_CHANGED', scope }); + }, [actorRef, scope]); + + // Parse the fetch into server truth and forward it whenever it changes. The + // machine reconciles the optimistic overlay against it. + const serverColors = useMemo( + () => parseServerColors(highlights, versionId, chapterUsfm), + [highlights, versionId, chapterUsfm], ); + const lastSentServerColorsRef = useRef(null); + useEffect(() => { + if ( + lastSentServerColorsRef.current !== null && + serverColorsEqual(lastSentServerColorsRef.current, serverColors) + ) { + return; + } + lastSentServerColorsRef.current = serverColors; + actorRef.send({ type: 'HIGHLIGHTS_UPDATED', serverColors }); + }, [actorRef, serverColors]); - 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), - }), - ), - ); + // ── Rendered verse map ────────────────────────────────────────────────────── + const overlay = useSelector(actorRef, (state) => state.context.overlay); + const machineScope = useSelector(actorRef, (state) => state.context.scope); + const highlightedVerses = useMemo(() => { + // Gate on `live`: sign-out or flag-off must render nothing this very render, + // including optimistic overlay entries still in the machine. + if (!live) return {}; + // Only apply the overlay when the machine's scope matches the current one. + // On a synchronous scope change (before the SCOPE_CHANGED effect runs) the + // machine scope still points at the old chapter, so the overlay is skipped + // and the new chapter renders from server truth alone — verse numbers + // collide across chapters. + if (!scopesEqual(machineScope, scope)) return { ...serverColors }; + return selectHighlightedVerses(serverColors, overlay); + }, [live, serverColors, overlay, machineScope, scope]); + + // ── Dialog state + commands ───────────────────────────────────────────────── + const signInDialogOpen = useSelector(actorRef, (state) => + state.matches({ enabled: { flow: 'signInDialog' } }), + ); + const permissionDialogOpen = useSelector(actorRef, (state) => + state.matches({ enabled: { flow: 'permissionDialog' } }), + ); - 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], + const api = useMemo< + Pick< + UseBibleReaderHighlightsReturn, + | 'apply' + | 'remove' + | 'onPermissionDialogOpenChange' + | 'confirmPermissionDialog' + | 'cancelPermissionDialog' + | 'confirmSignInDialog' + | 'cancelSignInDialog' + > + >( + () => ({ + apply: (color, verses) => { + actorRef.send({ type: 'TAP_COLOR', color, verses }); + return actorRef.getSnapshot().context.lastTapOutcome; + }, + remove: (color, verses) => { + actorRef.send({ type: 'REMOVE', color, verses }); + }, + onPermissionDialogOpenChange: (open) => { + // Dismiss via outside-click / Escape is a decline: discard the pending + // highlight but leave the verse selection untouched (the caller owns it). + if (!open) actorRef.send({ type: 'CANCEL_PERMISSION' }); + }, + confirmPermissionDialog: () => actorRef.send({ type: 'CONFIRM_PERMISSION' }), + cancelPermissionDialog: () => actorRef.send({ type: 'CANCEL_PERMISSION' }), + confirmSignInDialog: () => actorRef.send({ type: 'CONFIRM_SIGN_IN' }), + cancelSignInDialog: () => actorRef.send({ type: 'DECLINE_SIGN_IN' }), + }), + [actorRef], ); - return { highlightedVerses, apply, remove }; + return { + highlightedVerses, + permissionDialogOpen, + signInDialogOpen, + ...api, + }; } diff --git a/packages/ui/src/components/verse-action-popover.test.tsx b/packages/ui/src/components/verse-action-popover.test.tsx index 074b8902..db04c396 100644 --- a/packages/ui/src/components/verse-action-popover.test.tsx +++ b/packages/ui/src/components/verse-action-popover.test.tsx @@ -467,4 +467,39 @@ describe('VerseActionPopover', () => { expect(applyButtons).toHaveLength(5); }); }); + + function applyButtons() { + return screen + .getAllByRole('button') + .filter((btn) => btn.getAttribute('aria-label')?.includes('Apply')); + } + + function clearButtons() { + return screen + .getAllByRole('button') + .filter((btn) => btn.getAttribute('aria-label')?.includes('Clear')); + } + + describe('Highlights disabled (flag off)', () => { + it('hides the color row and remove circles but keeps Copy / Share', () => { + render( + ([HIGHLIGHT_COLORS[0]])} + selectedVerses={[1]} + highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} + />, + ); + + // No color group, no apply circles, no remove circles. + expect(screen.queryByRole('group', { name: 'Highlight colors' })).toBeNull(); + expect(applyButtons()).toHaveLength(0); + expect(clearButtons()).toHaveLength(0); + + // Copy / Share remain. + expect(screen.getByText('Copy')).toBeTruthy(); + expect(screen.getByText('Share')).toBeTruthy(); + }); + }); }); diff --git a/packages/ui/src/components/verse-action-popover.tsx b/packages/ui/src/components/verse-action-popover.tsx index ab6fe181..e8a30764 100644 --- a/packages/ui/src/components/verse-action-popover.tsx +++ b/packages/ui/src/components/verse-action-popover.tsx @@ -11,9 +11,9 @@ type Measurable = { getBoundingClientRect: () => DOMRect }; /** * Highlight colors, as 6-digit lowercase hex (no `#`) so they map 1:1 onto the - * future API `highlight.color` field (/^[0-9a-f]{6}$/). Order is the canonical - * apply order: yellow, green, blue, orange, pink. Hardcoded to match the - * YouVersion iOS app exactly. + * API `highlight.color` field (/^[0-9a-f]{6}$/). Order is the canonical apply + * order: yellow, green, blue, orange, pink. Hardcoded to match the YouVersion + * iOS app exactly. */ export const HIGHLIGHT_COLORS = ['fffe00', '5dff79', '00d6ff', 'ffc66f', 'ff95ef'] as const; @@ -32,6 +32,12 @@ type VerseActionPopoverProps = { * reachable instead of leaving with the verse. Omit for a purely anchored bar. */ scrollRoot?: HTMLElement | null; + /** + * Whether the highlights UI is available. When `false` (the `HIGHLIGHTS_LIVE` + * dark-launch flag is off) the color row and the remove (checkmark) circles are + * hidden entirely — only Copy / Share remain. Defaults to `true`. + */ + highlightsEnabled?: boolean; onHighlight: (color: string) => void; onClearHighlight: (color: string) => void; onCopy: () => void; @@ -103,6 +109,7 @@ export const VerseActionPopover: FC = ({ highlightedVerses, anchorElement, scrollRoot, + highlightsEnabled = true, onHighlight, onClearHighlight, onCopy, @@ -275,24 +282,30 @@ export const VerseActionPopover: FC = ({ )} -
- {view.colorCircles.map(({ color, showX, key }) => ( - (showX ? onClearHighlight(color) : onHighlight(color))} - /> - ))} -
+ {/* Highlights UI is hidden entirely when the feature is off (flag off): + only Copy / Share remain. */} + {highlightsEnabled && ( + <> +
+ {view.colorCircles.map(({ color, showX, key }) => ( + (showX ? onClearHighlight(color) : onHighlight(color))} + /> + ))} +
- {/* Separator */} -