From f455b7b8932c653eb62b8c726e4cb82e196f06ba Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 14:35:55 -0500 Subject: [PATCH 01/10] feat(highlights): highlight auth flow state machine (YPE-1034 PR2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn a color tap without a session or the `highlights` permission into an applied highlight across the two auth paths, behind the internal HIGHLIGHTS_LIVE flag. Core: DataExchangeClient (POST /data-exchange/token, Zod-validated), the hosted-grant URL builder + callback parser/handler, and an optimistic permission cache on YouVersionPlatformConfiguration seeded from `granted_permissions` on the sign-in and data-exchange callbacks (server 401/403 invalidates it). SignInWithYouVersionResult gains `permissions`. Hooks: useHighlightAuthActions exposes one-fell-swoop sign-in (requesting `highlights`), the just-in-time data-exchange redirect, permission reads/invalidation, and the data-exchange return handler. UI: useBibleReaderHighlights runs the state machine — pending highlights persist to sessionStorage (~10-min expiry) across the redirect round-trip and apply on a granted return; a permission confirm dialog (copy matched to the native SDK, en/fr/es) gates the grant. Write failures route by status (401/403 re-prompts and keeps pending; 5xx/network reverts and discards). Apply/remove writes are serialized through a FIFO queue with per-verse ownership, closing the two concurrency windows PR1 documented. Copy/share-only behavior with no auth provider is unchanged. Co-Authored-By: Claude Fable 5 --- .changeset/highlight-auth-flow.md | 11 + .../core/src/SignInWithYouVersionResult.ts | 9 + packages/core/src/Users.ts | 12 + .../src/YouVersionPlatformConfiguration.ts | 56 ++ .../core/src/__tests__/data-exchange.test.ts | 108 ++++ .../core/src/__tests__/permissions.test.ts | 73 +++ packages/core/src/data-exchange.ts | 140 +++++ packages/core/src/index.ts | 9 + packages/core/src/permissions.ts | 27 + packages/hooks/src/index.ts | 1 + .../src/useHighlightAuthActions.test.tsx | 83 +++ packages/hooks/src/useHighlightAuthActions.ts | 109 ++++ packages/ui/src/components/bible-reader.tsx | 20 +- .../highlight-permission-dialog.tsx | 79 +++ ...-highlights.auth-flow.integration.test.tsx | 334 +++++++++++ .../use-bible-reader-highlights.test.tsx | 13 +- .../components/use-bible-reader-highlights.ts | 530 +++++++++++++----- packages/ui/src/lib/pending-highlight.test.ts | 66 +++ packages/ui/src/lib/pending-highlight.ts | 108 ++++ 19 files changed, 1642 insertions(+), 146 deletions(-) create mode 100644 .changeset/highlight-auth-flow.md create mode 100644 packages/core/src/__tests__/data-exchange.test.ts create mode 100644 packages/core/src/__tests__/permissions.test.ts create mode 100644 packages/core/src/data-exchange.ts create mode 100644 packages/core/src/permissions.ts create mode 100644 packages/hooks/src/useHighlightAuthActions.test.tsx create mode 100644 packages/hooks/src/useHighlightAuthActions.ts create mode 100644 packages/ui/src/components/highlight-permission-dialog.tsx create mode 100644 packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx create mode 100644 packages/ui/src/lib/pending-highlight.test.ts create mode 100644 packages/ui/src/lib/pending-highlight.ts diff --git a/.changeset/highlight-auth-flow.md b/.changeset/highlight-auth-flow.md new file mode 100644 index 00000000..df82feec --- /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). `SignInWithYouVersionResult` gains a `permissions` field. +- **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/packages/core/src/SignInWithYouVersionResult.ts b/packages/core/src/SignInWithYouVersionResult.ts index 83383c32..fb550b4e 100644 --- a/packages/core/src/SignInWithYouVersionResult.ts +++ b/packages/core/src/SignInWithYouVersionResult.ts @@ -14,6 +14,7 @@ type SignInWithYouVersionResultProps = { name?: string; profilePicture?: string; email?: string; + permissions?: string[]; }; export class SignInWithYouVersionResult { public readonly accessToken: string | undefined; @@ -23,6 +24,12 @@ export class SignInWithYouVersionResult { public readonly name: string | undefined; public readonly profilePicture: string | undefined; public readonly email: string | undefined; + /** + * Data-exchange permissions the server reported as granted for this sign-in + * (parsed from `granted_permissions` on the callback). Empty when the callback + * carried none. Additive: existing consumers can ignore it. + */ + public permissions: string[]; constructor({ accessToken, @@ -32,6 +39,7 @@ export class SignInWithYouVersionResult { name, profilePicture, email, + permissions, }: SignInWithYouVersionResultProps) { this.accessToken = accessToken; this.expiryDate = expiresIn ? new Date(Date.now() + expiresIn * 1000) : new Date(); @@ -40,5 +48,6 @@ export class SignInWithYouVersionResult { this.name = name; this.profilePicture = profilePicture; this.email = email; + this.permissions = permissions ?? []; } } diff --git a/packages/core/src/Users.ts b/packages/core/src/Users.ts index cf1678c3..5fdadcd8 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 { /** @@ -124,6 +125,17 @@ export class YouVersionAPIUsers { // Extract user info from ID token const result = this.extractSignInResult(tokens); + // Surface + persist 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). This seeds 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. + const grantedPermissions = parseGrantedPermissions(urlParams); + result.permissions = grantedPermissions; + if (grantedPermissions.length > 0) { + YouVersionPlatformConfiguration.saveGrantedPermissions(grantedPermissions); + } + // Store tokens in configuration. The ID token is intentionally not // persisted — it is only used here to derive the user profile below. YouVersionPlatformConfiguration.saveAuthData( diff --git a/packages/core/src/YouVersionPlatformConfiguration.ts b/packages/core/src/YouVersionPlatformConfiguration.ts index 1e46f94f..25a35ff9 100644 --- a/packages/core/src/YouVersionPlatformConfiguration.ts +++ b/packages/core/src/YouVersionPlatformConfiguration.ts @@ -73,6 +73,62 @@ 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}. Stored as a JSON string array. + */ + private static readonly grantedPermissionsKey = 'youversion-platform:granted-permissions'; + + public static get grantedPermissions(): string[] { + if (typeof localStorage === 'undefined') return []; + const raw = localStorage.getItem(this.grantedPermissionsKey); + if (!raw) return []; + try { + const parsed: unknown = JSON.parse(raw); + if (!Array.isArray(parsed)) return []; + return parsed.filter((entry): entry is string => typeof entry === 'string'); + } catch { + return []; + } + } + + /** Merges `permissions` into the cache (union), preserving existing entries. */ + public static saveGrantedPermissions(permissions: string[]): void { + if (typeof localStorage === 'undefined') return; + const merged = new Set([...this.grantedPermissions, ...permissions]); + localStorage.setItem(this.grantedPermissionsKey, JSON.stringify([...merged])); + } + + /** + * Replaces the cache with exactly `permissions` (used to reconcile the cache + * with the authoritative set the server returns on a data-exchange grant). + */ + public static setGrantedPermissions(permissions: string[]): void { + if (typeof localStorage === 'undefined') return; + localStorage.setItem(this.grantedPermissionsKey, JSON.stringify([...new Set(permissions)])); + } + + /** 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 next = this.grantedPermissions.filter((entry) => entry !== permission); + localStorage.setItem(this.grantedPermissionsKey, JSON.stringify(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 { 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..3cb42e2f --- /dev/null +++ b/packages/core/src/__tests__/data-exchange.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { http, HttpResponse } from 'msw'; +import { ApiClient } from '../client'; +import { + DataExchangeClient, + buildDataExchangeUrl, + parseDataExchangeCallback, +} 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: [], + }); + }); +}); diff --git a/packages/core/src/__tests__/permissions.test.ts b/packages/core/src/__tests__/permissions.test.ts new file mode 100644 index 00000000..e39b0f32 --- /dev/null +++ b/packages/core/src/__tests__/permissions.test.ts @@ -0,0 +1,73 @@ +/** + * @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', () => { + beforeEach(() => { + localStorage.clear(); + }); + + 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('setGrantedPermissions overwrites the cache (reconcile)', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights', 'votd']); + YouVersionPlatformConfiguration.setGrantedPermissions(['highlights']); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual(['highlights']); + }); + + 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('youversion-platform:granted-permissions', '{not json'); + 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..2ebd2e8b --- /dev/null +++ b/packages/core/src/data-exchange.ts @@ -0,0 +1,140 @@ +import { z } from 'zod'; +import type { ApiClient } from './client'; +import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; +import { parseGrantedPermissions } from './permissions'; + +/** + * 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. + */ + +const DataExchangeTokenResponseSchema = z.object({ + token: z.string().min(1), +}); + +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].sort() }, + { '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(); +} + +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. + */ +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); + cleanUrl.search = ''; + window.history.replaceState({}, '', cleanUrl.toString()); + + return result; +} 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..bc77c918 --- /dev/null +++ b/packages/core/src/permissions.ts @@ -0,0 +1,27 @@ +/** + * 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]+/)) { + const trimmed = part.trim(); + if (trimmed) seen.add(trimmed); + } + } + return [...seen]; +} 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..16d6b721 --- /dev/null +++ b/packages/hooks/src/useHighlightAuthActions.test.tsx @@ -0,0 +1,83 @@ +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', () => { + 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..f5208a44 --- /dev/null +++ b/packages/hooks/src/useHighlightAuthActions.ts @@ -0,0 +1,109 @@ +'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; + +/** + * 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 hasHighlightsPermission = useCallback( + () => YouVersionPlatformConfiguration.hasPermission(HIGHLIGHTS_PERMISSION), + [], + ); + + const invalidateHighlightsPermission = useCallback( + () => YouVersionPlatformConfiguration.removeGrantedPermission(HIGHLIGHTS_PERMISSION), + [], + ); + + const consumeDataExchangeReturn = useCallback(() => handleDataExchangeCallback(), []); + + 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/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index 23388aa2..31678dda 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -40,6 +40,7 @@ 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 { BibleTextView, getCleanVerseText, type FootnoteData } from './verse'; import { buildVerseReference, buildVerseShareText, joinVerseTexts } from '@/lib/verse-share'; @@ -520,6 +521,10 @@ function Content() { highlightedVerses, apply: applyHighlight, remove: removeHighlight, + permissionDialogOpen, + onPermissionDialogOpenChange, + confirmPermissionDialog, + cancelPermissionDialog, } = useBibleReaderHighlights({ versionId, book, chapter }); // Navigating away (book/chapter/version) drops the selection — those verses no @@ -571,7 +576,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(); } @@ -744,6 +754,14 @@ function Content() { 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('highlightPermissionTitle')} + + + {t('highlightPermissionBody')} + +
+ +
+ + +
+ + + + ); +}; 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..f08a68aa --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx @@ -0,0 +1,334 @@ +/** + * @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 { readPendingHighlight } 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); + 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 stashes pending, starts sign-in with highlights, then applies on granted return', 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, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + // Signed out color tap → auth flow, not a write. + act(() => { + expect(result.current.apply('FFFE00', [16])).toBe('flow'); + }); + + const pending = readPendingHighlight(); + expect(pending).toMatchObject({ verses: [16], color: 'fffe00', versionId: 111, chapter: '3' }); + expect(signIn).toHaveBeenCalledWith( + 'https://host.example/callback', + ['profile'], + ['highlights'], + ); + expect(createHighlight).not.toHaveBeenCalled(); + + // Simulate the granted return: handleAuthCallback would have persisted the + // granted permission; the session then resolves authenticated. + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + signedIn = true; + rerender(); + + await waitFor(() => { + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }); + }); + // Pending consumed (the write is the proof it applied; the post-write + // refetch here returns empty server truth, so the optimistic overlay clears). + expect(readPendingHighlight()).toBeNull(); + }); +}); + +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(readPendingHighlight()).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(readPendingHighlight()).not.toBeNull(); + }); + + it('declining the dialog discards only the pending highlight', () => { + const { result } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(readPendingHighlight()).not.toBeNull(); + + act(() => { + result.current.cancelPermissionDialog(); + }); + expect(result.current.permissionDialogOpen).toBe(false); + expect(readPendingHighlight()).toBeNull(); + }); +}); + +describe('highlight auth flow — data-exchange return', () => { + it('applies the pending highlight on a granted return', async () => { + signedIn = true; + setLocation( + 'https://host.example/read?data_exchange_status=granted&granted_permissions=highlights', + ); + // Pre-stash a pending highlight as the confirm path would have. + sessionStorage.setItem( + 'youversion-platform:pending-highlight', + JSON.stringify({ + 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' }); + + renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + await waitFor(() => { + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }); + }); + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(true); + expect(readPendingHighlight()).toBeNull(); + }); + + it('discards the pending highlight on a cancelled return', async () => { + signedIn = true; + setLocation('https://host.example/read?data_exchange_status=cancel'); + sessionStorage.setItem( + 'youversion-platform:pending-highlight', + JSON.stringify({ + verses: [16], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: Date.now(), + }), + ); + const createHighlight = vi.spyOn(HighlightsClient.prototype, 'createHighlight'); + + renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + + await waitFor(() => { + expect(readPendingHighlight()).toBeNull(); + }); + 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(readPendingHighlight()).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(readPendingHighlight()).toBeNull(); + 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.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.test.tsx index a2e914f1..aad2ce4f 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,12 +71,20 @@ const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; beforeEach(() => { vi.clearAllMocks(); + localStorage.clear(); + sessionStorage.clear(); signedIn = true; setHighlightsLive(true); + // These tests exercise the authorized-write path, so seed the optimistic + // permission cache. The auth-flow branches (missing session/permission) have + // their own dedicated coverage. + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); }); afterEach(() => { setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); }); describe('useBibleReaderHighlights — flag off (dark launch)', () => { diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 76617795..7d1bf3ec 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -1,8 +1,17 @@ 'use client'; import { isHighlightsLive } from '@/lib/feature-flags'; +import { + clearPendingHighlight, + readPendingHighlight, + stashPendingHighlight, +} from '@/lib/pending-highlight'; import { buildPassageIds } from '@/lib/usfm-ranges'; -import { useHighlights, YouVersionAuthContext } from '@youversion/platform-react-hooks'; +import { + useHighlightAuthActions, + useHighlights, + YouVersionAuthContext, +} from '@youversion/platform-react-hooks'; import { Result } from 'better-result'; import { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; @@ -15,10 +24,27 @@ 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 stashes a pending highlight and + * enters the highlight auth flow (`'flow'` — sign-in redirect or the + * permission confirm dialog). 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; }; /** @@ -46,12 +72,20 @@ class BibleReaderHighlightError extends Error { } } -function snapshotOverlay(overlay: HighlightOverlay, verses: number[]): HighlightOverlay { - const snapshot: HighlightOverlay = {}; - for (const verse of verses) { - if (verse in overlay) snapshot[verse] = overlay[verse] as string | null; +/** 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 snapshot; + return undefined; +} + +/** A 401/403 means the app lost (or never had) the highlights permission. */ +function isPermissionError(error: unknown): boolean { + const status = extractStatus(error); + return status === 401 || status === 403; } /** @@ -62,11 +96,17 @@ function snapshotOverlay(overlay: HighlightOverlay, verses: number[]): Highlight * in-memory optimistic overlay. There is no local persistence (ADR-001 in * docs/adr/YPE-1034-highlights-server-only.md). * - * Everything is gated on `isHighlightsLive() && isAuthenticated`: while the - * dark-launch flag is off, or the user has no session, the hook is inert — no - * fetches, no writes, an empty rendered map. Failures in this PR get a - * `console.error` and an overlay revert only; toasts and 401/403 handling are - * PR 2. + * Rendering and fetching are gated on `isHighlightsLive() && isAuthenticated`. + * Writing additionally requires the `highlights` permission; when it (or the + * session) is missing, a color tap stashes a pending highlight and enters the + * highlight auth flow instead — one-fell-swoop sign-in when signed out, or the + * just-in-time permission confirm dialog → data-exchange grant when signed in. + * + * Apply/remove writes are serialized through a single FIFO promise chain so a + * later operation can never race an earlier one to the server (e.g. a DELETE + * overtaking an in-flight POST for the same verse). A per-verse ownership token + * guarantees a failed write only reverts verses no newer operation has claimed, + * so overlapping writes settle to the last-issued operation's state. */ export function useBibleReaderHighlights({ versionId, @@ -74,11 +114,22 @@ export function useBibleReaderHighlights({ 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. + // the consumer never mounted an auth provider. With no provider we keep the + // PR 1 posture: no fetch, no writes, and a color tap never enters the auth + // flow (there is no auth to run) — copy/share still work. 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( @@ -87,38 +138,16 @@ export function useBibleReaderHighlights({ ); const [overlay, setOverlay] = useState({}); + const [permissionDialogOpen, setPermissionDialogOpen] = useState(false); - // 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. + // Drop the optimistic overlay synchronously (during render) the moment the + // scope changes, so an in-flight overlay never paints over another + // chapter's or version's verses — their verse numbers collide. const overlayScope = `${versionId}:${chapterUsfm}`; - const overlayScopeRef = useRef(overlayScope); - overlayScopeRef.current = overlayScope; const [loadedOverlayScope, setLoadedOverlayScope] = useState(overlayScope); if (loadedOverlayScope !== overlayScope) { setLoadedOverlayScope(overlayScope); setOverlay({}); - confirmedVersesRef.current = new Set(); } const highlightedVerses = useMemo(() => { @@ -144,15 +173,20 @@ export function useBibleReaderHighlights({ return map; }, [live, highlights, overlay, chapterUsfm, versionId]); - // Refs so `apply` / `remove` can snapshot current state without re-memoizing - // on every overlay/fetch change. + // Refs so callbacks can snapshot current state without re-memoizing on every + // overlay/fetch change. const overlayRef = useRef(overlay); overlayRef.current = overlay; const highlightedVersesRef = useRef(highlightedVerses); highlightedVersesRef.current = highlightedVerses; - // When the post-write refetch lands, drain the confirmed verses' overlay - // entries so the server's truth wins again (see `confirmedVersesRef` above). + // Verses whose write settled successfully and are awaiting the post-write + // refetch. Once fresh data lands, their overlay entries are dropped so the + // server's truth wins again — otherwise a successful write's overlay entry + // would mask every later server-side change to that verse (another device, + // another tab) until navigation. + const confirmedVersesRef = useRef>(new Set()); + useEffect(() => { if (confirmedVersesRef.current.size === 0) return; const confirmed = confirmedVersesRef.current; @@ -178,132 +212,340 @@ export function useBibleReaderHighlights({ }); }, []); - const revertOverlay = useCallback((verses: number[], snapshot: HighlightOverlay) => { + // Per-verse ownership: each write claims its verses with a fresh token. A + // failed write only reverts (drops the optimistic entry, letting server truth + // show) verses it still owns — if a newer write re-claimed a verse, the newer + // write owns its final state. This closes the "loser clobbers winner" window. + const writeIntentRef = useRef>(new Map()); + + const claimVerses = useCallback((verses: number[]): object => { + const token = {}; + for (const verse of verses) writeIntentRef.current.set(verse, token); + return token; + }, []); + + const revertOwned = useCallback((verses: number[], token: object) => { setOverlay((current) => { + let changed = false; const next = { ...current }; for (const verse of verses) { - if (verse in snapshot) next[verse] = snapshot[verse] as string | null; - else delete next[verse]; + if (writeIntentRef.current.get(verse) === token && verse in next) { + delete next[verse]; + changed = true; + } } - return next; + return changed ? next : current; }); }, []); - // 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; + // Single FIFO promise chain serializing every apply/remove write. `.then` on + // both fulfil and reject keeps the chain alive past a failed operation. + const writeQueueRef = useRef>(Promise.resolve()); + const enqueueWrite = useCallback((task: () => Promise) => { + const run = writeQueueRef.current.then(task, task); + writeQueueRef.current = run; + return run; + }, []); - 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), - }), - ), + // Stable refs for values the queued tasks and the resume effect read, so those + // callbacks don't need to re-create on every render. + const scopeRef = useRef({ versionId, book, chapter }); + scopeRef.current = { versionId, book, chapter }; + const createHighlightRef = useRef(createHighlight); + createHighlightRef.current = createHighlight; + const authActionsRef = useRef({ + invalidateHighlightsPermission, + startDataExchangeForHighlights, + }); + authActionsRef.current = { invalidateHighlightsPermission, startDataExchangeForHighlights }; + + const logWriteFailures = useCallback( + (failures: BibleReaderHighlightError[], color: string) => { + for (const failure of failures) { + console.error( + `[YouVersion SDK] Failed to ${failure.operation} highlight (version ${versionId}, ` + + `passage ${failure.passageId}, color ${color})`, + failure, ); + } + }, + [versionId], + ); - 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; + const runApply = useCallback( + async (color: string, verses: number[], token: object) => { + const results = await Promise.all( + buildPassageIds(book, chapter, verses).map((passageId) => + Result.tryPromise({ + try: () => + createHighlightRef.current({ version_id: versionId, passage_id: passageId, color }), + catch: (cause) => new BibleReaderHighlightError('apply', passageId, cause), + }), + ), + ); + + const failures = results.filter(Result.isError).map((r) => r.error); + if (failures.length === 0) { + for (const verse of verses) { + if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); } - for (const failure of failures) { - console.error( - `[YouVersion SDK] Failed to apply highlight (version ${versionId}, ` + - `passage ${failure.error.passageId}, color ${normalizedColor})`, - failure.error, - ); + return; + } + + logWriteFailures(failures, color); + revertOwned(verses, token); + + if (failures.some(isPermissionError)) { + // Server says the permission is gone (or never applied): invalidate the + // optimistic cache, keep this highlight as pending, and re-prompt. + authActionsRef.current.invalidateHighlightsPermission(); + const scope = scopeRef.current; + stashPendingHighlight({ + verses, + color, + versionId: scope.versionId, + book: scope.book, + chapter: scope.chapter, + timestamp: Date.now(), + }); + setPermissionDialogOpen(true); + } else { + // Network / 5xx: overlay already reverted; drop any pending intent. + clearPendingHighlight(); + } + }, + [book, chapter, versionId, logWriteFailures, revertOwned], + ); + + const runRemove = useCallback( + async (color: string, verses: number[], token: object) => { + const results = await Promise.all( + buildPassageIds(book, chapter, verses).map((passageId) => + Result.tryPromise({ + try: () => deleteHighlight(passageId, { version_id: versionId }), + catch: (cause) => new BibleReaderHighlightError('remove', passageId, cause), + }), + ), + ); + + const failures = results.filter(Result.isError).map((r) => r.error); + if (failures.length === 0) { + for (const verse of verses) { + if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); } - // 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); - })(); + return; + } + + logWriteFailures(failures, color); + revertOwned(verses, token); + + if (failures.some(isPermissionError)) { + authActionsRef.current.invalidateHighlightsPermission(); + setPermissionDialogOpen(true); + } + }, + [book, chapter, versionId, deleteHighlight, logWriteFailures, revertOwned], + ); + + const apply = useCallback( + (color: string, verses: number[]): 'applied' | 'flow' | 'noop' => { + if (!flagOn || verses.length === 0 || !hasAuthProvider) return 'noop'; + + const normalizedColor = color.toLowerCase(); + + // Authorized write: optimistic paint now, serialized POST behind the queue. + if (isAuthenticated && hasHighlightsPermission()) { + const token = claimVerses(verses); + patchOverlay(verses, normalizedColor); + void enqueueWrite(() => runApply(normalizedColor, verses, token)); + return 'applied'; + } + + // Enter the highlight auth flow: stash the intent so it survives a redirect + // round-trip (sign-in) or resumes after the confirm dialog's grant. + stashPendingHighlight({ + verses, + color: normalizedColor, + versionId, + book, + chapter, + timestamp: Date.now(), + }); + + if (!isAuthenticated) { + // One-fell-swoop: full-page redirect to sign-in requesting `highlights`. + void startSignInForHighlights().catch((error) => { + console.error('[YouVersion SDK] Failed to start sign-in for highlights', error); + clearPendingHighlight(); + }); + return 'flow'; + } + + // Signed in, permission missing → just-in-time confirm dialog. + setPermissionDialogOpen(true); + return 'flow'; }, - [live, book, chapter, versionId, createHighlight, patchOverlay, revertOverlay], + [ + flagOn, + hasAuthProvider, + isAuthenticated, + hasHighlightsPermission, + claimVerses, + patchOverlay, + enqueueWrite, + runApply, + versionId, + book, + chapter, + startSignInForHighlights, + ], ); const remove = useCallback( (color: string, verses: number[]) => { + // Removal only ever touches highlights already on screen, so it needs no + // auth-flow branch: without a live authenticated session there is nothing + // rendered to remove. 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.) + // 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); + const token = claimVerses(targetVerses); patchOverlay(targetVerses, null); + void enqueueWrite(() => runRemove(normalizedColor, targetVerses, token)); + }, + [live, claimVerses, patchOverlay, enqueueWrite, runRemove], + ); - 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), - }), - ), - ); + // ---- Resume after an auth round-trip -------------------------------------- + // Runs on mount and whenever the session flips authenticated. Consumes a + // data-exchange return once (reconciling the permission cache + cleaning the + // URL), then resolves any pending highlight: apply it when the permission is + // now granted, discard it on an explicit cancel/failure, or re-prompt when the + // user came back signed in but still without the permission. + const dataExchangeConsumedRef = useRef(false); + const consumeDataExchangeReturnRef = useRef(consumeDataExchangeReturn); + consumeDataExchangeReturnRef.current = consumeDataExchangeReturn; + const hasHighlightsPermissionRef = useRef(hasHighlightsPermission); + hasHighlightsPermissionRef.current = hasHighlightsPermission; + + useEffect(() => { + if (!flagOn || !hasAuthProvider) return; + + let dataExchangeStatus: 'granted' | 'cancel' | 'failure' | null = null; + if (!dataExchangeConsumedRef.current) { + dataExchangeConsumedRef.current = true; + dataExchangeStatus = consumeDataExchangeReturnRef.current()?.status ?? null; + } + + const pending = readPendingHighlight(); + if (!pending) return; + + // Sign-in is still resolving the session — wait for the authenticated flip. + if (!isAuthenticated) return; + + if (!hasHighlightsPermissionRef.current()) { + // Explicit cancel/failure from a data-exchange return → discard. + if (dataExchangeStatus === 'cancel' || dataExchangeStatus === 'failure') { + clearPendingHighlight(); + return; + } + // Signed in but the permission was not granted (e.g. one-fell-swoop + // sign-in that returned without it) → offer the data-exchange grant. + // NOTE: this replaces the state-machine's automatic sign-in→data-exchange + // hop with a confirm prompt, to avoid an unattended redirect loop on load. + setPermissionDialogOpen(true); + return; + } + + // Permission granted: apply the pending highlight. Write to its own scope so + // a return that lands on a different chapter still persists it; paint the + // optimistic overlay only when it matches what's on screen. + clearPendingHighlight(); + const scope = scopeRef.current; + const matchesCurrent = + pending.versionId === scope.versionId && + pending.book === scope.book && + pending.chapter === scope.chapter; - 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); + let token: object | null = null; + if (matchesCurrent) { + token = claimVerses(pending.verses); + patchOverlay(pending.verses, pending.color); + } + + void enqueueWrite(async () => { + const results = await Promise.all( + buildPassageIds(pending.book, pending.chapter, pending.verses).map((passageId) => + Result.tryPromise({ + try: () => + createHighlightRef.current({ + version_id: pending.versionId, + passage_id: passageId, + color: pending.color, + }), + catch: (cause) => new BibleReaderHighlightError('apply', passageId, cause), + }), + ), + ); + const failures = results.filter(Result.isError).map((r) => r.error); + if (failures.length === 0) { + if (token) { + for (const verse of pending.verses) { + if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); } - return; - } - for (const failure of failures) { - console.error( - `[YouVersion SDK] Failed to remove highlight (version ${versionId}, ` + - `passage ${failure.error.passageId}, color ${normalizedColor})`, - failure.error, - ); } - // Gate on scope like the success path (see `apply`). - if (overlayScopeRef.current === scopeAtWrite) revertOverlay(targetVerses, snapshot); - })(); - }, - [live, book, chapter, versionId, deleteHighlight, patchOverlay, revertOverlay], - ); + return; + } + logWriteFailures(failures, pending.color); + if (token) revertOwned(pending.verses, token); + }); + }, [ + flagOn, + hasAuthProvider, + isAuthenticated, + claimVerses, + patchOverlay, + enqueueWrite, + revertOwned, + logWriteFailures, + ]); + + const onPermissionDialogOpenChange = useCallback((open: boolean) => { + setPermissionDialogOpen(open); + // Dismissing via outside-click / Escape is a decline: discard the pending + // highlight but leave the verse selection untouched (the caller owns it). + if (!open) clearPendingHighlight(); + }, []); + + const confirmPermissionDialog = useCallback(() => { + setPermissionDialogOpen(false); + // The pending highlight is already stashed; the data-exchange redirect will + // round-trip and the resume effect applies it on return. + void startDataExchangeForHighlights().catch((error) => { + console.error('[YouVersion SDK] Failed to start data exchange for highlights', error); + clearPendingHighlight(); + }); + }, [startDataExchangeForHighlights]); + + const cancelPermissionDialog = useCallback(() => { + setPermissionDialogOpen(false); + clearPendingHighlight(); + }, []); - return { highlightedVerses, apply, remove }; + return { + highlightedVerses, + apply, + remove, + permissionDialogOpen, + onPermissionDialogOpenChange, + confirmPermissionDialog, + cancelPermissionDialog, + }; } diff --git a/packages/ui/src/lib/pending-highlight.test.ts b/packages/ui/src/lib/pending-highlight.test.ts new file mode 100644 index 00000000..776c7f7c --- /dev/null +++ b/packages/ui/src/lib/pending-highlight.test.ts @@ -0,0 +1,66 @@ +/** + * @vitest-environment jsdom + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + clearPendingHighlight, + PENDING_HIGHLIGHT_TTL_MS, + readPendingHighlight, + stashPendingHighlight, + type PendingHighlight, +} from './pending-highlight'; + +const base: PendingHighlight = { + verses: [16, 17], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: 1_000_000, +}; + +describe('pending-highlight', () => { + beforeEach(() => sessionStorage.clear()); + afterEach(() => sessionStorage.clear()); + + it('round-trips a stashed pending highlight', () => { + stashPendingHighlight(base); + expect(readPendingHighlight(base.timestamp)).toEqual(base); + }); + + it('returns null when nothing is stashed', () => { + expect(readPendingHighlight()).toBeNull(); + }); + + it('discards and clears an expired entry on read', () => { + stashPendingHighlight(base); + const expiredNow = base.timestamp + PENDING_HIGHLIGHT_TTL_MS + 1; + expect(readPendingHighlight(expiredNow)).toBeNull(); + // Cleared as a side effect: even reading at a fresh time finds nothing. + expect(readPendingHighlight(base.timestamp)).toBeNull(); + }); + + it('keeps an entry that is exactly at the TTL boundary', () => { + stashPendingHighlight(base); + expect(readPendingHighlight(base.timestamp + PENDING_HIGHLIGHT_TTL_MS)).toEqual(base); + }); + + it('discards malformed JSON', () => { + sessionStorage.setItem('youversion-platform:pending-highlight', '{broken'); + expect(readPendingHighlight()).toBeNull(); + }); + + it('discards a structurally-invalid entry', () => { + sessionStorage.setItem( + 'youversion-platform:pending-highlight', + JSON.stringify({ verses: 'nope', color: 1 }), + ); + expect(readPendingHighlight()).toBeNull(); + }); + + it('clearPendingHighlight removes the entry', () => { + stashPendingHighlight(base); + clearPendingHighlight(); + expect(readPendingHighlight(base.timestamp)).toBeNull(); + }); +}); diff --git a/packages/ui/src/lib/pending-highlight.ts b/packages/ui/src/lib/pending-highlight.ts new file mode 100644 index 00000000..5f5bcd25 --- /dev/null +++ b/packages/ui/src/lib/pending-highlight.ts @@ -0,0 +1,108 @@ +/** + * A user's stashed highlight intent while the highlight auth flow is in flight + * (YPE-1034). Persisted to `sessionStorage` so it survives the full-page OAuth / + * data-exchange redirect round-trip, and expires (~10 min) so an abandoned + * round-trip can never silently apply a highlight during a much later sign-in. + * + * This is NOT highlight data (highlights are server-only account data, ADR-001); + * it is a short-lived intent record, discarded on decline, cancel, failure, or + * successful apply. + */ +export type PendingHighlight = { + /** Verse numbers to highlight. */ + verses: number[]; + /** Highlight color, 6-char lowercase hex without `#`. */ + color: string; + /** Bible version id (a.k.a. `bible_id` at the API boundary). */ + versionId: number; + /** USFM book id (e.g. `JHN`). */ + book: string; + /** Chapter id (e.g. `3`). */ + chapter: string; + /** Epoch ms when stashed; drives expiry. */ + timestamp: number; +}; + +const STORAGE_KEY = 'youversion-platform:pending-highlight'; + +/** Pending highlights older than this on read are treated as expired. */ +export const PENDING_HIGHLIGHT_TTL_MS = 10 * 60 * 1000; + +function getSessionStorage(): Storage | null { + try { + return typeof sessionStorage === 'undefined' ? null : sessionStorage; + } catch { + // Access can throw in sandboxed/blocked-storage contexts. + return null; + } +} + +function isPendingHighlight(value: unknown): value is PendingHighlight { + if (typeof value !== 'object' || value === null) return false; + const candidate = value as Record; + return ( + Array.isArray(candidate.verses) && + candidate.verses.every((v) => typeof v === 'number') && + typeof candidate.color === 'string' && + typeof candidate.versionId === 'number' && + typeof candidate.book === 'string' && + typeof candidate.chapter === 'string' && + typeof candidate.timestamp === 'number' + ); +} + +/** Stashes the pending highlight, replacing any existing one. */ +export function stashPendingHighlight(pending: PendingHighlight): void { + const store = getSessionStorage(); + if (!store) return; + try { + store.setItem(STORAGE_KEY, JSON.stringify(pending)); + } catch { + // Quota / disabled storage — nothing to do; the flow simply won't resume. + } +} + +/** + * Reads the pending highlight, or `null` when there is none, it is malformed, or + * it has expired. Expired / malformed entries are cleared as a side effect so a + * stale intent can't linger. + * + * @param now Injectable clock for tests; defaults to `Date.now()`. + */ +export function readPendingHighlight(now: number = Date.now()): PendingHighlight | null { + const store = getSessionStorage(); + if (!store) return null; + const raw = store.getItem(STORAGE_KEY); + if (!raw) return null; + + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + clearPendingHighlight(); + return null; + } + + if (!isPendingHighlight(parsed)) { + clearPendingHighlight(); + return null; + } + + if (now - parsed.timestamp > PENDING_HIGHLIGHT_TTL_MS) { + clearPendingHighlight(); + return null; + } + + return parsed; +} + +/** Discards the pending highlight. Safe to call when there is none. */ +export function clearPendingHighlight(): void { + const store = getSessionStorage(); + if (!store) return; + try { + store.removeItem(STORAGE_KEY); + } catch { + // Ignore. + } +} From 25917440e9ec274dbae446903bd7cb409ea298a7 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Fri, 10 Jul 2026 14:46:52 -0500 Subject: [PATCH 02/10] fix(highlights): act on data-exchange cancel/failure before the session hydrates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resume effect consumed the data-exchange return status into a local, but bailed on `!isAuthenticated` first. The shipped YouVersionAuthProvider hydrates userInfo asynchronously, so the first effect run after a redirect return is always unauthenticated: the status was consumed and lost, and when the session flipped the cancel/failure branch was skipped — the pending highlight survived a decline and the just-declined dialog re-opened. Discard the pending highlight at consume time for any non-granted return, before the auth gate: it runs exactly once and needs no session (a decline kills the intent regardless of who signs in). Tests: the cancel-return test previously set signedIn=true synchronously — a timing the real provider never produces — which hid the bug. It now mounts signed out and flips the session after mount (failed against the old code, passes now), plus a failure-status variant with the same timing and the granted-return test aligned for realism. Also document the three review-deferred follow-ups (expired-token 401 misrouting, resume-write failure handling, remove-failure re-prompt) at their code sites and in the changeset. Co-Authored-By: Claude Fable 5 --- .changeset/highlight-auth-flow.md | 6 ++ ...-highlights.auth-flow.integration.test.tsx | 59 +++++++++++++++++-- .../components/use-bible-reader-highlights.ts | 38 +++++++++--- 3 files changed, 89 insertions(+), 14 deletions(-) diff --git a/.changeset/highlight-auth-flow.md b/.changeset/highlight-auth-flow.md index df82feec..c7ce574a 100644 --- a/.changeset/highlight-auth-flow.md +++ b/.changeset/highlight-auth-flow.md @@ -9,3 +9,9 @@ Add the highlight auth flow: a color tap in BibleReader without a session or the - **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). `SignInWithYouVersionResult` gains a `permissions` field. - **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. + +Deferred follow-ups (documented at each code site in `use-bible-reader-highlights.ts`, accepted for dark launch): + +1. A 401 from an expired token (not a missing permission) misroutes to the permission re-prompt; follow-up is distinguishing auth-expiry from permission-denied at the `isPermissionError` boundary. +2. A failure while applying a resumed pending highlight only logs + reverts (the pending intent was cleared before the write), so a 401 there loses the highlight instead of keep-pending + re-prompt; follow-up is routing resume-write failures through the standard apply failure handling. +3. A 401/403 on `remove` opens the permission dialog with no pending highlight, so the post-grant resume is a no-op; follow-up is not re-prompting on remove failures. 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 index f08a68aa..9c2d059e 100644 --- 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 @@ -176,8 +176,8 @@ describe('highlight auth flow — just-in-time (signed in, no permission)', () = }); describe('highlight auth flow — data-exchange return', () => { - it('applies the pending highlight on a granted return', async () => { - signedIn = true; + 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', ); @@ -197,7 +197,13 @@ describe('highlight auth flow — data-exchange return', () => { .spyOn(HighlightsClient.prototype, 'createHighlight') .mockResolvedValue({ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }); - renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + 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({ @@ -210,8 +216,12 @@ describe('highlight auth flow — data-exchange return', () => { expect(readPendingHighlight()).toBeNull(); }); - it('discards the pending highlight on a cancelled return', async () => { - signedIn = true; + 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'); sessionStorage.setItem( 'youversion-platform:pending-highlight', @@ -226,11 +236,48 @@ describe('highlight auth flow — data-exchange return', () => { ); const createHighlight = vi.spyOn(HighlightsClient.prototype, 'createHighlight'); - renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); + const { result, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + // The session resolves after mount. + signedIn = true; + rerender(); await waitFor(() => { expect(readPendingHighlight()).toBeNull(); }); + 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'); + sessionStorage.setItem( + 'youversion-platform:pending-highlight', + JSON.stringify({ + 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(readPendingHighlight()).toBeNull(); + }); + expect(result.current.permissionDialogOpen).toBe(false); expect(createHighlight).not.toHaveBeenCalled(); }); }); diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 7d1bf3ec..c5f74c11 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -82,7 +82,14 @@ function extractStatus(error: unknown): number | undefined { return undefined; } -/** A 401/403 means the app lost (or never had) the highlights permission. */ +/** + * 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; @@ -341,6 +348,10 @@ export function useBibleReaderHighlights({ if (failures.some(isPermissionError)) { authActionsRef.current.invalidateHighlightsPermission(); + // DEFERRED: a remove failure has no pending highlight, so the dialog's + // post-grant resume is a no-op — the user grants and nothing visibly + // happens. Follow-up: don't re-prompt on remove failures (invalidating + // the cache above is enough; the next apply re-enters the flow). setPermissionDialogOpen(true); } }, @@ -438,10 +449,19 @@ export function useBibleReaderHighlights({ useEffect(() => { if (!flagOn || !hasAuthProvider) return; - let dataExchangeStatus: 'granted' | 'cancel' | 'failure' | null = null; if (!dataExchangeConsumedRef.current) { dataExchangeConsumedRef.current = true; - dataExchangeStatus = consumeDataExchangeReturnRef.current()?.status ?? null; + const returned = consumeDataExchangeReturnRef.current(); + // Act on a decline/failure the moment the return is consumed — BEFORE the + // auth gate below. The shipped YouVersionAuthProvider hydrates the session + // asynchronously, so this effect's first run after a redirect return is + // always unauthenticated; if the status only lived in a local across that + // flip, the discard would be lost and the dialog the user just declined + // would re-open. Discarding here runs exactly once and needs no session: + // a decline means the intent is dead regardless of who ends up signed in. + if (returned && returned.status !== 'granted') { + clearPendingHighlight(); + } } const pending = readPendingHighlight(); @@ -451,13 +471,10 @@ export function useBibleReaderHighlights({ if (!isAuthenticated) return; if (!hasHighlightsPermissionRef.current()) { - // Explicit cancel/failure from a data-exchange return → discard. - if (dataExchangeStatus === 'cancel' || dataExchangeStatus === 'failure') { - clearPendingHighlight(); - return; - } // Signed in but the permission was not granted (e.g. one-fell-swoop // sign-in that returned without it) → offer the data-exchange grant. + // A cancelled/failed data-exchange return never reaches here: its pending + // highlight was discarded at consume time above. // NOTE: this replaces the state-machine's automatic sign-in→data-exchange // hop with a confirm prompt, to avoid an unattended redirect loop on load. setPermissionDialogOpen(true); @@ -503,6 +520,11 @@ export function useBibleReaderHighlights({ } return; } + // DEFERRED: this resume-path failure only logs + reverts. The pending + // highlight was already cleared before enqueueing, so a 401 here loses + // the intent instead of keep-pending + re-prompt like runApply does. + // Rare window (grant just succeeded); follow-up: route resume-write + // failures through runApply's failure handling. logWriteFailures(failures, pending.color); if (token) revertOwned(pending.verses, token); }); From f5261dd3a1b993fc00474870ac17f61dafa419f1 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Mon, 20 Jul 2026 15:00:24 -0500 Subject: [PATCH 03/10] refactor(highlights): xstate statechart rewrite + sign-in dialog + review fixes --- .changeset/fix-highlights-jam-issues.md | 15 + .changeset/highlight-auth-flow.md | 6 +- .changeset/xstate-highlights-flow.md | 12 + CONTEXT.md | 25 + docs/highlight-flow-statechart.md | 102 +++ packages/core/src/Users.ts | 25 +- .../src/YouVersionPlatformConfiguration.ts | 100 ++- .../YouVersionPlatformConfiguration.test.ts | 26 + .../core/src/__tests__/data-exchange.test.ts | 53 ++ .../core/src/__tests__/permissions.test.ts | 69 +- packages/core/src/data-exchange.ts | 14 +- packages/core/src/highlights.ts | 8 +- packages/core/src/styles/bible-reader.css | 14 + .../src/useHighlightAuthActions.test.tsx | 2 + packages/hooks/src/useHighlights.test.tsx | 20 +- packages/hooks/src/useHighlights.ts | 22 +- packages/ui/package.json | 4 +- .../src/components/YouVersionAuthButton.tsx | 8 + .../bible-reader-highlights-machine.ts | 780 ++++++++++++++++++ packages/ui/src/components/bible-reader.tsx | 27 + .../highlight-permission-dialog.tsx | 8 +- .../ui/src/components/sign-in-dialog.test.tsx | 112 +++ packages/ui/src/components/sign-in-dialog.tsx | 99 +++ packages/ui/src/components/ui/button.test.tsx | 21 + packages/ui/src/components/ui/button.tsx | 2 +- ...-highlights.auth-flow.integration.test.tsx | 67 +- ...ble-reader-highlights.integration.test.tsx | 346 +++++++- .../use-bible-reader-highlights.test.tsx | 123 ++- .../components/use-bible-reader-highlights.ts | 635 ++++---------- .../components/verse-action-popover.test.tsx | 41 + .../src/components/verse-action-popover.tsx | 53 +- pnpm-lock.yaml | 45 + 32 files changed, 2310 insertions(+), 574 deletions(-) create mode 100644 .changeset/fix-highlights-jam-issues.md create mode 100644 .changeset/xstate-highlights-flow.md create mode 100644 docs/highlight-flow-statechart.md create mode 100644 packages/ui/src/components/bible-reader-highlights-machine.ts create mode 100644 packages/ui/src/components/sign-in-dialog.test.tsx create mode 100644 packages/ui/src/components/sign-in-dialog.tsx create mode 100644 packages/ui/src/components/ui/button.test.tsx diff --git a/.changeset/fix-highlights-jam-issues.md b/.changeset/fix-highlights-jam-issues.md new file mode 100644 index 00000000..942fa0d8 --- /dev/null +++ b/.changeset/fix-highlights-jam-issues.md @@ -0,0 +1,15 @@ +--- +'@youversion/platform-core': patch +'@youversion/platform-react-hooks': patch +'@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. The sole 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 index c7ce574a..0ccb614d 100644 --- a/.changeset/highlight-auth-flow.md +++ b/.changeset/highlight-auth-flow.md @@ -10,8 +10,10 @@ Add the highlight auth flow: a color tap in BibleReader without a session or the - **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. -Deferred follow-ups (documented at each code site in `use-bible-reader-highlights.ts`, accepted for dark launch): +Deferred follow-ups (documented at each code site in `bible-reader-highlights-machine.ts`, accepted for dark launch): 1. A 401 from an expired token (not a missing permission) misroutes to the permission re-prompt; follow-up is distinguishing auth-expiry from permission-denied at the `isPermissionError` boundary. 2. A failure while applying a resumed pending highlight only logs + reverts (the pending intent was cleared before the write), so a 401 there loses the highlight instead of keep-pending + re-prompt; follow-up is routing resume-write failures through the standard apply failure handling. -3. A 401/403 on `remove` opens the permission dialog with no pending highlight, so the post-grant resume is a no-op; follow-up is not re-prompting on remove failures. +3. ~~A 401/403 on `remove` opens the permission dialog with no pending highlight.~~ Resolved in the PR-288 xstate rewrite: a remove failure now invalidates the cache without re-prompting. + +Note: the signed-out one-fell-swoop redirect described above is superseded by the sign-in dialog introduced in the PR-288 xstate rewrite (see the separate changeset) — a signed-out color tap now opens a dialog before OAuth launches. diff --git a/.changeset/xstate-highlights-flow.md b/.changeset/xstate-highlights-flow.md new file mode 100644 index 00000000..17586297 --- /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 (writes serialized, per-verse ownership tokens, per-sub-write settlement, apply collapses to ranges / remove DELETEs per verse, one refetch per settled batch, scope-change drops the overlay, `live` gates rendering/fetching). + +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..038ebb40 --- /dev/null +++ b/docs/highlight-flow-statechart.md @@ -0,0 +1,102 @@ +# 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. + +```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 + +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: `reconcileOverlay` never retires remove-overlay entries — a removed verse's +optimistic `null` is held until a reset path (scope change, sign-out, or a newer +write re-claiming the verse). Apply entries still retire on reflection, keeping +the tested apply-convergence behavior. This 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. diff --git a/packages/core/src/Users.ts b/packages/core/src/Users.ts index 5fdadcd8..571c5bd7 100644 --- a/packages/core/src/Users.ts +++ b/packages/core/src/Users.ts @@ -125,17 +125,6 @@ export class YouVersionAPIUsers { // Extract user info from ID token const result = this.extractSignInResult(tokens); - // Surface + persist 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). This seeds 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. - const grantedPermissions = parseGrantedPermissions(urlParams); - result.permissions = grantedPermissions; - if (grantedPermissions.length > 0) { - YouVersionPlatformConfiguration.saveGrantedPermissions(grantedPermissions); - } - // Store tokens in configuration. The ID token is intentionally not // persisted — it is only used here to derive the user profile below. YouVersionPlatformConfiguration.saveAuthData( @@ -145,7 +134,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, @@ -153,6 +143,17 @@ export class YouVersionAPIUsers { avatar_url: result.profilePicture, }); + // Surface + persist 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). This seeds 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. + const grantedPermissions = parseGrantedPermissions(urlParams); + result.permissions = grantedPermissions; + 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 25a35ff9..1d1233f7 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') { @@ -81,44 +83,91 @@ export class YouVersionPlatformConfiguration { * 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}. Stored as a JSON string array. + * {@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'; - public static get grantedPermissions(): string[] { - if (typeof localStorage === 'undefined') return []; + /** 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 []; + if (!raw) return null; try { const parsed: unknown = JSON.parse(raw); - if (!Array.isArray(parsed)) return []; - return parsed.filter((entry): entry is string => typeof entry === 'string'); + 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 []; + return null; } } - /** Merges `permissions` into the cache (union), preserving existing entries. */ + 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]); - localStorage.setItem(this.grantedPermissionsKey, JSON.stringify([...merged])); + this.writeStoredGrants(userId, [...merged]); } /** - * Replaces the cache with exactly `permissions` (used to reconcile the cache - * with the authoritative set the server returns on a data-exchange grant). + * Replaces the cache with exactly `permissions` for the current user (used to + * reconcile the cache with the authoritative set the server returns on a + * data-exchange grant). No-ops when signed out. */ public static setGrantedPermissions(permissions: string[]): void { if (typeof localStorage === 'undefined') return; - localStorage.setItem(this.grantedPermissionsKey, JSON.stringify([...new Set(permissions)])); + const userId = this.currentUserId; + if (!userId) return; + this.writeStoredGrants(userId, [...new Set(permissions)]); } /** 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); - localStorage.setItem(this.grantedPermissionsKey, JSON.stringify(next)); + this.writeStoredGrants(userId, next); } public static clearGrantedPermissions(): void { @@ -205,4 +254,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 index 3cb42e2f..4ae0e357 100644 --- a/packages/core/src/__tests__/data-exchange.test.ts +++ b/packages/core/src/__tests__/data-exchange.test.ts @@ -5,6 +5,7 @@ import { DataExchangeClient, buildDataExchangeUrl, parseDataExchangeCallback, + handleDataExchangeCallback, } from '../data-exchange'; import { server } from './setup'; @@ -106,3 +107,55 @@ describe('parseDataExchangeCallback', () => { }); }); }); + +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 index e39b0f32..e1a20871 100644 --- a/packages/core/src/__tests__/permissions.test.ts +++ b/packages/core/src/__tests__/permissions.test.ts @@ -29,8 +29,13 @@ describe('parseGrantedPermissions', () => { }); 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', () => { @@ -67,7 +72,69 @@ describe('YouVersionPlatformConfiguration permission cache', () => { }); it('tolerates malformed stored JSON', () => { - localStorage.setItem('youversion-platform:granted-permissions', '{not 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([]); + }); + + it('same-user merge still accumulates grants', () => { + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + YouVersionPlatformConfiguration.saveGrantedPermissions(['votd']); + expect(YouVersionPlatformConfiguration.grantedPermissions.sort()).toEqual([ + 'highlights', + 'votd', + ]); + }); + }); }); diff --git a/packages/core/src/data-exchange.ts b/packages/core/src/data-exchange.ts index 2ebd2e8b..a0a346c4 100644 --- a/packages/core/src/data-exchange.ts +++ b/packages/core/src/data-exchange.ts @@ -94,6 +94,13 @@ export function buildDataExchangeUrl( 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 = { @@ -122,6 +129,9 @@ export function parseDataExchangeCallback(search: string): DataExchangeCallbackR * 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; @@ -133,7 +143,9 @@ export function handleDataExchangeCallback(): DataExchangeCallbackResult | null } const cleanUrl = new URL(window.location.href); - cleanUrl.search = ''; + 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/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/useHighlightAuthActions.test.tsx b/packages/hooks/src/useHighlightAuthActions.test.tsx index 16d6b721..00b683d2 100644 --- a/packages/hooks/src/useHighlightAuthActions.test.tsx +++ b/packages/hooks/src/useHighlightAuthActions.test.tsx @@ -44,6 +44,8 @@ describe('useHighlightAuthActions', () => { }); 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); 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..41a0aef0 100644 --- a/packages/hooks/src/useHighlights.ts +++ b/packages/hooks/src/useHighlights.ts @@ -54,21 +54,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..b7d6670c 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; + +/** 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: string } | 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; + } +} + +/** The range USFM for one contiguous run: `{2,3} -> "JHN.3.2-3"`, `{5,5} -> "JHN.3.5"`. */ +function runToPassageId(book: string, chapter: string, run: VerseRun): string { + return run.start === run.end + ? `${book}.${chapter}.${run.start}` + : `${book}.${chapter}.${run.start}-${run.end}`; +} + +/** 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(), + }; +} + +function scopesEqual(a: HighlightScope, b: HighlightScope): boolean { + return a.versionId === b.versionId && a.book === b.book && a.chapter === b.chapter; +} + +/** 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: runToPassageId(op.scope.book, op.scope.chapter, run), + color: op.color, + }), + catch: (cause) => + new BibleReaderHighlightError( + 'apply', + runToPassageId(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 ── + noPending: () => readPendingHighlight() === null, + pendingNotAuthed: ({ context }) => readPendingHighlight() !== null && !context.isAuthenticated, + pendingAuthedHasPermission: ({ context }) => + readPendingHighlight() !== null && + context.isAuthenticated && + context.services.current.hasHighlightsPermission(), + pendingAuthedNoPermission: ({ context }) => + readPendingHighlight() !== null && + 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. In-flight writes carry their own scope and + // still settle correctly. This is also the escape hatch for a + // never-converging write — navigating away releases it. + return { + scope: event.scope, + overlay: {}, + reconcile: 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 }) => { + const writeIntent = new Map(current.writeIntent); + const reconcile = new Map(current.reconcile); + const overlay = { ...current.overlay }; + for (const verse of verses) { + writeIntent.set(verse, token); + // A newer write supersedes any older reconciliation still pending for + // this verse, so the reconcile effect can't retire/hold the fresh + // overlay against the old write's target. + reconcile.delete(verse); + overlay[verse] = color; + } + return { writeIntent, reconcile, overlay, 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 }) => { + const writeIntent = new Map(current.writeIntent); + const reconcile = new Map(current.reconcile); + const overlay = { ...current.overlay }; + for (const verse of targetVerses) { + writeIntent.set(verse, token); + reconcile.delete(verse); + overlay[verse] = null; + } + return { writeIntent, reconcile, overlay }; + }); + const op: WriteOp = { + kind: 'remove', + color, + verses: targetVerses, + scope: context.scope, + token, + paint: true, + reprompt: false, + }; + enqueue.raise({ type: 'ENQUEUE', op }); + }), + + /** + * Apply a pending highlight after a granted return. Writes to the PENDING's + * own scope even if the user returned on a different chapter; only paints the + * overlay when that scope matches what is on screen. + */ + applyPendingHighlight: enqueueActions(({ enqueue, context }) => { + const pending = readPendingHighlight(); + if (!pending) return; + clearPendingHighlight(); + 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 }) => { + const writeIntent = new Map(current.writeIntent); + const reconcile = new Map(current.reconcile); + const overlay = { ...current.overlay }; + for (const verse of pending.verses) { + writeIntent.set(verse, token); + reconcile.delete(verse); + overlay[verse] = pending.color; + } + return { writeIntent, reconcile, overlay }; + }); + } + 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 carries `output` + // but is not part of the public event union, so read it via a cast. + const { op, failures, failedVerses, succeededVerses } = ( + event as unknown as { output: WriteResult } + ).output; + + enqueue.assign(({ context: current }) => { + const overlay = { ...current.overlay }; + const reconcile = new Map(current.reconcile); + for (const verse of succeededVerses) { + if (current.writeIntent.get(verse) === op.token) { + reconcile.set(verse, { op: op.kind, color: op.color }); + } + } + for (const verse of failedVerses) { + if (current.writeIntent.get(verse) === op.token && verse in overlay) { + delete overlay[verse]; + } + } + return { overlay, reconcile }; + }); + + // 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. + enqueue(() => stashPendingHighlight(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. + } else if (op.kind === 'apply' && op.reprompt) { + // Network / 5xx on a user apply: overlay already reverted; drop pending. + enqueue(() => clearPendingHighlight()); + } + }), + + // ── 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 31678dda..6b0445d6 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -41,8 +41,11 @@ import { Popover, PopoverClose, PopoverContent, PopoverTrigger } from './ui/popo 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; @@ -525,8 +528,19 @@ function Content() { 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 ?? 'This app'; + const signInPromptMessage = YouVersionPlatformConfiguration.signInPromptMessage; + // Navigating away (book/chapter/version) drops the selection — those verses no // longer exist on screen (ADR-007). useEffect(() => { @@ -745,6 +759,7 @@ function Content() { activeHighlights={activeHighlights} selectedVerses={selectedVerses} highlightedVerses={highlightedVerses} + highlightsEnabled={highlightsEnabled} anchorElement={anchorElement} scrollRoot={scrollContainerRef.current} onHighlight={handleHighlight} @@ -762,6 +777,18 @@ function Content() { theme={background} /> + { + if (!open) cancelSignInDialog(); + }} + appName={signInAppName} + promptMessage={signInPromptMessage} + onConfirm={confirmSignInDialog} + onDecline={cancelSignInDialog} + theme={background} + /> + {showLoadingOverlay ? (
= ({ >
- {t('highlightPermissionTitle')} + {t('dataExchangeHighlightsQuestion')} - {t('highlightPermissionBody')} + {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..a0ab8206 --- /dev/null +++ b/packages/ui/src/components/sign-in-dialog.tsx @@ -0,0 +1,99 @@ +import type { FC } from 'react'; +import * as DialogPrimitive from '@radix-ui/react-dialog'; +import { useTranslation } from 'react-i18next'; +import i18n from '@/i18n'; +import { cn } from '../lib/utils'; +import { YouVersionLogo } from './icons/youversion-logo'; +import { Button } from './ui/button'; + +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/use-bible-reader-highlights.auth-flow.integration.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.auth-flow.integration.test.tsx index 9c2d059e..c3ecd198 100644 --- 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 @@ -66,6 +66,10 @@ beforeEach(() => { 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({ @@ -82,35 +86,46 @@ afterEach(() => { }); describe('highlight auth flow — one-fell-swoop (signed out)', () => { - it('color tap stashes pending, starts sign-in with highlights, then applies on granted return', async () => { + 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, rerender } = renderHook(() => useBibleReaderHighlights(options), { + const { result, unmount } = renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers, }); - // Signed out color tap → auth flow, not a write. + // 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 = readPendingHighlight(); 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'], ); - expect(createHighlight).not.toHaveBeenCalled(); - // Simulate the granted return: handleAuthCallback would have persisted the - // granted permission; the session then resolves authenticated. + // 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; - rerender(); + renderHook(() => useBibleReaderHighlights(options), { wrapper: Providers }); await waitFor(() => { expect(createHighlight).toHaveBeenCalledWith({ @@ -119,9 +134,43 @@ describe('highlight auth flow — one-fell-swoop (signed out)', () => { color: 'fffe00', }); }); - // Pending consumed (the write is the proof it applied; the post-write - // refetch here returns empty server truth, so the optimistic overlay clears). + // Pending consumed (the write is the proof it applied). + expect(readPendingHighlight()).toBeNull(); + }); +}); + +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(readPendingHighlight()).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(readPendingHighlight()).not.toBeNull(); + + act(() => { + result.current.cancelSignInDialog(); + }); + expect(result.current.signInDialogOpen).toBe(false); expect(readPendingHighlight()).toBeNull(); + expect(signIn).not.toHaveBeenCalled(); }); }); 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..c78992e3 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,335 @@ 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 () => { + const consoleError = 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); + consoleError.mockRestore(); + }); + + it('remove partial failure: no ghosts — succeeded DELETEs stay un-painted, failed one reverts (regression)', async () => { + const consoleError = 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); + consoleError.mockRestore(); + }); + + it('apply total failure: everything reverts and the batch still refetches exactly once', async () => { + const consoleError = 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); + consoleError.mockRestore(); + }); + + 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 () => { + const consoleError = 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({}); + }); + consoleError.mockRestore(); + }); + + 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 aad2ce4f..2f1a8760 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -75,9 +75,11 @@ beforeEach(() => { sessionStorage.clear(); signedIn = true; setHighlightsLive(true); - // These tests exercise the authorized-write path, so seed the optimistic - // permission cache. The auth-flow branches (missing session/permission) have - // their own dedicated coverage. + // 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']); }); @@ -212,6 +214,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 () => { @@ -241,8 +245,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), { @@ -262,23 +266,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' }, @@ -298,10 +390,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 c5f74c11..8bcb296b 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -2,18 +2,19 @@ import { isHighlightsLive } from '@/lib/feature-flags'; import { - clearPendingHighlight, - readPendingHighlight, - stashPendingHighlight, -} from '@/lib/pending-highlight'; -import { buildPassageIds } from '@/lib/usfm-ranges'; + bibleReaderHighlightsMachine, + selectHighlightedVerses, + type HighlightScope, + type HighlightServices, + type ServerColors, +} from './bible-reader-highlights-machine'; import { useHighlightAuthActions, useHighlights, YouVersionAuthContext, } from '@youversion/platform-react-hooks'; -import { Result } from 'better-result'; -import { useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; +import { useActorRef, useSelector } from '@xstate/react'; +import { useContext, useEffect, useMemo, useRef } from 'react'; export type UseBibleReaderHighlightsOptions = { versionId: number; @@ -27,12 +28,12 @@ export type UseBibleReaderHighlightsReturn = { /** * 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 stashes a pending highlight and - * enters the highlight auth flow (`'flow'` — sign-in redirect or the - * permission confirm dialog). 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. + * 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`. */ @@ -45,85 +46,63 @@ export type UseBibleReaderHighlightsReturn = { 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; } -/** 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; +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 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; + 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. * * Rendering and fetching are gated on `isHighlightsLive() && isAuthenticated`. - * Writing additionally requires the `highlights` permission; when it (or the - * session) is missing, a color tap stashes a pending highlight and enters the - * highlight auth flow instead — one-fell-swoop sign-in when signed out, or the - * just-in-time permission confirm dialog → data-exchange grant when signed in. - * - * Apply/remove writes are serialized through a single FIFO promise chain so a - * later operation can never race an earlier one to the server (e.g. a DELETE - * overtaking an in-flight POST for the same verse). A per-verse ownership token - * guarantees a failed write only reverts verses no newer operation has claimed, - * so overlapping writes settle to the last-issued operation's state. + * 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. With no provider we keep the - // PR 1 posture: no fetch, no writes, and a color tap never enters the auth - // flow (there is no auth to run) — copy/share still work. + // 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); @@ -139,435 +118,133 @@ export function useBibleReaderHighlights({ } = 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({}); - const [permissionDialogOpen, setPermissionDialogOpen] = useState(false); - - // Drop the optimistic overlay synchronously (during render) the moment the - // scope changes, so an in-flight overlay never paints over another - // chapter's or version's verses — their verse numbers collide. - const overlayScope = `${versionId}:${chapterUsfm}`; - const [loadedOverlayScope, setLoadedOverlayScope] = useState(overlayScope); - if (loadedOverlayScope !== overlayScope) { - setLoadedOverlayScope(overlayScope); - setOverlay({}); - } - - const highlightedVerses = useMemo(() => { - // Gate on `live` here too (useApiData also clears data when disabled): - // sign-out or flag-off must render nothing this very render, including - // any optimistic overlay entries still in state. - if (!live) return {}; - - const map: Record = {}; - const versePrefix = `${chapterUsfm}.`; - // The API returns one highlight per verse (no ranges) — parse the verse - // number off the chapter prefix and normalize color case for rendering. - for (const highlight of highlights?.data ?? []) { - if (highlight.version_id !== versionId) continue; - if (!highlight.passage_id.startsWith(versePrefix)) continue; - const verse = parseInt(highlight.passage_id.slice(versePrefix.length), 10); - if (verse > 0) map[verse] = highlight.color.toLowerCase(); - } - for (const [verse, color] of Object.entries(overlay)) { - if (color === null) delete map[Number(verse)]; - else map[Number(verse)] = color; - } - return map; - }, [live, highlights, overlay, chapterUsfm, versionId]); - - // Refs so callbacks 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; - - // Verses whose write settled successfully and are awaiting the post-write - // refetch. Once fresh data lands, their overlay entries are dropped so the - // server's truth wins again — otherwise a successful write's overlay entry - // would mask every later server-side change to that verse (another device, - // another tab) until navigation. - const confirmedVersesRef = useRef>(new Set()); - - useEffect(() => { - if (confirmedVersesRef.current.size === 0) return; - const confirmed = confirmedVersesRef.current; - confirmedVersesRef.current = new Set(); - setOverlay((current) => { - let changed = false; - const next = { ...current }; - for (const verse of confirmed) { - if (verse in next) { - delete next[verse]; - changed = true; - } - } - return changed ? next : current; - }); - }, [highlights]); - - const patchOverlay = useCallback((verses: number[], value: string | null) => { - setOverlay((current) => { - const next = { ...current }; - for (const verse of verses) next[verse] = value; - return next; - }); - }, []); - - // Per-verse ownership: each write claims its verses with a fresh token. A - // failed write only reverts (drops the optimistic entry, letting server truth - // show) verses it still owns — if a newer write re-claimed a verse, the newer - // write owns its final state. This closes the "loser clobbers winner" window. - const writeIntentRef = useRef>(new Map()); - - const claimVerses = useCallback((verses: number[]): object => { - const token = {}; - for (const verse of verses) writeIntentRef.current.set(verse, token); - return token; - }, []); - - const revertOwned = useCallback((verses: number[], token: object) => { - setOverlay((current) => { - let changed = false; - const next = { ...current }; - for (const verse of verses) { - if (writeIntentRef.current.get(verse) === token && verse in next) { - delete next[verse]; - changed = true; - } - } - return changed ? next : current; - }); - }, []); - - // Single FIFO promise chain serializing every apply/remove write. `.then` on - // both fulfil and reject keeps the chain alive past a failed operation. - const writeQueueRef = useRef>(Promise.resolve()); - const enqueueWrite = useCallback((task: () => Promise) => { - const run = writeQueueRef.current.then(task, task); - writeQueueRef.current = run; - return run; - }, []); - - // Stable refs for values the queued tasks and the resume effect read, so those - // callbacks don't need to re-create on every render. - const scopeRef = useRef({ versionId, book, chapter }); - scopeRef.current = { versionId, book, chapter }; - const createHighlightRef = useRef(createHighlight); - createHighlightRef.current = createHighlight; - const authActionsRef = useRef({ + // 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. + const servicesRef = useRef(null as unknown as HighlightServices); + servicesRef.current = { + createHighlight, + deleteHighlight, + refetch, + hasHighlightsPermission, invalidateHighlightsPermission, + consumeDataExchangeReturn, + startSignInForHighlights, startDataExchangeForHighlights, - }); - authActionsRef.current = { invalidateHighlightsPermission, startDataExchangeForHighlights }; - - const logWriteFailures = useCallback( - (failures: BibleReaderHighlightError[], color: string) => { - for (const failure of failures) { - console.error( - `[YouVersion SDK] Failed to ${failure.operation} highlight (version ${versionId}, ` + - `passage ${failure.passageId}, color ${color})`, - failure, - ); - } - }, - [versionId], - ); - - const runApply = useCallback( - async (color: string, verses: number[], token: object) => { - const results = await Promise.all( - buildPassageIds(book, chapter, verses).map((passageId) => - Result.tryPromise({ - try: () => - createHighlightRef.current({ version_id: versionId, passage_id: passageId, color }), - catch: (cause) => new BibleReaderHighlightError('apply', passageId, cause), - }), - ), - ); - - const failures = results.filter(Result.isError).map((r) => r.error); - if (failures.length === 0) { - for (const verse of verses) { - if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); - } - return; - } - - logWriteFailures(failures, color); - revertOwned(verses, token); - - if (failures.some(isPermissionError)) { - // Server says the permission is gone (or never applied): invalidate the - // optimistic cache, keep this highlight as pending, and re-prompt. - authActionsRef.current.invalidateHighlightsPermission(); - const scope = scopeRef.current; - stashPendingHighlight({ - verses, - color, - versionId: scope.versionId, - book: scope.book, - chapter: scope.chapter, - timestamp: Date.now(), - }); - setPermissionDialogOpen(true); - } else { - // Network / 5xx: overlay already reverted; drop any pending intent. - clearPendingHighlight(); - } - }, - [book, chapter, versionId, logWriteFailures, revertOwned], - ); - - const runRemove = useCallback( - async (color: string, verses: number[], token: object) => { - const results = await Promise.all( - buildPassageIds(book, chapter, verses).map((passageId) => - Result.tryPromise({ - try: () => deleteHighlight(passageId, { version_id: versionId }), - catch: (cause) => new BibleReaderHighlightError('remove', passageId, cause), - }), - ), - ); - - const failures = results.filter(Result.isError).map((r) => r.error); - if (failures.length === 0) { - for (const verse of verses) { - if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); - } - return; - } - - logWriteFailures(failures, color); - revertOwned(verses, token); + }; - if (failures.some(isPermissionError)) { - authActionsRef.current.invalidateHighlightsPermission(); - // DEFERRED: a remove failure has no pending highlight, so the dialog's - // post-grant resume is a no-op — the user grants and nothing visibly - // happens. Follow-up: don't re-prompt on remove failures (invalidating - // the cache above is enough; the next apply re-enters the flow). - setPermissionDialogOpen(true); - } - }, - [book, chapter, versionId, deleteHighlight, logWriteFailures, revertOwned], + const scope: HighlightScope = useMemo( + () => ({ versionId, book, chapter }), + [versionId, book, chapter], ); - const apply = useCallback( - (color: string, verses: number[]): 'applied' | 'flow' | 'noop' => { - if (!flagOn || verses.length === 0 || !hasAuthProvider) return 'noop'; - - const normalizedColor = color.toLowerCase(); - - // Authorized write: optimistic paint now, serialized POST behind the queue. - if (isAuthenticated && hasHighlightsPermission()) { - const token = claimVerses(verses); - patchOverlay(verses, normalizedColor); - void enqueueWrite(() => runApply(normalizedColor, verses, token)); - return 'applied'; - } - - // Enter the highlight auth flow: stash the intent so it survives a redirect - // round-trip (sign-in) or resumes after the confirm dialog's grant. - stashPendingHighlight({ - verses, - color: normalizedColor, - versionId, - book, - chapter, - timestamp: Date.now(), - }); - - if (!isAuthenticated) { - // One-fell-swoop: full-page redirect to sign-in requesting `highlights`. - void startSignInForHighlights().catch((error) => { - console.error('[YouVersion SDK] Failed to start sign-in for highlights', error); - clearPendingHighlight(); - }); - return 'flow'; - } - - // Signed in, permission missing → just-in-time confirm dialog. - setPermissionDialogOpen(true); - return 'flow'; - }, - [ + const actorRef = useActorRef(bibleReaderHighlightsMachine, { + input: { + services: servicesRef, + scope, flagOn, hasAuthProvider, isAuthenticated, - hasHighlightsPermission, - claimVerses, - patchOverlay, - enqueueWrite, - runApply, - versionId, - book, - chapter, - startSignInForHighlights, - ], - ); - - const remove = useCallback( - (color: string, verses: number[]) => { - // Removal only ever touches highlights already on screen, so it needs no - // auth-flow branch: without a live authenticated session there is nothing - // rendered to remove. - 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 token = claimVerses(targetVerses); - patchOverlay(targetVerses, null); - void enqueueWrite(() => runRemove(normalizedColor, targetVerses, token)); }, - [live, claimVerses, patchOverlay, enqueueWrite, runRemove], - ); - - // ---- Resume after an auth round-trip -------------------------------------- - // Runs on mount and whenever the session flips authenticated. Consumes a - // data-exchange return once (reconciling the permission cache + cleaning the - // URL), then resolves any pending highlight: apply it when the permission is - // now granted, discard it on an explicit cancel/failure, or re-prompt when the - // user came back signed in but still without the permission. - const dataExchangeConsumedRef = useRef(false); - const consumeDataExchangeReturnRef = useRef(consumeDataExchangeReturn); - consumeDataExchangeReturnRef.current = consumeDataExchangeReturn; - const hasHighlightsPermissionRef = useRef(hasHighlightsPermission); - hasHighlightsPermissionRef.current = hasHighlightsPermission; + }); + // ── Feed React-owned inputs to the machine ────────────────────────────────── useEffect(() => { - if (!flagOn || !hasAuthProvider) return; + actorRef.send({ type: 'AUTH_CHANGED', flagOn, hasAuthProvider, isAuthenticated }); + }, [actorRef, flagOn, hasAuthProvider, isAuthenticated]); - if (!dataExchangeConsumedRef.current) { - dataExchangeConsumedRef.current = true; - const returned = consumeDataExchangeReturnRef.current(); - // Act on a decline/failure the moment the return is consumed — BEFORE the - // auth gate below. The shipped YouVersionAuthProvider hydrates the session - // asynchronously, so this effect's first run after a redirect return is - // always unauthenticated; if the status only lived in a local across that - // flip, the discard would be lost and the dialog the user just declined - // would re-open. Discarding here runs exactly once and needs no session: - // a decline means the intent is dead regardless of who ends up signed in. - if (returned && returned.status !== 'granted') { - clearPendingHighlight(); - } - } - - const pending = readPendingHighlight(); - if (!pending) return; - - // Sign-in is still resolving the session — wait for the authenticated flip. - if (!isAuthenticated) return; - - if (!hasHighlightsPermissionRef.current()) { - // Signed in but the permission was not granted (e.g. one-fell-swoop - // sign-in that returned without it) → offer the data-exchange grant. - // A cancelled/failed data-exchange return never reaches here: its pending - // highlight was discarded at consume time above. - // NOTE: this replaces the state-machine's automatic sign-in→data-exchange - // hop with a confirm prompt, to avoid an unattended redirect loop on load. - setPermissionDialogOpen(true); + 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]); - // Permission granted: apply the pending highlight. Write to its own scope so - // a return that lands on a different chapter still persists it; paint the - // optimistic overlay only when it matches what's on screen. - clearPendingHighlight(); - const scope = scopeRef.current; - const matchesCurrent = - pending.versionId === scope.versionId && - pending.book === scope.book && - pending.chapter === scope.chapter; - - let token: object | null = null; - if (matchesCurrent) { - token = claimVerses(pending.verses); - patchOverlay(pending.verses, pending.color); - } - - void enqueueWrite(async () => { - const results = await Promise.all( - buildPassageIds(pending.book, pending.chapter, pending.verses).map((passageId) => - Result.tryPromise({ - try: () => - createHighlightRef.current({ - version_id: pending.versionId, - passage_id: passageId, - color: pending.color, - }), - catch: (cause) => new BibleReaderHighlightError('apply', passageId, cause), - }), - ), - ); - const failures = results.filter(Result.isError).map((r) => r.error); - if (failures.length === 0) { - if (token) { - for (const verse of pending.verses) { - if (writeIntentRef.current.get(verse) === token) confirmedVersesRef.current.add(verse); - } - } - return; - } - // DEFERRED: this resume-path failure only logs + reverts. The pending - // highlight was already cleared before enqueueing, so a 401 here loses - // the intent instead of keep-pending + re-prompt like runApply does. - // Rare window (grant just succeeded); follow-up: route resume-write - // failures through runApply's failure handling. - logWriteFailures(failures, pending.color); - if (token) revertOwned(pending.verses, token); - }); - }, [ - flagOn, - hasAuthProvider, - isAuthenticated, - claimVerses, - patchOverlay, - enqueueWrite, - revertOwned, - logWriteFailures, - ]); - - const onPermissionDialogOpenChange = useCallback((open: boolean) => { - setPermissionDialogOpen(open); - // Dismissing via outside-click / Escape is a decline: discard the pending - // highlight but leave the verse selection untouched (the caller owns it). - if (!open) clearPendingHighlight(); - }, []); - - const confirmPermissionDialog = useCallback(() => { - setPermissionDialogOpen(false); - // The pending highlight is already stashed; the data-exchange redirect will - // round-trip and the resume effect applies it on return. - void startDataExchangeForHighlights().catch((error) => { - console.error('[YouVersion SDK] Failed to start data exchange for highlights', error); - clearPendingHighlight(); - }); - }, [startDataExchangeForHighlights]); + // ── 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 (!scopesMatch(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 cancelPermissionDialog = useCallback(() => { - setPermissionDialogOpen(false); - clearPendingHighlight(); - }, []); + 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, permissionDialogOpen, - onPermissionDialogOpenChange, - confirmPermissionDialog, - cancelPermissionDialog, + signInDialogOpen, + ...api, }; } + +function scopesMatch(a: HighlightScope, b: HighlightScope): boolean { + return a.versionId === b.versionId && a.book === b.book && a.chapter === b.chapter; +} diff --git a/packages/ui/src/components/verse-action-popover.test.tsx b/packages/ui/src/components/verse-action-popover.test.tsx index 074b8902..99c92ed9 100644 --- a/packages/ui/src/components/verse-action-popover.test.tsx +++ b/packages/ui/src/components/verse-action-popover.test.tsx @@ -467,4 +467,45 @@ 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(); + }); + + it('still shows the color row by default (highlightsEnabled defaults to true)', () => { + render(); + expect(screen.getByRole('group', { name: 'Highlight colors' })).toBeTruthy(); + expect(applyButtons()).toHaveLength(5); + }); + }); }); 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 */} -