diff --git a/.changeset/bible-reader-highlights-api.md b/.changeset/bible-reader-highlights-api.md deleted file mode 100644 index 2fbeb750..00000000 --- a/.changeset/bible-reader-highlights-api.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -'@youversion/platform-react-hooks': patch -'@youversion/platform-react-ui': patch ---- - -BibleReader highlights now wire to the highlights API behind an internal dark-launch flag; the localStorage highlight store is removed. - -- Highlights are server-only account data (YPE-1034): the temporary `youversion-platform:highlights:` localStorage store is deleted outright, with no migration. -- A new internal `useBibleReaderHighlights` seam fetches the current chapter's highlights via `useHighlights`, renders them through an in-memory optimistic overlay (reverted on write failure), and collapses contiguous verse selections into range USFMs (e.g. `JHN.3.16-18`) on the wire. -- Everything is gated on an internal `HIGHLIGHTS_LIVE` flag (currently off) plus an authenticated session: while off or signed out, the color row is inert — no fetches, no writes, no rendered highlights. Signing out un-renders highlights immediately. -- `@youversion/platform-react-hooks` now exports `YouVersionAuthContext`, the no-throw alternative to `useYVAuth` for components that must tolerate a missing auth provider (its error message already advertised the export). -- `useApiData` (the fetch engine behind all data hooks, e.g. `useHighlights`, `usePassage`) fixes and behavior changes: - - An `enabled` false→true flip now triggers the fetch even when the caller's deps are unchanged — previously a hook that mounted disabled (e.g. waiting on auth) never fetched after being enabled. - - Disabling now clears `data` (and `error`) instead of keeping the last response, so stale account data cannot linger after sign-out or an auth user switch. - - Responses are now latest-wins across `refetch` too: a stale in-flight request (e.g. a refetch for a previous chapter) resolving after a newer fetch no longer overwrites its data. diff --git a/.changeset/checkmark-active-swatch.md b/.changeset/checkmark-active-swatch.md deleted file mode 100644 index 5df3f80e..00000000 --- a/.changeset/checkmark-active-swatch.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -'@youversion/platform-react-ui': patch ---- - -BibleReader's verse action popover now marks active swatches with a checkmark (YPE-1034 PR3, still behind the internal `HIGHLIGHTS_LIVE` flag). - -- **Checkmark swap**: active/remove swatches now render a 24px checkmark (`icons/check`) instead of the X, matching iOS (platform-sdk-swift #179). Same theme-invariant `#121212` fill and identical behavior — tapping still removes the highlight. diff --git a/.changeset/fix-granted-permissions-bracket-notation.md b/.changeset/fix-granted-permissions-bracket-notation.md new file mode 100644 index 00000000..a4a51747 --- /dev/null +++ b/.changeset/fix-granted-permissions-bracket-notation.md @@ -0,0 +1,7 @@ +--- +'@youversion/platform-core': patch +--- + +Fix one-shot sign-in dropping the `highlights` grant when the auth server echoes it back with PHP/Rails bracket-array notation (`granted_permissions[]=highlights`). + +`parseGrantedPermissions` read only the bare `granted_permissions` key, but `URLSearchParams` treats `granted_permissions[]` (and indexed `granted_permissions[0]`) as distinct keys. The server demonstrably uses this bracket notation for the analogous outbound `requested_permissions[]` param it builds on the hosted consent redirect, so a return echo in the same shape was silently discarded — leaving the permission cache empty and re-prompting the just-in-time data-exchange consent after a completed sign-in. The parser now accepts `granted_permissions`, `granted_permissions[]`, and `granted_permissions[]`, keeping it symmetric with the server's encoding. Purely additive: bare `granted_permissions` and all user/state-scoping and fail-closed protections are unchanged. diff --git a/.changeset/fix-highlights-jam-issues.md b/.changeset/fix-highlights-jam-issues.md deleted file mode 100644 index 53c12932..00000000 --- a/.changeset/fix-highlights-jam-issues.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -'@youversion/platform-core': patch -'@youversion/platform-react-hooks': minor -'@youversion/platform-react-ui': patch ---- - -Fix four highlights-stack bugs surfaced by a staging session, plus a fill fade-in. - -- **Invisible primary button (ui):** the `default` button variant paired `bg-background` with `text-primary-foreground`, resolving to white-on-white in the light theme (the highlight permission dialog's Continue button was invisible). It now uses the standard `bg-primary` / `text-primary-foreground` pairing. `YouVersionAuthButton` pins `bg-background` explicitly so its neutral brand surface is unchanged. -- **Optimistic overlay dropped before the write is visible (ui):** after a successful highlight write the seam hook dropped the optimistic overlay as soon as ANY refetch landed, trusting it as server truth. Under read-after-write lag (staging slowness, prod read replicas) that GET often did not yet reflect the write, so a highlight flickered out and back on apply, or a removed highlight reappeared. The overlay is now retired only once a fetch actually reflects the write (apply → the verse shows the written color; remove → the verse no longer shows the removed color); until then the overlay wins. Reset paths (chapter/version change, sign-out) release any write the server never converges on. -- **Refetch coalescing (hooks behavior change):** `useHighlights.createHighlight` / `deleteHighlight` no longer refetch on each call. Direct consumers of `useHighlights` must now call `refetch()` themselves after their mutations settle. The sole in-repo consumer (`useBibleReaderHighlights`) now issues a single refetch after each batch settles — success or failure — so highlighting verses `[2,3,5]` fires two POSTs but only one GET (previously two). Batches with partial failures settle per sub-write: succeeded writes reconcile to server truth, only the failed writes' verses revert. -- **DELETE by range is unsupported (core + ui):** range passage-ids (e.g. `JHN.1.2-3`) returned a non-2xx from staging and threw. The remove path now sends one DELETE per verse (`JHN.1.2`, `JHN.1.3`); POST/apply still collapses to ranges, which works. The `HighlightsClient.deleteHighlight` docstring no longer claims range delete is supported (marked unverified pending API-team confirmation). Large removals issue more DELETEs but still coalesce to a single refetch. -- **Highlight fade-in (core styles):** verse highlight fills now transition their background color (~250ms, disabled under `prefers-reduced-motion`) instead of popping in, matching the Bible app. The selection underline and layout are untouched. - -Known/accepted trade-off (pending product sign-off): the optimistic overlay holds the locally-written value until a fetch reflects that write. A concurrent change from another device to the same verse therefore renders stale until navigation or the next write on this client. This is the deliberate cost of eliminating the flicker/ghost bugs above; documented in code comments in `useBibleReaderHighlights`. diff --git a/.changeset/highlight-auth-flow.md b/.changeset/highlight-auth-flow.md deleted file mode 100644 index da007500..00000000 --- a/.changeset/highlight-auth-flow.md +++ /dev/null @@ -1,11 +0,0 @@ ---- -'@youversion/platform-core': patch -'@youversion/platform-react-hooks': patch -'@youversion/platform-react-ui': patch ---- - -Add the highlight auth flow: a color tap in BibleReader without a session or the `highlights` permission now stashes the intent and runs the two-path grant flow (YPE-1034, still behind the internal `HIGHLIGHTS_LIVE` flag). - -- **Core**: new `DataExchangeClient.updateToken` (`POST /data-exchange/token`, 201 → `{ token }`, Zod-validated) plus `buildDataExchangeUrl` / `parseDataExchangeCallback` / `handleDataExchangeCallback` for the hosted just-in-time grant. Sign-in and data-exchange callbacks now parse `granted_permissions` and seed an optimistic permission cache on `YouVersionPlatformConfiguration` (`grantedPermissions`, `hasPermission`, `saveGrantedPermissions`, `removeGrantedPermission`); the cache is cleared on sign-out and a 401/403 invalidates it (server truth wins). -- **Hooks**: new `useHighlightAuthActions` exposing the one-fell-swoop sign-in (requesting `highlights`), the just-in-time data-exchange redirect, the permission-cache reads/invalidation, and the data-exchange return handler. -- **UI**: `useBibleReaderHighlights` now runs the state machine — pending highlights persist to `sessionStorage` (~10-minute expiry) to survive the redirect round-trip and apply automatically on a granted return; a just-in-time permission confirm dialog (`HighlightPermissionDialog`, copy matched to the native SDK) gates the data-exchange grant. Write failures route by status: 401/403 invalidates the cache, keeps the pending highlight, and re-prompts; 5xx/network reverts the optimistic overlay and discards. Apply/remove writes are serialized through a FIFO queue with per-verse ownership so overlapping operations settle to the last-issued state. Copy/share-only behavior when no auth provider is configured is unchanged. diff --git a/.changeset/highlights-server-backed.md b/.changeset/highlights-server-backed.md new file mode 100644 index 00000000..e5d700da --- /dev/null +++ b/.changeset/highlights-server-backed.md @@ -0,0 +1,24 @@ +--- +'@youversion/platform-core': patch +'@youversion/platform-react-hooks': minor +'@youversion/platform-react-ui': minor +--- + +BibleReader highlights are now real, server-backed YouVersion highlights, with a built-in sign-in and permission flow so a reader's highlights persist to their YouVersion account. + +**Important: previous releases accidentally shipped a demo-only, localStorage-based highlights implementation in BibleReader. It stored highlights only in the local browser and never synced to the reader's YouVersion account. That implementation has been removed and replaced by the server-backed highlights described here.** These highlights are live in this release and enabled by default. + +- Server-backed highlights in BibleReader: tap verses, pick a color, and highlights persist to the reader's YouVersion account instead of the local browser. +- Built-in sign-in dialog and just-in-time `highlights` permission flow: a highlight tap while signed out opens a sign-in dialog; a missing `highlights` permission triggers the YouVersion data-exchange consent flow, and the pending highlight is applied automatically once consent is granted (surviving the redirect round-trip). +- New `BibleReader` props `appName` and `signInPromptMessage` for the sign-in dialog (Swift SDK parity): `appName` names your app in the dialog, and `signInPromptMessage` shows an optional integrator pitch (hidden when unset). +- Optimistic UI: color taps apply and remove instantly while writes settle in the background, with automatic revert on network/5xx failures and a re-prompt on auth failures (401/403). +- Behavior change in `useHighlights`: `createHighlight` and `deleteHighlight` no longer auto-refetch. Direct consumers must now call `refetch()` themselves after their mutations settle; the BibleReader flow coordinates this for you (batching a set of verse writes into a single refetch). +- New exports: `YouVersionAuthContext` from `@youversion/platform-react-hooks` (the no-throw alternative to `useYVAuth` for components that must tolerate a missing auth provider), and a re-export of the `Highlight` type from `@youversion/platform-react-ui`. +- The active color swatch now shows a checkmark (24px `icons/check`) instead of an X, matching the Bible app; tapping it still removes the highlight. +- Highlight-flow reliability fixes surfaced by staging: the previously invisible primary button in the permission dialog now uses the correct `bg-primary` / `text-primary-foreground` pairing (it resolved to white-on-white in light mode); the optimistic overlay is retired only once a refetch actually reflects the write, so highlights no longer flicker out and back under read-after-write lag; removes now send one DELETE per verse (range DELETE is unsupported by the API), while applies still collapse contiguous verses into range USFMs; and highlight fills now fade their background color (~250ms, disabled under `prefers-reduced-motion`) instead of popping in. +- Fix: the core `ApiClient` now treats empty-body 2xx JSON responses as success with no data. Previously a successful highlight delete (a 200 with an empty body) was misread as a failure, briefly flashing the removed highlight back before it disappeared. +- Fix: duplicate processing of the same OAuth callback (e.g. the double-invoked auth init effect under React StrictMode) can no longer clear a just-granted `highlights` permission. The code-for-token exchange is now deduped by authorization code, so a repeated callback shares the one exchange instead of firing a second request whose failure would wipe the freshly seeded grant and re-prompt for permission. +- Fix: a one-shot `highlights` sign-in no longer re-prompts for permission when the auth server omits the grant echo. On the web flow the server returns no `granted_permissions` on the callback and no data-exchange scope on the token, so nothing seeded the permission cache and the flow immediately re-prompted after consent. The permissions requested at sign-in are now stashed (bound to the OAuth `state`) and seeded optimistically on return; the seed is self-correcting because a 401/403 on the first write drops the permission and re-prompts. +- Fix: concurrent token refreshes (e.g. the double-invoked auth init effect under React StrictMode) now share a single in-flight refresh, so a losing duplicate can no longer spend the single-use refresh token a second time and wipe the session on its failure. +- Fix: verse labels and footnote icons now inherit the surrounding text color over highlight fills instead of being painted with the fill color, keeping them legible on highlighted verses. +- Theme-aware highlight rendering: highlight fills render at full opacity in light mode and 30 percent opacity in dark mode, matching the Swift SDK; verse numbers over dark-mode highlights render white. Highlight fills now have subtly rounded corners, with each wrapped line fragment getting its own rounded ends so a multi-line highlight no longer cuts off square at the wrap. The verse-action popover color swatches preview the real fill: in dark mode they show the same dimmed color a highlight will apply. Opening the popover with a mouse or touch no longer flashes a focus ring on the first swatch; keyboard navigation still shows a clearly visible focus ring. diff --git a/.changeset/localization-aria-labels.md b/.changeset/localization-aria-labels.md index 6ec3cff0..ef018ad3 100644 --- a/.changeset/localization-aria-labels.md +++ b/.changeset/localization-aria-labels.md @@ -3,3 +3,5 @@ --- Localize BibleReader font-size/line-spacing and popover close accessibility labels via i18n. + +- The YouVersion Platform logo component's accessible label is now a required prop with no hardcoded English default; the SDK's built-in usages pass localized labels. diff --git a/.changeset/xstate-highlights-flow.md b/.changeset/xstate-highlights-flow.md deleted file mode 100644 index 68d41edb..00000000 --- a/.changeset/xstate-highlights-flow.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -'@youversion/platform-react-ui': patch ---- - -Rewrite the BibleReader highlights flow as an explicit xstate v5 statechart (YPE-1034 / PR-288, still behind the internal `HIGHLIGHTS_LIVE` flag). `useBibleReaderHighlights` is now a thin adapter over `bibleReaderHighlightsMachine`; the machine (authored with `setup()` and named guards/actions/actors, so it is Stately-visualizable) owns the whole flow — optimistic overlay, serialized writes with per-verse ownership, reconcile, the auth flow, and the resume-on-return path. A mermaid statechart lives in `docs/highlight-flow-statechart.md`. All previously-shipped invariants are preserved. - -Behavior changes: - -- **Sign-in dialog first (signed out):** a signed-out color tap now opens a `SignInDialog` (introducing the app, with an optional integrator prompt) instead of redirecting to OAuth immediately. Confirm launches the sign-in redirect requesting `highlights`; decline/dismiss discards the pending highlight and keeps the verse selection. The dialog's `appName` comes from `YouVersionPlatformConfiguration.appName` (falling back to "This app") and its prompt from `signInPromptMessage`. The hook return gains `signInDialogOpen`, `confirmSignInDialog`, and `cancelSignInDialog`. -- **Flag-off hides the highlights UI:** when `HIGHLIGHTS_LIVE` is off, `VerseActionPopover` hides the color row and the remove (checkmark) circles entirely via a new `highlightsEnabled` prop — only Copy / Share remain. Previously the row still rendered while taps were inert. -- **"Vapor" fix:** a deleted highlight no longer briefly reappears when a stale read-replica fetch lands after the delete settled. The reconcile step no longer retires remove-overlay entries; a removed verse is held until a reset path (scope change, sign-out, or a newer write). Apply-side convergence is unchanged. This matches the already-accepted trade-off (a concurrent same-verse edit from another device renders stale until navigation or the next write). -- **Remove-failure no longer re-prompts:** a 401/403 on a remove invalidates the permission cache but no longer opens the permission dialog (there is no pending highlight to resume), resolving a documented deferred wart. diff --git a/CONTEXT.md b/CONTEXT.md index 4d283d04..df3975c7 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -78,6 +78,15 @@ 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). +## Sign-in prompt message + +Optional integrator-owned pitch line shown on the **sign-in dialog** +(`YouVersionPlatformConfiguration.signInPromptMessage`). Hidden when unset; +the SDK does not ship a default (Swift parity). Distinct from localized SDK +copy (`signIn.paragraph`, buttons, etc.). +_Avoid_: app message, integrator message (informal); `signIn.appMessage` +(unused Swift stub key) + ## Highlight auth flow The flow (YPE-1034) that turns a color tap into an applied highlight when the diff --git a/docs/adr/YPE-642-verse-action-popover.md b/docs/adr/YPE-642-verse-action-popover.md index eb522547..83cd5e21 100644 --- a/docs/adr/YPE-642-verse-action-popover.md +++ b/docs/adr/YPE-642-verse-action-popover.md @@ -25,7 +25,7 @@ gone.** #131's `VerseActionPopover` (correct AC logic, tested, already uses Radi | **Remove circle (X)** | Color button that clears an existing highlight. | | **Anchor** | DOM element the popover triangle points at — the last-selected verse's `.yv-v` element. | | **Swatch** | The full-saturation color shown in the circle. | -| **Fill** | The faded (~20-30% alpha) background painted on the verse. | +| **Fill** | The color background painted on the verse (Swift-parity opacity: full in light mode, faded in dark mode). | ## Decisions (ADRs) @@ -89,8 +89,9 @@ palette is hardcoded here too. Simpler than mapping to tokens. - Lowercased to satisfy the API `color` pattern `/^[0-9a-f]{6}$/`. - **Swatch** (circle) = `#` solid + a `1px #121212 @ 20%` inner stroke (applies to all swatches). -- **Fill** (verse) = the hex at **35% opacity** behind the text - (`rgba(, 0.35)`, `HIGHLIGHT_FILL_OPACITY` in `verse.tsx`). +- **Fill** (verse) = the hex behind the text, at a **theme-aware opacity** matching + the Swift SDK (see as-built note below). Superseded the original flat 35% + ("per Figma") value. - **Active/remove swatch** = the solid color circle with a **24px X icon** in the Text/Everdark color (`--yv-gray-50` = `#121212`, theme-invariant) — replaces the old stroke-based selected indicator. @@ -155,9 +156,32 @@ Selection is always enabled in BibleReader (no opt-out prop for now; YAGNI). context. Copy/Share/anchor all need the rendered verse DOM (which lives in Content), so Root ownership would fragment the feature. BibleTextView stays presentational. No new Root props — smaller API surface. -- **ADR-005 mechanism:** fill uses `rgba(r,g,b,0.25)` (computed from the stored +- **ADR-005 mechanism:** fill uses `rgba(r,g,b,)` (computed from the stored hex in `verse.tsx` `hexToRgba`), not `color-mix`. Same alpha-composite result, zero browser-support caveats. +- **ADR-005 fill opacity revised (Swift parity, user decision 2026-07-22):** the + original flat `0.35` ("per Figma") is replaced by theme-aware opacity to match the + Swift SDK, the cross-SDK UX baseline: **light mode `1.0`, dark mode `0.3`** + (`HIGHLIGHT_FILL_OPACITY_LIGHT` / `HIGHLIGHT_FILL_OPACITY_DARK` in `verse.tsx`, + mirroring Swift's `darkMode ? 0.3 : 1.0`). Additionally, in dark mode a highlighted + verse's number label (`.yv-vlbl`) is painted **white** so it stays legible over the + fill (Swift `BibleTextView+Rendering.swift`); unhighlighted labels keep their + inherited color. Both the fill and the label color are set/cleared in the same + imperative `useLayoutEffect` paint path, which now reads the reader's theme + (`theme` prop, falling back to the provider's `useTheme()`). +- **ADR-005 rounded fills (web-native divergence, user-approved 2026-07-22):** the + Swift SDK paints square highlight fills; the web reader instead rounds them — + `border-radius: 4px`, `box-decoration-break: clone` (so each wrapped line + fragment gets its own rounded ends), and `2px` inline padding. These are set + **statically** on every `.yv-v` (not just highlighted verses) in + `bible-reader.css`, so applying/removing a fill only toggles `background-color`: + no padding appears or disappears, so there is no reflow or layout shift and the + 250ms fade stays smooth. Structural styles live in CSS (not the imperative paint + path, which only sets colors). Additionally, the verse-action popover color + swatches preview the applied fill: dark mode dims each swatch to the same `0.3` + alpha (`HIGHLIGHT_FILL_OPACITY_DARK`) via `hexToRgba` from `verse.tsx`, with the + active-swatch checkmark switched to white and the inner stroke flipped to a light + `rgba(255,255,255,0.2)` so the dimmed circle stays legible on the dark popover. - **Verse-tap vs outside-click:** ADR-007 says outside-click clears selection, but Radix treats a *second verse tap* as an outside-click too. The popover's `onInteractOutside` calls `preventDefault()` when the target is inside diff --git a/examples/vite-react/src/ThemedApp.tsx b/examples/vite-react/src/ThemedApp.tsx index c91fb4d4..eeefd473 100644 --- a/examples/vite-react/src/ThemedApp.tsx +++ b/examples/vite-react/src/ThemedApp.tsx @@ -17,6 +17,7 @@ export default function ThemedApp() { appKey={appKey ?? ''} includeAuth authRedirectUrl={authRedirectUrl} + appName="SDK Demo" > diff --git a/packages/core/src/SignInWithYouVersionPKCE.ts b/packages/core/src/SignInWithYouVersionPKCE.ts index 28fb4767..23608900 100644 --- a/packages/core/src/SignInWithYouVersionPKCE.ts +++ b/packages/core/src/SignInWithYouVersionPKCE.ts @@ -69,11 +69,12 @@ export class SignInWithYouVersionPKCEAuthorizationRequestBuilder { } // YouVersion data-exchange permissions (e.g. `highlights`) are intentionally - // NOT OIDC scopes. They ride alongside the standard scopes as a repeatable - // `requested_permissions[]` query param and are authorized via a separate - // per-app ACL rather than the token's scope claim. - for (const permission of permissions ?? []) { - queryParams.append('requested_permissions[]', permission); + // NOT OIDC scopes. They ride alongside `scope` as a single comma-joined + // `requested_permissions` query param (Swift/Kotlin wire format) and are + // authorized via a separate per-app ACL rather than the token's scope claim. + const permissionsValue = [...(permissions ?? [])].sort().join(','); + if (permissionsValue) { + queryParams.set('requested_permissions', permissionsValue); } components.search = queryParams.toString(); diff --git a/packages/core/src/Users.ts b/packages/core/src/Users.ts index 98846ddb..e356f716 100644 --- a/packages/core/src/Users.ts +++ b/packages/core/src/Users.ts @@ -3,7 +3,136 @@ import { YouVersionUserInfo } from './YouVersionUserInfo'; import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; import { SignInWithYouVersionPKCEAuthorizationRequestBuilder } from './SignInWithYouVersionPKCE'; import { SignInWithYouVersionResult } from './SignInWithYouVersionResult'; -import { parseGrantedPermissions } from './permissions'; +import { parseGrantedPermissions, parsePermissionList } from './permissions'; + +/** Stash key for `granted_permissions` seen on the pre-code OAuth hop. */ +const PENDING_GRANTED_PERMISSIONS_KEY = 'youversion-auth-pending-granted-permissions'; + +/** + * Stash key for the data-exchange permissions REQUESTED at `signIn` (e.g. + * `highlights`), bound to the OAuth `state`. Seeded optimistically into the + * granted cache on callback because the web flow returns no grant echo — see + * the union in {@link YouVersionAPIUsers.exchangeCodeForTokens}. + */ +const REQUESTED_PERMISSIONS_KEY = 'youversion-auth-requested-permissions'; + +/** OIDC scopes that must not be stored in the data-exchange permission cache. */ +const OIDC_SCOPES = new Set(['openid', 'profile', 'email', 'offline_access']); + +type StatePermissionsStash = { + state: string; + permissions: string[]; +}; + +type PendingGrantedPermissionsStash = StatePermissionsStash; + +type RequestedPermissionsStash = StatePermissionsStash; + +/** + * Dedupe map for in-flight/settled code-for-token exchanges, keyed by the + * single-use authorization `code`. Under React StrictMode the auth init effect + * double-invokes and both runs call {@link YouVersionAPIUsers.handleAuthCallback} + * with the same `code` still in the URL; without deduping, the second exchange + * 400s (single-use code already spent) and its catch path clears the tokens and + * granted-permissions cache the first run just seeded. Entries are kept after + * settlement — a code is single-use per page load, and a failed exchange must + * not be retried — and module state resets naturally on reload. + */ +const inFlightCodeExchanges = new Map>(); + +/** Test-only: clears the code-exchange dedupe map between test cases. */ +export function __resetAuthCallbackDedupeForTests(): void { + inFlightCodeExchanges.clear(); +} + +/** + * In-flight token refresh shared across concurrent callers. There is only ever + * one refresh token, so a single slot suffices (no keying needed). Under React + * StrictMode the auth init effect double-invokes and both runs call + * {@link YouVersionAPIUsers.refreshTokenIfNeeded} while the token reads expired; + * without sharing, both spend the same single-use refresh token and the loser's + * failure runs {@link YouVersionPlatformConfiguration.clearAuthTokens}, wiping + * the session and granted-permissions cache the winner just re-established. + * + * Unlike the code-exchange map, this slot is CLEARED once settled: a later, + * genuine refresh (e.g. the token expiring an hour on) must be able to run. + * Sharing applies only to calls that overlap in time. + */ +let inFlightRefresh: Promise | null = null; + +/** Test-only: clears the shared in-flight refresh between test cases. */ +export function __resetTokenRefreshDedupeForTests(): void { + inFlightRefresh = null; +} + +/** Persist early grants bound to the OAuth `state` that produced them. */ +const stashPendingGrantedPermissions = (state: string, permissions: string[]): void => { + const payload: PendingGrantedPermissionsStash = { + state, + permissions, + }; + localStorage.setItem(PENDING_GRANTED_PERMISSIONS_KEY, JSON.stringify(payload)); +}; + +/** + * Read early grants only when they were stashed for this OAuth `state`. + * Mismatched or legacy unbound values are discarded (fail closed). + */ +const readPendingGrantedPermissions = (state: string): string[] => { + const raw = localStorage.getItem(PENDING_GRANTED_PERMISSIONS_KEY); + localStorage.removeItem(PENDING_GRANTED_PERMISSIONS_KEY); + if (!raw) return []; + + try { + const parsed = JSON.parse(raw) as PendingGrantedPermissionsStash; + if ( + parsed && + typeof parsed === 'object' && + parsed.state === state && + Array.isArray(parsed.permissions) + ) { + return parsed.permissions.filter((permission) => typeof permission === 'string'); + } + } catch { + // Legacy plain comma-list (no state binding) — discard. + } + return []; +}; + +/** Persist the requested data-exchange permissions bound to the OAuth `state`. */ +const stashRequestedPermissions = (state: string, permissions: string[]): void => { + const payload: RequestedPermissionsStash = { + state, + permissions, + }; + localStorage.setItem(REQUESTED_PERMISSIONS_KEY, JSON.stringify(payload)); +}; + +/** + * Read the requested permissions only when they were stashed for this OAuth + * `state`. Mismatched or malformed values are discarded (fail closed), mirroring + * {@link readPendingGrantedPermissions}. + */ +const readRequestedPermissions = (state: string): string[] => { + const raw = localStorage.getItem(REQUESTED_PERMISSIONS_KEY); + localStorage.removeItem(REQUESTED_PERMISSIONS_KEY); + if (!raw) return []; + + try { + const parsed = JSON.parse(raw) as RequestedPermissionsStash; + if ( + parsed && + typeof parsed === 'object' && + parsed.state === state && + Array.isArray(parsed.permissions) + ) { + return parsed.permissions.filter((permission) => typeof permission === 'string'); + } + } catch { + // Malformed stash — discard. + } + return []; +}; export class YouVersionAPIUsers { /** @@ -15,7 +144,7 @@ export class YouVersionAPIUsers { * @param redirectURL - The URL to redirect back to after authentication. * @param scopes - The OIDC scopes given to the authentication call (e.g. `profile`, `email`). * @param permissions - YouVersion data-exchange permissions to request (e.g. `highlights`). - * These are sent as `requested_permissions[]` params, separate from OIDC scopes. + * These are sent as a comma-joined `requested_permissions` param, separate from OIDC scopes. * @throws An error if authentication fails or configuration is invalid. */ static async signIn( @@ -45,6 +174,22 @@ export class YouVersionAPIUsers { : redirectURL.toString(); localStorage.setItem('youversion-auth-redirect-uri', redirectUrlString); localStorage.setItem('youversion-auth-state', authorizationRequest.parameters.state); + // Clear any stash left by a prior abandoned flow (it's only ever produced later, + // during the callback pre-code hop, and never needs to survive a new signIn). + // Otherwise a previous user's abandoned grants could leak into this flow. + localStorage.removeItem(PENDING_GRANTED_PERMISSIONS_KEY); + // Same hygiene for a stale requested-permissions stash from an abandoned flow. + localStorage.removeItem(REQUESTED_PERMISSIONS_KEY); + // Same hygiene for an abandoned just-in-time data-exchange initiator. + YouVersionPlatformConfiguration.clearDataExchangeInitiator(); + + // Stash the REQUESTED data-exchange permissions (not the OIDC scopes) bound + // to this OAuth `state`. On the web flow the server returns no grant echo + // (no URL param, no token scope) for these, so the callback seeds them + // optimistically from here — see exchangeCodeForTokens. Empty/absent → no stash. + if (permissions && permissions.length > 0) { + stashRequestedPermissions(authorizationRequest.parameters.state, permissions); + } // Simple redirect to authorization URL window.location.href = authorizationRequest.url.toString(); @@ -84,9 +229,17 @@ export class YouVersionAPIUsers { } // If we don't have a code, this might be the first callback with user data - // We need to redirect to the server callback to get the authorization code + // We need to redirect to the server callback to get the authorization code. + // Stash any granted_permissions from this hop first — Swift keeps the original + // callback URL for grants, but the web flow navigates away to /auth/callback + // and the final redirect with `code` may omit them. if (!code && state) { + const earlyGrants = parseGrantedPermissions(urlParams); + if (earlyGrants.length > 0) { + stashPendingGrantedPermissions(state, earlyGrants); + } this.obtainLocation(window.location.href, state); + return null; } // Get stored auth data @@ -97,6 +250,34 @@ export class YouVersionAPIUsers { throw new Error('Missing required authentication parameters'); } + // Dedupe concurrent/repeated exchanges of this single-use code (e.g. the + // StrictMode double-invocation) so only one token request is ever made and + // no duplicate-400 catch path can wipe the grants the winning run seeded. + // The synchronous get/set here (before the first await inside the exchange) + // makes the check re-entrancy safe. + const existingExchange = inFlightCodeExchanges.get(code); + if (existingExchange) { + return existingExchange; + } + + const exchange = this.exchangeCodeForTokens(code, codeVerifier, redirectUri, state, urlParams); + inFlightCodeExchanges.set(code, exchange); + return exchange; + } + + /** + * Exchanges a single-use authorization `code` for tokens, persists the + * resulting session/profile/grants, and returns the sign-in result. Callers + * must dedupe by `code` (see {@link inFlightCodeExchanges}); on failure the + * partial session is cleared and the error rethrown. + */ + private static async exchangeCodeForTokens( + code: string, + codeVerifier: string, + redirectUri: string, + state: string | null, + urlParams: URLSearchParams, + ): Promise { try { // Exchange authorization code for tokens const tokenRequest = SignInWithYouVersionPKCEAuthorizationRequestBuilder.tokenURLRequest( @@ -122,11 +303,31 @@ export class YouVersionAPIUsers { token_type: string; }; - // Parse the data-exchange permissions the server granted. The server - // echoes them as `granted_permissions` on the callback URL (comma- or - // space-separated, param may repeat). Used below to seed the optimistic - // permission cache — the single source of truth for granted permissions. - const grantedPermissions = parseGrantedPermissions(urlParams); + // Match Swift: union grants from (1) this URL, (2) stashed pre-code hop, + // (3) token scope — then drop OIDC scopes before seeding the data-exchange + // permission cache. Stash is state-bound so a leftover from another flow + // cannot seed this user's optimistic permission cache. + // + // (4) is the optimistic seed of the permissions REQUESTED at signIn. On the + // web flow the auth server returns ZERO grant evidence for data-exchange + // permissions after consent — captured live 2026-07-23, the pre-code hop + // carried only `state`, the code hop `scope="profile openid"`, and the token + // body `scope: "profile openid"` — so sources (1)-(3) are all empty and the + // machine would re-prompt right after sign-in. Seeding the request is + // self-correcting: a 401/403 on the first write drops the permission + // (removeGrantedPermission) and the machine's PERMISSION_LOST path re-prompts. + // Same fail-closed state binding as the granted stash; a Set union means a + // future server echo never double-counts. + const stashedGrants = state ? readPendingGrantedPermissions(state) : []; + const requestedGrants = state ? readRequestedPermissions(state) : []; + const grantedPermissions = [ + ...new Set([ + ...parseGrantedPermissions(urlParams), + ...stashedGrants, + ...parsePermissionList(tokens.scope), + ...requestedGrants, + ]), + ].filter((permission) => !OIDC_SCOPES.has(permission)); // Extract user info from ID token const result = this.extractSignInResult(tokens); @@ -160,6 +361,8 @@ export class YouVersionAPIUsers { localStorage.removeItem('youversion-auth-code-verifier'); localStorage.removeItem('youversion-auth-redirect-uri'); localStorage.removeItem('youversion-auth-state'); + localStorage.removeItem(PENDING_GRANTED_PERMISSIONS_KEY); + localStorage.removeItem(REQUESTED_PERMISSIONS_KEY); // Clean up URL const cleanUrl = new URL(window.location.href); @@ -172,6 +375,8 @@ export class YouVersionAPIUsers { localStorage.removeItem('youversion-auth-code-verifier'); localStorage.removeItem('youversion-auth-redirect-uri'); localStorage.removeItem('youversion-auth-state'); + localStorage.removeItem(PENDING_GRANTED_PERMISSIONS_KEY); + localStorage.removeItem(REQUESTED_PERMISSIONS_KEY); throw error; } } @@ -428,6 +633,36 @@ export class YouVersionAPIUsers { return true; // Token is still valid } + // Share one refresh across overlapping callers so a duplicate (e.g. the + // StrictMode double-invocation) can't spend the single-use refresh token a + // second time and wipe the session on its failure. The get/set here is + // synchronous before the first await, so the check is re-entrancy safe. + const existingRefresh = inFlightRefresh; + if (existingRefresh) { + return existingRefresh; + } + + const refresh = this.performTokenRefresh(); + inFlightRefresh = refresh; + + try { + return await refresh; + } finally { + // Clear the slot once settled so a later, genuine refresh can run. Guard + // against clobbering a newer refresh that a subsequent call may have set. + if (inFlightRefresh === refresh) { + inFlightRefresh = null; + } + } + } + + /** + * Performs a single token refresh, clearing the session exactly once on + * failure. Callers must share this via {@link inFlightRefresh} so a duplicate + * refresh never runs concurrently; a shared failure clears tokens once and + * resolves false for every caller awaiting it. + */ + private static async performTokenRefresh(): Promise { try { const result = await this.refreshTokens(); return !!result; diff --git a/packages/core/src/YouVersionPlatformConfiguration.ts b/packages/core/src/YouVersionPlatformConfiguration.ts index 2222dba1..43149591 100644 --- a/packages/core/src/YouVersionPlatformConfiguration.ts +++ b/packages/core/src/YouVersionPlatformConfiguration.ts @@ -76,6 +76,7 @@ export class YouVersionPlatformConfiguration { this.saveAuthData(null, null, null); this.saveUserInfo(null); this.clearGrantedPermissions(); + this.clearDataExchangeInitiator(); } /** @@ -163,6 +164,42 @@ export class YouVersionPlatformConfiguration { localStorage.removeItem(this.grantedPermissionsKey); } + /** + * The id of the user who initiated the pending data-exchange redirect. + * + * The just-in-time data-exchange flow is fire-and-forget: it full-page + * redirects to a hosted consent page and the grant comes back on the return + * URL. If a different user signs in on another tab before the redirect + * returns, the grant would otherwise be saved under whoever is signed in when + * the callback loads. Recording the initiating user here lets the callback + * verify the same user is still signed in before honoring the grant. + */ + private static readonly dataExchangeInitiatorKey = 'youversion-platform:data-exchange-initiator'; + + /** + * Records the current user as the initiator of a data-exchange redirect. + * No-ops when signed out — the just-in-time flow only starts authenticated + * (minting the token requires an access token), so a missing initiator on + * return is treated as untrusted by {@link dataExchangeInitiator}'s consumer. + */ + public static saveDataExchangeInitiator(): void { + if (typeof localStorage === 'undefined') return; + const userId = this.currentUserId; + if (!userId) return; + localStorage.setItem(this.dataExchangeInitiatorKey, userId); + } + + /** The initiating user's id for the pending data exchange, or `null`. */ + public static get dataExchangeInitiator(): string | null { + if (typeof localStorage === 'undefined') return null; + return localStorage.getItem(this.dataExchangeInitiatorKey); + } + + public static clearDataExchangeInitiator(): void { + if (typeof localStorage === 'undefined') return; + localStorage.removeItem(this.dataExchangeInitiatorKey); + } + /** Optimistic check against the permission cache. Server 401/403 still wins. */ public static hasPermission(permission: string): boolean { return this.grantedPermissions.includes(permission); diff --git a/packages/core/src/__tests__/SignInWithYouVersionPKCE.test.ts b/packages/core/src/__tests__/SignInWithYouVersionPKCE.test.ts index c7ef4878..d8c28d99 100644 --- a/packages/core/src/__tests__/SignInWithYouVersionPKCE.test.ts +++ b/packages/core/src/__tests__/SignInWithYouVersionPKCE.test.ts @@ -216,7 +216,7 @@ describe('SignInWithYouVersionPKCEAuthorizationRequestBuilder', () => { expect(scope).toBe('openid'); }); - it('should send requested permissions as requested_permissions[] params, not scopes', async () => { + it('should send requested permissions as requested_permissions (comma-joined), not scopes', async () => { const redirectURL = new URL('https://example.com/callback'); const result = await SignInWithYouVersionPKCEAuthorizationRequestBuilder.make( @@ -228,15 +228,14 @@ describe('SignInWithYouVersionPKCEAuthorizationRequestBuilder', () => { const params = new URLSearchParams(result.url.search); - // Permission rides alongside scopes as a separate param - expect(params.getAll('requested_permissions[]')).toEqual(['highlights']); + // Permission rides alongside scopes as a separate param (Swift wire format) + expect(params.get('requested_permissions')).toBe('highlights'); + expect(params.getAll('requested_permissions[]')).toEqual([]); // ...and is NOT folded into the OIDC scope value expect(params.get('scope')).not.toContain('highlights'); - // Raw query string uses the encoded array-bracket syntax the API expects - expect(result.url.search).toContain('requested_permissions%5B%5D=highlights'); }); - it('should support multiple requested permissions', async () => { + it('should support multiple requested permissions as a sorted comma-joined value', async () => { const redirectURL = new URL('https://example.com/callback'); const result = await SignInWithYouVersionPKCEAuthorizationRequestBuilder.make( @@ -247,10 +246,10 @@ describe('SignInWithYouVersionPKCEAuthorizationRequestBuilder', () => { ); const params = new URLSearchParams(result.url.search); - expect(params.getAll('requested_permissions[]')).toEqual(['highlights', 'votd']); + expect(params.get('requested_permissions')).toBe('highlights,votd'); }); - it('should omit requested_permissions[] when no permissions are requested', async () => { + it('should omit requested_permissions when no permissions are requested', async () => { const redirectURL = new URL('https://example.com/callback'); const result = await SignInWithYouVersionPKCEAuthorizationRequestBuilder.make( @@ -260,7 +259,7 @@ describe('SignInWithYouVersionPKCEAuthorizationRequestBuilder', () => { ); const params = new URLSearchParams(result.url.search); - expect(params.getAll('requested_permissions[]')).toEqual([]); + expect(params.get('requested_permissions')).toBeNull(); }); it('should not duplicate openid if already present', async () => { diff --git a/packages/core/src/__tests__/Users.test.ts b/packages/core/src/__tests__/Users.test.ts index 02fddbff..5126943f 100644 --- a/packages/core/src/__tests__/Users.test.ts +++ b/packages/core/src/__tests__/Users.test.ts @@ -1,17 +1,119 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { YouVersionAPIUsers } from '../Users'; +import { + YouVersionAPIUsers, + __resetAuthCallbackDedupeForTests, + __resetTokenRefreshDedupeForTests, +} from '../Users'; import { YouVersionPlatformConfiguration } from '../YouVersionPlatformConfiguration'; import { YouVersionUserInfo } from '../YouVersionUserInfo'; import { setupBrowserMocks, cleanupBrowserMocks } from './mocks/browser'; const mockFetch = vi.fn(); +// Shared JWT fixture (HS256 header + `{sub,name,iat,email,profile_picture}` +// payload + invalid signature) reused across the token-exchange tests. +const MOCK_ID_TOKEN = + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJlbWFpbCI6ImpvaG5AZXhhbXBsZS5jb20iLCJwcm9maWxlX3BpY3R1cmUiOiJodHRwczovL2V4YW1wbGUuY29tL2F2YXRhci5qcGcifQ.invalid-signature'; + +/** Builds the token payload the /auth/token exchange returns; only `scope` varies per test. */ +function makeTokens(scope: string) { + return { + access_token: 'access-token-123', + expires_in: 3600, + id_token: MOCK_ID_TOKEN, + refresh_token: 'refresh-token-456', + scope, + token_type: 'Bearer', + }; +} + describe('YouVersionAPIUsers', () => { let mocks: ReturnType; + const STANDARD_CALLBACK_SEARCH = '?state=test-state&code=auth-code'; + const STANDARD_CALLBACK_HREF = 'https://example.com/callback?state=test-state&code=auth-code'; + const DEFAULT_CALLBACK_PROFILE = { + sub: '1234567890', + name: 'John Doe', + email: 'john@example.com', + }; + + /** + * Stubs the crypto primitives (`getRandomValues`, `subtle.digest`, `btoa`) + * that the PKCE `signIn` flow needs to build a deterministic authorize URL. + */ + function stubSignInCrypto() { + vi.spyOn(crypto, 'getRandomValues').mockImplementation((array: ArrayBufferView) => { + if (array instanceof Uint8Array) { + for (let i = 0; i < array.length; i++) { + array[i] = i; + } + } + return array; + }); + + vi.spyOn(crypto.subtle, 'digest').mockResolvedValue(new Uint8Array(32).buffer); + mocks.btoa.mockReturnValue('mockBase64Value'); + } + + /** + * Wires up the shared handleAuthCallback token-exchange setup: callback URL, + * localStorage reads (state/verifier/redirect-uri + optional pending grant), + * a successful token response for `scope`, and the decoded JWT profile. + */ + function setupCallbackFlow({ + scope, + pendingGrant, + requestedGrant, + search = STANDARD_CALLBACK_SEARCH, + href = STANDARD_CALLBACK_HREF, + profile = DEFAULT_CALLBACK_PROFILE, + }: { + scope: string; + pendingGrant?: string; + requestedGrant?: string; + search?: string; + href?: string; + profile?: Record; + }) { + mocks.window.location.search = search; + mocks.window.location.href = href; + + mocks.localStorage.getItem.mockImplementation((key: string) => { + switch (key) { + case 'youversion-auth-state': + return 'test-state'; + case 'youversion-auth-code-verifier': + return 'code-verifier-123'; + case 'youversion-auth-redirect-uri': + return 'https://example.com/callback'; + case 'youversion-auth-pending-granted-permissions': + return pendingGrant ?? null; + case 'youversion-auth-requested-permissions': + return requestedGrant ?? null; + default: + return null; + } + }); + + mockFetch.mockResolvedValue({ + ok: true, + status: 200, + statusText: 'OK', + text: vi.fn().mockResolvedValue(JSON.stringify(makeTokens(scope))), + }); + + vi.mocked(atob).mockReturnValue(JSON.stringify(profile)); + } + beforeEach(() => { vi.clearAllMocks(); + // The code-exchange dedupe map is module-scoped and persists across test + // cases within this file (it only resets on page reload in production). + // Clear it so tests reusing `code=auth-code` don't see a prior exchange. + __resetAuthCallbackDedupeForTests(); + // Setup global mocks mocks = setupBrowserMocks(); vi.stubGlobal('fetch', mockFetch); @@ -35,6 +137,12 @@ describe('YouVersionAPIUsers', () => { }); describe('signIn', () => { + afterEach(() => { + // Restore the crypto spies here so restoration still happens even when an + // assertion throws mid-test. + vi.restoreAllMocks(); + }); + it('should throw error when appKey is not set', async () => { YouVersionPlatformConfiguration.appKey = null; @@ -44,17 +152,7 @@ describe('YouVersionAPIUsers', () => { }); it('should create authorization request and redirect on successful signIn', async () => { - vi.spyOn(crypto, 'getRandomValues').mockImplementation((array: ArrayBufferView) => { - if (array instanceof Uint8Array) { - for (let i = 0; i < array.length; i++) { - array[i] = i; - } - } - return array; - }); - - vi.spyOn(crypto.subtle, 'digest').mockResolvedValue(new Uint8Array(32).buffer); - mocks.btoa.mockReturnValue('mockBase64Value'); + stubSignInCrypto(); const redirectURL = 'https://example.com/callback'; @@ -76,28 +174,61 @@ describe('YouVersionAPIUsers', () => { // Verify redirect occurred expect(mocks.window.location.href).toContain('https://api.youversion.com/auth/authorize'); - - vi.restoreAllMocks(); }); it('should forward requested permissions to the authorize URL', async () => { - vi.spyOn(crypto, 'getRandomValues').mockImplementation((array: ArrayBufferView) => { - if (array instanceof Uint8Array) { - for (let i = 0; i < array.length; i++) { - array[i] = i; - } - } - return array; - }); + stubSignInCrypto(); + + await YouVersionAPIUsers.signIn('https://example.com/callback', ['profile'], ['highlights']); + + expect(mocks.window.location.href).toContain('requested_permissions=highlights'); + }); + + it('clears any stale pre-code granted-permissions stash from a prior abandoned flow', async () => { + stubSignInCrypto(); + + await YouVersionAPIUsers.signIn('https://example.com/callback'); + + expect(mocks.localStorage.removeItem).toHaveBeenCalledWith( + 'youversion-auth-pending-granted-permissions', + ); + }); + + it('clears any stale requested-permissions stash from a prior abandoned flow', async () => { + stubSignInCrypto(); + + await YouVersionAPIUsers.signIn('https://example.com/callback'); + + expect(mocks.localStorage.removeItem).toHaveBeenCalledWith( + 'youversion-auth-requested-permissions', + ); + }); - vi.spyOn(crypto.subtle, 'digest').mockResolvedValue(new Uint8Array(32).buffer); - mocks.btoa.mockReturnValue('mockBase64Value'); + it('stashes the requested data-exchange permissions bound to the OAuth state', async () => { + stubSignInCrypto(); await YouVersionAPIUsers.signIn('https://example.com/callback', ['profile'], ['highlights']); - expect(mocks.window.location.href).toContain('requested_permissions%5B%5D=highlights'); + // The stash is keyed by the generated state; assert the payload shape and + // that the requested permissions (not the OIDC scopes) are what's stored. + const requestedCall = mocks.localStorage.setItem.mock.calls.find( + (args: unknown[]) => args[0] === 'youversion-auth-requested-permissions', + ) as [string, string] | undefined; + expect(requestedCall).toBeTruthy(); + const stored = JSON.parse(requestedCall![1]) as { state: string; permissions: string[] }; + expect(stored.permissions).toEqual(['highlights']); + expect(typeof stored.state).toBe('string'); + }); - vi.restoreAllMocks(); + it('does not stash requested permissions when none are requested', async () => { + stubSignInCrypto(); + + await YouVersionAPIUsers.signIn('https://example.com/callback', ['profile']); + + expect(mocks.localStorage.setItem).not.toHaveBeenCalledWith( + 'youversion-auth-requested-permissions', + expect.any(String), + ); }); }); @@ -145,15 +276,35 @@ describe('YouVersionAPIUsers', () => { return null; }); - // In test environment, the redirect continues execution, so expect the eventual error - await expect(YouVersionAPIUsers.handleAuthCallback()).rejects.toThrow(); + // obtainLocation navigates away; we return null so we don't fall through + // into the code-exchange path without a code. + await expect(YouVersionAPIUsers.handleAuthCallback()).resolves.toBeNull(); - // Verify that the redirect was attempted expect(mocks.window.location.href).toBe( 'https://api.youversion.com/auth/callback?state=test-state', ); }); + it('stashes granted_permissions from the pre-code OAuth hop', async () => { + mocks.window.location.href = + 'https://example.com/callback?state=test-state&granted_permissions=highlights'; + mocks.window.location.search = '?state=test-state&granted_permissions=highlights'; + mocks.localStorage.getItem.mockImplementation((key: string) => { + if (key === 'youversion-auth-state') return 'test-state'; + return null; + }); + + await expect(YouVersionAPIUsers.handleAuthCallback()).resolves.toBeNull(); + + expect(mocks.localStorage.setItem).toHaveBeenCalledWith( + 'youversion-auth-pending-granted-permissions', + JSON.stringify({ state: 'test-state', permissions: ['highlights'] }), + ); + expect(mocks.window.location.href).toBe( + 'https://api.youversion.com/auth/callback?state=test-state&granted_permissions=highlights', + ); + }); + it('should throw error when required parameters are missing', async () => { mocks.window.location.search = '?state=test-state&code=auth-code'; mocks.localStorage.getItem.mockImplementation((key: string) => { @@ -167,54 +318,18 @@ describe('YouVersionAPIUsers', () => { }); it('should successfully exchange code for tokens', async () => { - const mockTokens = { - access_token: 'access-token-123', - expires_in: 3600, - id_token: - 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJlbWFpbCI6ImpvaG5AZXhhbXBsZS5jb20iLCJwcm9maWxlX3BpY3R1cmUiOiJodHRwczovL2V4YW1wbGUuY29tL2F2YXRhci5qcGcifQ.invalid-signature', - refresh_token: 'refresh-token-456', + setupCallbackFlow({ scope: 'bibles highlights openid', - token_type: 'Bearer', - }; - - const mockResponse = { - ok: true, - status: 200, - statusText: 'OK', - text: vi.fn().mockResolvedValue(JSON.stringify(mockTokens)), - }; - - mocks.window.location.search = '?state=test-state&code=auth-code'; - mocks.window.location.href = 'https://example.com/callback?state=test-state&code=auth-code'; - - mocks.localStorage.getItem.mockImplementation((key: string) => { - switch (key) { - case 'youversion-auth-state': - return 'test-state'; - case 'youversion-auth-code-verifier': - return 'code-verifier-123'; - case 'youversion-auth-redirect-uri': - return 'https://example.com/callback'; - default: - return null; - } + profile: { ...DEFAULT_CALLBACK_PROFILE, profile_picture: 'https://example.com/avatar.jpg' }, }); - mockFetch.mockResolvedValue(mockResponse); - - // Mock atob for JWT decoding - vi.mocked(atob).mockReturnValue( - JSON.stringify({ - sub: '1234567890', - name: 'John Doe', - email: 'john@example.com', - profile_picture: 'https://example.com/avatar.jpg', - }), - ); - // Mock YouVersionPlatformConfiguration persistence const saveAuthDataSpy = vi.spyOn(YouVersionPlatformConfiguration, 'saveAuthData'); const saveUserInfoSpy = vi.spyOn(YouVersionPlatformConfiguration, 'saveUserInfo'); + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); const result = await YouVersionAPIUsers.handleAuthCallback(); @@ -240,12 +355,19 @@ describe('YouVersionAPIUsers', () => { avatar_url: 'https://example.com/avatar.jpg', }); + // Token scope seeds the permission cache (OIDC scopes filtered out) + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['bibles', 'highlights']); + saveUserInfoSpy.mockRestore(); + saveGrantedPermissionsSpy.mockRestore(); // Verify cleanup expect(mocks.localStorage.removeItem).toHaveBeenCalledWith('youversion-auth-code-verifier'); expect(mocks.localStorage.removeItem).toHaveBeenCalledWith('youversion-auth-redirect-uri'); expect(mocks.localStorage.removeItem).toHaveBeenCalledWith('youversion-auth-state'); + expect(mocks.localStorage.removeItem).toHaveBeenCalledWith( + 'youversion-auth-pending-granted-permissions', + ); expect(mocks.window.history.replaceState).toHaveBeenCalledWith( {}, @@ -256,6 +378,152 @@ describe('YouVersionAPIUsers', () => { saveAuthDataSpy.mockRestore(); }); + it('unions stashed early grants with token scope when final URL omits them', async () => { + setupCallbackFlow({ + scope: 'openid profile email', + pendingGrant: JSON.stringify({ state: 'test-state', permissions: ['highlights'] }), + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['highlights']); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('persists highlights when the final callback echoes granted_permissions[] (server bracket notation)', async () => { + // Models the real one-shot return: the hosted /auth/consent flow redirects + // straight to the app with the code AND the grant echo, and the server + // encodes that echo with bracket-array notation (as seen live on the + // outbound `requested_permissions[]`). The token scope does NOT carry the + // data-exchange `highlights` permission, so the URL echo is the only signal. + setupCallbackFlow({ + scope: 'profile openid', + search: '?state=test-state&code=auth-code&granted_permissions%5B%5D=highlights', + href: 'https://example.com/callback?state=test-state&code=auth-code&granted_permissions%5B%5D=highlights', + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['highlights']); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('stashes bracket-array granted_permissions[] from the pre-code OAuth hop', async () => { + mocks.window.location.href = + 'https://example.com/callback?state=test-state&granted_permissions%5B%5D=highlights'; + mocks.window.location.search = '?state=test-state&granted_permissions%5B%5D=highlights'; + mocks.localStorage.getItem.mockImplementation((key: string) => { + if (key === 'youversion-auth-state') return 'test-state'; + return null; + }); + + await expect(YouVersionAPIUsers.handleAuthCallback()).resolves.toBeNull(); + + expect(mocks.localStorage.setItem).toHaveBeenCalledWith( + 'youversion-auth-pending-granted-permissions', + JSON.stringify({ state: 'test-state', permissions: ['highlights'] }), + ); + }); + + it('discards stashed early grants bound to a different OAuth state', async () => { + setupCallbackFlow({ + scope: 'openid profile email', + pendingGrant: JSON.stringify({ state: 'other-flow-state', permissions: ['highlights'] }), + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).not.toHaveBeenCalled(); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('discards legacy unbound pre-code grant stash (plain comma list)', async () => { + setupCallbackFlow({ scope: 'openid profile email', pendingGrant: 'highlights' }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).not.toHaveBeenCalled(); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('seeds the requested permission when the server returns no grant echo (the live web-flow shape)', async () => { + // The exact captured failure (2026-07-23): signIn requested `highlights`, + // consent was granted, but the callback carries no `granted_permissions` + // and the token scope is only `profile openid`. Sources (1)-(3) are all + // empty; only the optimistic requested-permissions seed keeps `highlights`. + setupCallbackFlow({ + scope: 'profile openid', + requestedGrant: JSON.stringify({ state: 'test-state', permissions: ['highlights'] }), + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['highlights']); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('discards a requested-permissions stash bound to a different OAuth state (fail closed)', async () => { + setupCallbackFlow({ + scope: 'profile openid', + requestedGrant: JSON.stringify({ state: 'other-flow-state', permissions: ['highlights'] }), + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).not.toHaveBeenCalled(); + saveGrantedPermissionsSpy.mockRestore(); + }); + + it('unions the requested-permissions seed with a server echo without duplicating', async () => { + // If the server ever does echo the grant, the Set union must not double it. + setupCallbackFlow({ + scope: 'profile openid', + search: '?state=test-state&code=auth-code&granted_permissions=highlights', + href: 'https://example.com/callback?state=test-state&code=auth-code&granted_permissions=highlights', + requestedGrant: JSON.stringify({ state: 'test-state', permissions: ['highlights'] }), + }); + + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + await YouVersionAPIUsers.handleAuthCallback(); + + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['highlights']); + saveGrantedPermissionsSpy.mockRestore(); + }); + it('should handle token exchange failure', async () => { const mockResponse = { ok: false, @@ -288,6 +556,61 @@ describe('YouVersionAPIUsers', () => { expect(mocks.localStorage.removeItem).toHaveBeenCalledWith('youversion-auth-redirect-uri'); expect(mocks.localStorage.removeItem).toHaveBeenCalledWith('youversion-auth-state'); }); + + it('dedupes concurrent callbacks for the same code (one exchange, grants survive)', async () => { + setupCallbackFlow({ + scope: 'highlights openid', + profile: { ...DEFAULT_CALLBACK_PROFILE, profile_picture: 'https://example.com/avatar.jpg' }, + }); + + // A second token request for the same single-use code would 400 on the + // server. Model that: first fetch succeeds, any subsequent fetch fails. + let tokenRequestCount = 0; + mockFetch.mockImplementation(() => { + tokenRequestCount += 1; + if (tokenRequestCount === 1) { + return Promise.resolve({ + ok: true, + status: 200, + statusText: 'OK', + text: vi.fn().mockResolvedValue(JSON.stringify(makeTokens('highlights openid'))), + }); + } + return Promise.resolve({ ok: false, status: 400, statusText: 'Bad Request' }); + }); + + const clearAuthTokensSpy = vi.spyOn(YouVersionPlatformConfiguration, 'clearAuthTokens'); + const saveGrantedPermissionsSpy = vi.spyOn( + YouVersionPlatformConfiguration, + 'saveGrantedPermissions', + ); + + // Two concurrent invocations, as StrictMode's double-mount produces. + const [first, second] = await Promise.all([ + YouVersionAPIUsers.handleAuthCallback(), + YouVersionAPIUsers.handleAuthCallback(), + ]); + + // A repeat sequential call this page load must also reuse the exchange. + const third = await YouVersionAPIUsers.handleAuthCallback(); + + // Exactly one token request was made despite three callback invocations. + expect(tokenRequestCount).toBe(1); + expect(mockFetch).toHaveBeenCalledTimes(1); + + // All callers observe the same successful result. + expect(first).toBeTruthy(); + expect(first?.accessToken).toBe('access-token-123'); + expect(second).toBe(first); + expect(third).toBe(first); + + // The destructive catch path never ran, so the seeded grants survive. + expect(clearAuthTokensSpy).not.toHaveBeenCalled(); + expect(saveGrantedPermissionsSpy).toHaveBeenCalledWith(['highlights']); + + clearAuthTokensSpy.mockRestore(); + saveGrantedPermissionsSpy.mockRestore(); + }); }); describe('obtainLocation', () => { @@ -667,6 +990,9 @@ describe('YouVersionAPIUsers', () => { describe('refreshTokenIfNeeded', () => { beforeEach(() => { vi.clearAllMocks(); + // The in-flight refresh slot is module-scoped; clear it so a leftover + // promise from a prior case can't be shared into this one. + __resetTokenRefreshDedupeForTests(); }); it('should return true when token is not expired', async () => { @@ -736,5 +1062,102 @@ describe('YouVersionAPIUsers', () => { clearAuthTokensSpy.mockRestore(); }); + + it('shares one refresh across concurrent callers so a duplicate cannot wipe the session', async () => { + const pastDate = new Date(Date.now() - 10 * 60 * 1000); + mocks.localStorage.getItem.mockImplementation((key: string) => { + if (key === 'expiryDate') return pastDate.toISOString(); + if (key === 'refreshToken') return 'refresh-token-123'; + if (key === 'idToken') return 'id-token-123'; + return null; + }); + + // The refresh token is single-use: the first request succeeds; any second + // request for the same token 400s on the server. If both concurrent + // callers spent it, the loser's failure would clear the session. + let refreshRequestCount = 0; + mockFetch.mockImplementation(() => { + refreshRequestCount += 1; + if (refreshRequestCount === 1) { + return Promise.resolve({ + ok: true, + status: 200, + statusText: 'OK', + json: vi.fn().mockResolvedValue({ + access_token: 'new-access-token', + expires_in: 3600, + refresh_token: 'new-refresh-token', + scope: 'bibles highlights openid', + token_type: 'Bearer', + }), + }); + } + return Promise.resolve({ ok: false, status: 400, statusText: 'Bad Request' }); + }); + + const clearAuthTokensSpy = vi.spyOn(YouVersionPlatformConfiguration, 'clearAuthTokens'); + const saveAuthDataSpy = vi.spyOn(YouVersionPlatformConfiguration, 'saveAuthData'); + + // Two concurrent invocations, as StrictMode's double-mount produces. + const [first, second] = await Promise.all([ + YouVersionAPIUsers.refreshTokenIfNeeded(), + YouVersionAPIUsers.refreshTokenIfNeeded(), + ]); + + // Exactly one refresh network call despite two concurrent callers. + expect(refreshRequestCount).toBe(1); + expect(mockFetch).toHaveBeenCalledTimes(1); + + // Both callers resolve true off the single shared refresh. + expect(first).toBe(true); + expect(second).toBe(true); + + // The destructive clear never ran, so tokens and grants survive; the + // rotated tokens were persisted exactly once. + expect(clearAuthTokensSpy).not.toHaveBeenCalled(); + expect(saveAuthDataSpy).toHaveBeenCalledTimes(1); + expect(saveAuthDataSpy).toHaveBeenCalledWith( + 'new-access-token', + 'new-refresh-token', + expect.any(Date), + ); + + clearAuthTokensSpy.mockRestore(); + saveAuthDataSpy.mockRestore(); + }); + + it('clears the shared slot so a later refresh performs a new network call', async () => { + const pastDate = new Date(Date.now() - 10 * 60 * 1000); + mocks.localStorage.getItem.mockImplementation((key: string) => { + if (key === 'expiryDate') return pastDate.toISOString(); + if (key === 'refreshToken') return 'refresh-token-123'; + if (key === 'idToken') return 'id-token-123'; + return null; + }); + + mockFetch.mockResolvedValue({ + ok: true, + status: 200, + statusText: 'OK', + json: vi.fn().mockResolvedValue({ + access_token: 'new-access-token', + expires_in: 3600, + refresh_token: 'new-refresh-token', + scope: 'bibles highlights openid', + token_type: 'Bearer', + }), + }); + + // First refresh settles, clearing the in-flight slot. + const firstResult = await YouVersionAPIUsers.refreshTokenIfNeeded(); + expect(firstResult).toBe(true); + expect(mockFetch).toHaveBeenCalledTimes(1); + + // A later call (token still expired) is not merged into the settled + // refresh; it performs a fresh network call. + const secondResult = await YouVersionAPIUsers.refreshTokenIfNeeded(); + expect(secondResult).toBe(true); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); }); }); diff --git a/packages/core/src/__tests__/client.test.ts b/packages/core/src/__tests__/client.test.ts index 3d6090f6..451a3af8 100644 --- a/packages/core/src/__tests__/client.test.ts +++ b/packages/core/src/__tests__/client.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, beforeAll, afterEach, afterAll } from 'vitest'; -import { ApiClient } from '../client'; +import { ApiClient, getHttpStatus } from '../client'; import { http, HttpResponse } from 'msw'; import { server } from './setup'; @@ -244,3 +244,26 @@ describe('ApiClient', () => { }); }); }); + +describe('getHttpStatus', () => { + it('reads the status off an Error with a numeric status', () => { + const error = Object.assign(new Error('nope'), { status: 403 }); + expect(getHttpStatus(error)).toBe(403); + }); + + it('reads the status off a thrown plain object', () => { + expect(getHttpStatus({ status: 401 })).toBe(401); + }); + + it('returns undefined for objects without a numeric status', () => { + expect(getHttpStatus({ status: '500' })).toBeUndefined(); + expect(getHttpStatus(new Error('network'))).toBeUndefined(); + }); + + it('returns undefined for null and non-object values', () => { + expect(getHttpStatus(null)).toBeUndefined(); + expect(getHttpStatus(undefined)).toBeUndefined(); + expect(getHttpStatus(404)).toBeUndefined(); + expect(getHttpStatus('401')).toBeUndefined(); + }); +}); diff --git a/packages/core/src/__tests__/data-exchange.test.ts b/packages/core/src/__tests__/data-exchange.test.ts index 4ae0e357..6f2ed03a 100644 --- a/packages/core/src/__tests__/data-exchange.test.ts +++ b/packages/core/src/__tests__/data-exchange.test.ts @@ -7,10 +7,26 @@ import { parseDataExchangeCallback, handleDataExchangeCallback, } from '../data-exchange'; +import { YouVersionPlatformConfiguration } from '../YouVersionPlatformConfiguration'; import { server } from './setup'; const apiHost = process.env.YVP_API_HOST; +/** + * Stubs `window` with a location parsed from `href` plus a spyable + * `history.replaceState`. `localStorage` is left as the test polyfill so + * {@link YouVersionPlatformConfiguration} reads/writes real (mock) storage. + */ +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; +} + describe('DataExchangeClient.updateToken', () => { let client: DataExchangeClient; @@ -109,21 +125,20 @@ describe('parseDataExchangeCallback', () => { }); describe('handleDataExchangeCallback URL cleanup', () => { + beforeEach(() => { + // A signed-in user who is also the flow initiator, so a `granted` return is + // honored and these tests can focus on URL cleanup. + YouVersionPlatformConfiguration.clearAuthTokens(); + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a', name: 'A' }); + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); + }); + afterEach(() => { + YouVersionPlatformConfiguration.clearAuthTokens(); 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', @@ -159,3 +174,74 @@ describe('handleDataExchangeCallback URL cleanup', () => { expect(replaceState).not.toHaveBeenCalled(); }); }); + +describe('handleDataExchangeCallback grant safety (initiating user)', () => { + const GRANTED_URL = + 'https://app.example.com/read?data_exchange_status=granted&granted_permissions=highlights'; + + beforeEach(() => { + YouVersionPlatformConfiguration.clearAuthTokens(); + }); + + afterEach(() => { + YouVersionPlatformConfiguration.clearAuthTokens(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('saves the grant when the initiating user is still signed in on return', () => { + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a', name: 'A' }); + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); + + stubLocation(GRANTED_URL); + const result = handleDataExchangeCallback(); + + expect(result).toEqual({ status: 'granted', grantedPermissions: ['highlights'] }); + expect(YouVersionPlatformConfiguration.grantedPermissions).toContain('highlights'); + // Consumed on return so it cannot authorize a later, unrelated callback. + expect(YouVersionPlatformConfiguration.dataExchangeInitiator).toBeNull(); + }); + + it('discards the grant when a different user signed in mid-redirect', () => { + // User A initiates the flow... + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a', name: 'A' }); + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); + // ...then user B signs in on another tab before the redirect returns. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-b', name: 'B' }); + + const replaceState = stubLocation(GRANTED_URL); + const result = handleDataExchangeCallback(); + + // Grant is not honored; the result degrades to a failed exchange. + expect(result).toEqual({ status: 'failure', grantedPermissions: [] }); + + // Cache untouched for the signed-in user (B)... + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + // ...and for the initiating user (A) once they sign back in. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a', name: 'A' }); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + + // URL cleanup still happens on mismatch. + expect(replaceState).toHaveBeenCalledTimes(1); + const cleaned = new URL(replaceState.mock.calls[0]![2] as string); + expect(cleaned.searchParams.has('data_exchange_status')).toBe(false); + expect(cleaned.searchParams.has('granted_permissions')).toBe(false); + }); + + it('discards the grant when no initiator was recorded (legacy pending state)', () => { + // Signed-in user, but no initiator was ever stored for this return. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-a', name: 'A' }); + + const replaceState = stubLocation(GRANTED_URL); + const result = handleDataExchangeCallback(); + + expect(result).toEqual({ status: 'failure', grantedPermissions: [] }); + expect(YouVersionPlatformConfiguration.grantedPermissions).toEqual([]); + + // URL cleanup still happens. + expect(replaceState).toHaveBeenCalledTimes(1); + const cleaned = new URL(replaceState.mock.calls[0]![2] as string); + expect(cleaned.searchParams.has('data_exchange_status')).toBe(false); + expect(cleaned.searchParams.has('granted_permissions')).toBe(false); + }); +}); diff --git a/packages/core/src/__tests__/highlights.test.ts b/packages/core/src/__tests__/highlights.test.ts index 762720a6..34476d70 100644 --- a/packages/core/src/__tests__/highlights.test.ts +++ b/packages/core/src/__tests__/highlights.test.ts @@ -215,5 +215,20 @@ describe('HighlightsClient', () => { highlightsClient.deleteHighlight('MAT.1.1', { version_id: 0 }, 'token'), ).rejects.toThrow('Version ID must be a positive integer'); }); + + // Regression (YPE-1034 "vapor" flash): a successful DELETE that returns + // `200 application/json` with an EMPTY body must RESOLVE, not reject. + // `response.json()` throws "Unexpected end of JSON input" on an empty body; + // that rejection made the optimistic-removal overlay revert (settleWrite's + // failure path) and the removed highlight reappeared until the next refetch. + it('resolves when a successful DELETE returns 200 application/json with an empty body', async () => { + vi.spyOn(global, 'fetch').mockResolvedValue( + new Response('', { status: 200, headers: { 'content-type': 'application/json' } }), + ); + + await expect( + highlightsClient.deleteHighlight('MAT.1.1', { version_id: 111 }, 'token'), + ).resolves.toBeUndefined(); + }); }); }); diff --git a/packages/core/src/__tests__/permissions.test.ts b/packages/core/src/__tests__/permissions.test.ts index 52a5ed6b..a0dcf8d4 100644 --- a/packages/core/src/__tests__/permissions.test.ts +++ b/packages/core/src/__tests__/permissions.test.ts @@ -26,6 +26,42 @@ describe('parseGrantedPermissions', () => { it('returns [] when the param is absent', () => { expect(parseGrantedPermissions(new URLSearchParams('state=x'))).toEqual([]); }); + + // The auth server encodes permission lists with PHP/Rails bracket-array + // notation (observed live as `requested_permissions[]` on the consent + // redirect it builds). The grant echo on the return uses the same shape, and + // `URLSearchParams` treats it as a distinct key from bare + // `granted_permissions`. See parseGrantedPermissions for the full contract. + it('parses the bracket-array key the server emits (granted_permissions[])', () => { + const params = new URLSearchParams('granted_permissions%5B%5D=highlights'); + expect(parseGrantedPermissions(params)).toEqual(['highlights']); + }); + + it('unions repeated bracket-array entries', () => { + const params = new URLSearchParams( + 'granted_permissions%5B%5D=highlights&granted_permissions%5B%5D=votd', + ); + expect(parseGrantedPermissions(params)).toEqual(['highlights', 'votd']); + }); + + it('parses indexed bracket-array keys (granted_permissions[0])', () => { + const params = new URLSearchParams( + 'granted_permissions%5B0%5D=highlights&granted_permissions%5B1%5D=votd', + ); + expect(parseGrantedPermissions(params)).toEqual(['highlights', 'votd']); + }); + + it('unions bare and bracket-array forms together', () => { + const params = new URLSearchParams( + 'granted_permissions=highlights&granted_permissions%5B%5D=votd', + ); + expect(parseGrantedPermissions(params)).toEqual(['highlights', 'votd']); + }); + + it('ignores unrelated keys that merely start with granted_permissions', () => { + const params = new URLSearchParams('granted_permissions_extra=nope'); + expect(parseGrantedPermissions(params)).toEqual([]); + }); }); describe('YouVersionPlatformConfiguration permission cache', () => { diff --git a/packages/core/src/auth-token.ts b/packages/core/src/auth-token.ts new file mode 100644 index 00000000..6ed2b5de --- /dev/null +++ b/packages/core/src/auth-token.ts @@ -0,0 +1,29 @@ +import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; + +/** + * Resolves the auth token from an explicit `lat` or the ambient platform + * configuration. Shared by the service clients (highlights, data exchange) so + * they resolve the token identically. + * + * The implicit fallback reads from `YouVersionPlatformConfiguration`, which is + * backed by browser `localStorage`. In any environment without it (Node.js + * tests, SSR) there is intentionally no ambient token: server-side callers must + * pass `lat` explicitly rather than relying on this fallback. + * + * @param lat Optional explicit long access token. + * @param action Human-readable action for the error message tail (e.g. + * `'accessing highlights'`), rendered as + * `...sign in before .` + * @throws Error if no token is available. + */ +export function resolveAuthToken(lat: string | undefined, action: 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 ${action}.`); + } + return token; +} diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts index 3b6b3848..d9b9883b 100644 --- a/packages/core/src/client.ts +++ b/packages/core/src/client.ts @@ -6,6 +6,24 @@ type QueryParams = Record; type RequestData = Record; type RequestHeaders = Record; +/** + * Returns the HTTP status code attached to an error thrown by an API client, + * or undefined when the error did not come from an HTTP response (network + * failure, timeout, validation error). This is the supported way to branch on + * status codes; the error's internal shape is not part of the public API. + */ +export function getHttpStatus(error: unknown): number | undefined { + if ( + typeof error === 'object' && + error !== null && + 'status' in error && + typeof (error as { status: unknown }).status === 'number' + ) { + return (error as { status: number }).status; + } + return undefined; +} + /** * ApiClient is a lightweight HTTP client for interacting with the API using fetch. * It provides convenient methods for GET and POST requests with typed responses. @@ -113,8 +131,14 @@ export class ApiClient { const contentType = response.headers.get('content-type'); if (contentType?.includes('application/json')) { - const data = (await response.json()) as T; - return data; + // A successful (2xx) response can legitimately carry an EMPTY body even + // with a JSON content-type — most notably a DELETE that returns + // `200 application/json` with no payload. `response.json()` throws + // "Unexpected end of JSON input" on an empty body, which would surface a + // successful write as a failure (e.g. deleteHighlight rejecting on a + // real delete). Read as text first and treat an empty body as "no data". + const text = await response.text(); + return text ? (JSON.parse(text) as T) : (undefined as T); } else { const text = await response.text(); return text as unknown as T; diff --git a/packages/core/src/data-exchange.ts b/packages/core/src/data-exchange.ts index 9b7a816a..f7fd21e9 100644 --- a/packages/core/src/data-exchange.ts +++ b/packages/core/src/data-exchange.ts @@ -1,5 +1,6 @@ import type { ApiClient } from './client'; import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; +import { resolveAuthToken } from './auth-token'; import { parseGrantedPermissions } from './permissions'; import { DataExchangeTokenResponseSchema } from './schemas/data-exchange'; @@ -31,15 +32,7 @@ export class DataExchangeClient { * (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; + return resolveAuthToken(lat, 'requesting a data exchange'); } /** @@ -128,14 +121,42 @@ export function parseDataExchangeCallback(search: string): DataExchangeCallbackR * * Cleanup surgically removes only the data-exchange params, preserving any * unrelated app query params and the hash fragment. + * + * Grant safety: the just-in-time data-exchange flow is a fire-and-forget + * full-page redirect, so the user signed in when this callback loads is not + * guaranteed to be the one who started the flow (e.g. user B signs in on another + * tab mid-redirect). The grant is only saved when the initiator recorded at + * {@link YouVersionPlatformConfiguration.saveDataExchangeInitiator} still matches + * the current user. Any mismatch — a different user, or a missing initiator (a + * legacy pending state or a return we did not originate) — fails closed: the + * grant is discarded and the result is downgraded to a `failure` so callers + * re-prompt instead of proceeding as granted. Dropping a legitimate grant is + * harmless — it just re-prompts — whereas saving one for the wrong user is not. + * + * The sign-in callback (see `Users.ts`) needs no such check: it derives the user + * profile from the same ID token that carries `granted_permissions`, so the + * grant is inherently bound to the correct user. */ 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 parsed = parseDataExchangeCallback(window.location.search); + if (!parsed) return null; + + // Read and clear the initiator up front so a stale value can never authorize a + // later, unrelated return. + const initiator = YouVersionPlatformConfiguration.dataExchangeInitiator; + YouVersionPlatformConfiguration.clearDataExchangeInitiator(); + + let result = parsed; + if (parsed.status === 'granted' && parsed.grantedPermissions.length > 0) { + const currentUserId = YouVersionPlatformConfiguration.storedUserInfo?.id ?? null; + if (initiator && currentUserId && initiator === currentUserId) { + YouVersionPlatformConfiguration.saveGrantedPermissions(parsed.grantedPermissions); + } else { + // Initiator missing or a different user is signed in: discard the grant + // and degrade to a failed exchange. + result = { status: 'failure', grantedPermissions: [] }; + } } const cleanUrl = new URL(window.location.href); diff --git a/packages/core/src/highlights.ts b/packages/core/src/highlights.ts index c56a29b9..a9459354 100644 --- a/packages/core/src/highlights.ts +++ b/packages/core/src/highlights.ts @@ -1,7 +1,7 @@ import { z } from 'zod'; import type { ApiClient } from './client'; import type { Collection, Highlight, CreateHighlight } from './types'; -import { YouVersionPlatformConfiguration } from './YouVersionPlatformConfiguration'; +import { resolveAuthToken } from './auth-token'; import { HighlightCollectionWireSchema, HighlightWireSchema, @@ -57,21 +57,7 @@ export class HighlightsClient { * @throws Error if no token is available. */ private getAuthToken(lat?: string): string { - if (lat) { - return lat; - } - // The implicit token fallback reads from `YouVersionPlatformConfiguration`, - // which is backed by browser `localStorage`. In any environment without it - // (Node.js tests, SSR) there is intentionally no ambient token: server-side - // callers must pass `lat` explicitly rather than relying on this fallback. - const token = - typeof localStorage === 'undefined' ? null : YouVersionPlatformConfiguration.accessToken; - if (!token) { - throw new Error( - 'Authentication required. Please provide a token or sign in before accessing highlights.', - ); - } - return token; + return resolveAuthToken(lat, 'accessing highlights'); } private authHeaders(lat?: string): Record { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0979c693..15359732 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,4 +1,4 @@ -export { ApiClient } from './client'; +export { ApiClient, getHttpStatus } from './client'; export { BibleClient } from './bible'; export { LanguagesClient, type GetLanguagesOptions } from './languages'; export { OrganizationsClient } from './organizations'; diff --git a/packages/core/src/permissions.ts b/packages/core/src/permissions.ts index e4304bda..3d78b83b 100644 --- a/packages/core/src/permissions.ts +++ b/packages/core/src/permissions.ts @@ -14,10 +14,38 @@ * 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. + * + * The YouVersion auth server encodes the request/response permission lists using + * PHP/Rails-style bracket-array notation (observed live as `requested_permissions[]` + * in the hosted consent redirect it builds). `URLSearchParams` treats + * `granted_permissions[]` (and indexed `granted_permissions[0]`) as keys distinct + * from bare `granted_permissions`, so we accept every `granted_permissions`, + * `granted_permissions[]`, and `granted_permissions[]` key. This keeps the + * reader symmetric with what the server emits; a plain `granted_permissions` still + * works unchanged. */ export function parseGrantedPermissions(params: URLSearchParams): string[] { + const values: string[] = []; + for (const [key, value] of params) { + if (/^granted_permissions(\[\d*\])?$/.test(key)) { + values.push(value); + } + } + return mergePermissionValues(values); +} + +/** + * Splits a permission list string the way Swift's `permissions(from:)` does + * (comma or whitespace). Used for token `scope` and stashed grant lists. + */ +export function parsePermissionList(value: string | null | undefined): string[] { + if (!value) return []; + return mergePermissionValues([value]); +} + +function mergePermissionValues(values: string[]): string[] { const seen = new Set(); - for (const value of params.getAll('granted_permissions')) { + for (const value of values) { for (const part of value.split(/[,\s]+/)) { if (part) seen.add(part); } diff --git a/packages/core/src/styles/bible-reader.css b/packages/core/src/styles/bible-reader.css index 9c34a632..abeb637d 100644 --- a/packages/core/src/styles/bible-reader.css +++ b/packages/core/src/styles/bible-reader.css @@ -126,8 +126,21 @@ /* 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. */ + underline (text-decoration) and without any layout shift. + + The rounded-corner + padding + box-decoration-break styles are deliberately + STATIC (applied to every `.yv-v`, highlighted or not). Keeping them off the + highlight state means applying/removing a fill only changes + `background-color` — the 2px inline padding never appears or disappears, so + text never reflows and the 250ms fade stays smooth. `box-decoration-break: + clone` gives each wrapped line fragment its own rounded ends and padding + instead of a single hard cut at the wrap. This is a web-native divergence + from the Swift SDK's square fills (user-approved 2026-07-22). */ & .yv-v { + border-radius: 4px; + padding-inline: 2px; + box-decoration-break: clone; + -webkit-box-decoration-break: clone; transition: background-color 250ms ease; } diff --git a/packages/hooks/src/context/YouVersionAuthProvider.test.tsx b/packages/hooks/src/context/YouVersionAuthProvider.test.tsx index d02d50c2..38e70951 100644 --- a/packages/hooks/src/context/YouVersionAuthProvider.test.tsx +++ b/packages/hooks/src/context/YouVersionAuthProvider.test.tsx @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { StrictMode } from 'react'; import { render } from '@testing-library/react'; import { YouVersionAPIUsers, YouVersionPlatformConfiguration } from '@youversion/platform-core'; import YouVersionAuthProvider from './YouVersionAuthProvider'; @@ -162,6 +163,35 @@ describe('YouVersionAuthProvider', () => { }); }); + it('tolerates the StrictMode double-invocation without a spurious error', async () => { + // The init effect has no re-entrancy guard, so under StrictMode it runs + // twice and calls handleAuthCallback twice. The real fix is the core-layer + // dedupe (see packages/core Users.test.ts); here we only assert the + // provider effect tolerates the double-invocation and still resolves to an + // authenticated, error-free state. + mockWindow.location.search = '?state=test-state&code=auth-code'; + vi.spyOn(YouVersionAPIUsers, 'getStoredUserInfo').mockReturnValue(mockUserInfo); + vi.spyOn(YouVersionAPIUsers, 'handleAuthCallback').mockResolvedValue(mockAuthResult); + + const { getByTestId } = render( + + + + + , + ); + + await vi.waitFor(() => { + expect(getByTestId('is-loading')).toHaveTextContent('false'); + }); + + // The effect double-invoked (this is the condition the bug depended on). + expect(vi.mocked(YouVersionAPIUsers).handleAuthCallback).toHaveBeenCalledTimes(2); + // Final state is authenticated with no error surfaced. + expect(getByTestId('user-info')).toHaveTextContent(JSON.stringify(mockUserInfo)); + expect(getByTestId('error')).toHaveTextContent('null'); + }); + it('should leave user null when no profile was stored during callback', async () => { mockWindow.location.search = '?state=test-state&code=auth-code'; vi.spyOn(YouVersionAPIUsers, 'handleAuthCallback').mockResolvedValue(mockAuthResult); diff --git a/packages/hooks/src/context/YouVersionProvider.tsx b/packages/hooks/src/context/YouVersionProvider.tsx index cf54a94d..af9af471 100644 --- a/packages/hooks/src/context/YouVersionProvider.tsx +++ b/packages/hooks/src/context/YouVersionProvider.tsx @@ -13,6 +13,20 @@ interface YouVersionProviderPropsBase { appKey: string; apiHost?: string; theme?: 'light' | 'dark' | 'system'; + /** + * Integrator display name for the sign-in dialog body copy. Synced onto + * `YouVersionPlatformConfiguration.appName`. The UI package also mirrors this + * onto its bundled core copy (tsup `noExternal`), so pass it via + * `YouVersionProvider` props — do not set the config from a separate + * `@youversion/platform-core` import when consuming `@youversion/platform-react-ui`. + */ + appName?: string; + /** + * Optional pitch line for the sign-in dialog. Synced onto + * `YouVersionPlatformConfiguration.signInPromptMessage` (and mirrored by the + * UI provider onto its bundled core copy — same dual-instance caveat as `appName`). + */ + signInPromptMessage?: string; /** * Extra HTTP headers to add to every API call made through hooks created by * this provider. Values here override the SDK's built-in headers when keys @@ -99,6 +113,8 @@ function YouVersionProviderInner( includeAuth, theme = 'light', additionalHeaders, + appName, + signInPromptMessage, children, } = props; @@ -124,7 +140,9 @@ function YouVersionProviderInner( useEffect(() => { YouVersionPlatformConfiguration.appKey = appKey; YouVersionPlatformConfiguration.apiHost = apiHost; - }, [appKey, apiHost]); + YouVersionPlatformConfiguration.appName = appName; + YouVersionPlatformConfiguration.signInPromptMessage = signInPromptMessage; + }, [appKey, apiHost, appName, signInPromptMessage]); const contextValue = { appKey, diff --git a/packages/hooks/src/internal/useApiClient.ts b/packages/hooks/src/internal/useApiClient.ts new file mode 100644 index 00000000..03f4efcf --- /dev/null +++ b/packages/hooks/src/internal/useApiClient.ts @@ -0,0 +1,44 @@ +'use client'; + +import { useContext, useMemo } from 'react'; +import { ApiClient } from '@youversion/platform-core'; +import { YouVersionContext } from '../context'; + +/** + * Reads {@link YouVersionContext} and returns a memoized {@link ApiClient} built + * from its config. Shared by the per-service client hooks so the client + * construction (and its dependency array) lives in one place. + * + * By default it throws when no app key is configured (the contract every data + * hook relies on). Pass `{ optional: true }` for the no-provider-tolerant + * variant that returns `null` instead — used by `useHighlightAuthActions`, + * which must not throw when no provider is mounted. + */ +export function useApiClient(options: { optional: true }): ApiClient | null; +export function useApiClient(options?: { optional?: false }): ApiClient; +export function useApiClient(options: { optional?: boolean } = {}): ApiClient | null { + const context = useContext(YouVersionContext); + const optional = options.optional ?? false; + + return useMemo(() => { + if (!context?.appKey) { + if (optional) return null; + throw new Error( + 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', + ); + } + + return new ApiClient({ + appKey: context.appKey, + apiHost: context.apiHost, + installationId: context.installationId, + additionalHeaders: context.additionalHeaders, + }); + }, [ + context?.appKey, + context?.apiHost, + context?.installationId, + context?.additionalHeaders, + optional, + ]); +} diff --git a/packages/hooks/src/useApiData.test.tsx b/packages/hooks/src/useApiData.test.tsx index cb21a189..a8dab070 100644 --- a/packages/hooks/src/useApiData.test.tsx +++ b/packages/hooks/src/useApiData.test.tsx @@ -138,6 +138,47 @@ describe('useApiData — existing behavior', () => { expect(result.current.data).toBeNull(); }); + it('keeps a stable refetch identity across re-renders with an inline fetchFn', async () => { + // Callers pass a fresh inline arrow every render; refetch must not churn. + const { result, rerender } = renderHook( + ({ scope }: { scope: string }) => useApiData(() => Promise.resolve(scope), ['stable-dep']), + { initialProps: { scope: 'a' } }, + ); + + await waitFor(() => { + expect(result.current.data).toBe('a'); + }); + + const firstRefetch = result.current.refetch; + rerender({ scope: 'b' }); + rerender({ scope: 'c' }); + + expect(result.current.refetch).toBe(firstRefetch); + }); + + it('refetch uses the latest inline fetchFn after re-renders', async () => { + // The ref must hold the newest closure so a refetch reflects current props. + const { result, rerender } = renderHook( + ({ scope }: { scope: string }) => useApiData(() => Promise.resolve(scope), ['stable-dep']), + { initialProps: { scope: 'a' } }, + ); + + await waitFor(() => { + expect(result.current.data).toBe('a'); + }); + + // Deps unchanged, so no effect-driven refetch; only the closure updates. + rerender({ scope: 'b' }); + + act(() => { + result.current.refetch(); + }); + + await waitFor(() => { + expect(result.current.data).toBe('b'); + }); + }); + it('refetches on demand', async () => { const fetchFn = vi.fn().mockResolvedValueOnce('first').mockResolvedValueOnce('second'); diff --git a/packages/hooks/src/useApiData.ts b/packages/hooks/src/useApiData.ts index 63624e70..1969668c 100644 --- a/packages/hooks/src/useApiData.ts +++ b/packages/hooks/src/useApiData.ts @@ -30,6 +30,15 @@ export function useApiData( // previous chapter) resolving after a newer fetch must not overwrite it. const requestSeqRef = useRef(0); + // Hold the (typically inline) `fetchFn` in a ref, refreshed every render, so + // it can stay out of `fetchData`'s deps below. Otherwise a new closure each + // render would churn `fetchData` — and therefore `refetch` — identity every + // render. `fetchData` reads the ref only when a fetch actually starts, and + // the ref is updated during render before any effect runs, so a dep-change + // fetch still uses the latest `fetchFn`. + const fetchFnRef = useRef(fetchFn); + fetchFnRef.current = fetchFn; + const fetchData = useCallback(() => { const requestSeq = ++requestSeqRef.current; @@ -47,7 +56,8 @@ export function useApiData( setLoading(true); setError(null); - fetchFn() + fetchFnRef + .current() .then((result) => { if (requestSeq === requestSeqRef.current) { setData(result); @@ -63,7 +73,7 @@ export function useApiData( setLoading(false); } }); - }, [fetchFn, enabled]); + }, [enabled]); const refetch = useCallback(() => { fetchData(); diff --git a/packages/hooks/src/useBibleClient.ts b/packages/hooks/src/useBibleClient.ts index 1673f47f..3dbd064b 100644 --- a/packages/hooks/src/useBibleClient.ts +++ b/packages/hooks/src/useBibleClient.ts @@ -1,26 +1,10 @@ 'use client'; -import { useContext, useMemo } from 'react'; -import { YouVersionContext } from './context'; -import { BibleClient, ApiClient } from '@youversion/platform-core'; +import { useMemo } from 'react'; +import { BibleClient } from '@youversion/platform-core'; +import { useApiClient } from './internal/useApiClient'; export function useBibleClient(): BibleClient { - const context = useContext(YouVersionContext); - - return useMemo(() => { - if (!context?.appKey) { - throw new Error( - 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', - ); - } - - return new BibleClient( - new ApiClient({ - appKey: context.appKey, - apiHost: context.apiHost, - installationId: context.installationId, - additionalHeaders: context.additionalHeaders, - }), - ); - }, [context?.apiHost, context?.appKey, context?.installationId, context?.additionalHeaders]); + const apiClient = useApiClient(); + return useMemo(() => new BibleClient(apiClient), [apiClient]); } diff --git a/packages/hooks/src/useHighlightAuthActions.test.tsx b/packages/hooks/src/useHighlightAuthActions.test.tsx index 00b683d2..7e51e9f7 100644 --- a/packages/hooks/src/useHighlightAuthActions.test.tsx +++ b/packages/hooks/src/useHighlightAuthActions.test.tsx @@ -82,4 +82,36 @@ describe('useHighlightAuthActions', () => { expect(window.location.href).toContain('token=dx-token'); expect(window.location.href).toContain('app_key=app-1'); }); + + it('records the data-exchange initiator before awaiting the token mint', async () => { + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1' }); + let resolveToken!: (value: string) => void; + const tokenGate = new Promise((resolve) => { + resolveToken = resolve; + }); + vi.spyOn(DataExchangeClient.prototype, 'updateToken').mockReturnValue(tokenGate); + + const { result } = renderHook(() => useHighlightAuthActions(), { wrapper }); + const pending = result.current.startDataExchangeForHighlights(); + + // Must be stamped before the mint resolves — otherwise a mid-await user + // switch could bind the grant to the wrong session. + expect(YouVersionPlatformConfiguration.dataExchangeInitiator).toBe('user-1'); + + resolveToken('dx-token'); + await pending; + expect(window.location.href).toContain('token=dx-token'); + }); + + it('clears the data-exchange initiator when token mint fails', async () => { + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1' }); + vi.spyOn(DataExchangeClient.prototype, 'updateToken').mockRejectedValue( + new Error('mint failed'), + ); + + const { result } = renderHook(() => useHighlightAuthActions(), { wrapper }); + + await expect(result.current.startDataExchangeForHighlights()).rejects.toThrow('mint failed'); + expect(YouVersionPlatformConfiguration.dataExchangeInitiator).toBeNull(); + }); }); diff --git a/packages/hooks/src/useHighlightAuthActions.ts b/packages/hooks/src/useHighlightAuthActions.ts index 79f846bc..0ef62ac6 100644 --- a/packages/hooks/src/useHighlightAuthActions.ts +++ b/packages/hooks/src/useHighlightAuthActions.ts @@ -2,7 +2,6 @@ import { useCallback, useContext, useMemo } from 'react'; import { - ApiClient, DataExchangeClient, buildDataExchangeUrl, handleDataExchangeCallback, @@ -13,6 +12,7 @@ import { } from '@youversion/platform-core'; import { YouVersionContext } from './context'; import { YouVersionAuthContext } from './context/YouVersionAuthContext'; +import { useApiClient } from './internal/useApiClient'; const HIGHLIGHTS_PERMISSION = SignInWithYouVersionPermission.highlights; @@ -65,17 +65,11 @@ export function useHighlightAuthActions(): { 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 apiClient = useApiClient({ optional: true }); + const dataExchangeClient = useMemo( + () => (apiClient ? new DataExchangeClient(apiClient) : null), + [apiClient], + ); const startSignInForHighlights = useCallback( async (redirectUrl?: string) => { @@ -95,9 +89,21 @@ export function useHighlightAuthActions(): { 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); + // Record the initiator BEFORE minting the token. A mid-await user switch + // (another tab) must not stamp the new session as the initiator — that + // would let the callback honor a grant the new user never consented to. + // Saving first fails closed on mismatch instead. + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); + try { + const token = await dataExchangeClient.updateToken([HIGHLIGHTS_PERMISSION]); + if (typeof window !== 'undefined') { + window.location.href = buildDataExchangeUrl(token, context.appKey, context.apiHost); + } + } catch (error) { + // Mint/redirect aborted — drop the initiator so a later unrelated + // `granted` return cannot ride this abandoned attempt. + YouVersionPlatformConfiguration.clearDataExchangeInitiator(); + throw error; } }, [dataExchangeClient, context?.appKey, context?.apiHost]); diff --git a/packages/hooks/src/useHighlights.ts b/packages/hooks/src/useHighlights.ts index 2dc62819..97539c4c 100644 --- a/packages/hooks/src/useHighlights.ts +++ b/packages/hooks/src/useHighlights.ts @@ -1,9 +1,8 @@ 'use client'; import { useMemo, useCallback } from 'react'; -import { useContext } from 'react'; -import { YouVersionContext } from './context'; -import { HighlightsClient, ApiClient } from '@youversion/platform-core'; +import { HighlightsClient } from '@youversion/platform-core'; +import { useApiClient } from './internal/useApiClient'; import { useApiData, type UseApiDataOptions } from './useApiData'; import { type GetHighlightsOptions, @@ -34,23 +33,9 @@ export function useHighlights( */ deleteHighlight: (passageId: string, deleteOptions: DeleteHighlightOptions) => Promise; } { - const context = useContext(YouVersionContext); + const apiClient = useApiClient(); - const highlightsClient = useMemo(() => { - if (!context?.appKey) { - throw new Error( - 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', - ); - } - return new HighlightsClient( - new ApiClient({ - appKey: context.appKey, - apiHost: context.apiHost, - installationId: context.installationId, - additionalHeaders: context.additionalHeaders, - }), - ); - }, [context?.apiHost, context?.appKey, context?.installationId, context?.additionalHeaders]); + const highlightsClient = useMemo(() => new HighlightsClient(apiClient), [apiClient]); // The dep array keys on the primitive fields of `options` rather than the // object reference, so an inline `{ version_id, passage_id }` literal doesn't diff --git a/packages/hooks/src/useLanguageClient.ts b/packages/hooks/src/useLanguageClient.ts index 4fa6bd25..15e9ccf5 100644 --- a/packages/hooks/src/useLanguageClient.ts +++ b/packages/hooks/src/useLanguageClient.ts @@ -1,26 +1,10 @@ 'use client'; -import { useContext, useMemo } from 'react'; -import { YouVersionContext } from './context'; -import { ApiClient, LanguagesClient } from '@youversion/platform-core'; +import { useMemo } from 'react'; +import { LanguagesClient } from '@youversion/platform-core'; +import { useApiClient } from './internal/useApiClient'; export function useLanguagesClient(): LanguagesClient { - const context = useContext(YouVersionContext); - - return useMemo(() => { - if (!context?.appKey) { - throw new Error( - 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', - ); - } - - return new LanguagesClient( - new ApiClient({ - appKey: context.appKey, - apiHost: context.apiHost, - installationId: context.installationId, - additionalHeaders: context.additionalHeaders, - }), - ); - }, [context?.apiHost, context?.appKey, context?.installationId, context?.additionalHeaders]); + const apiClient = useApiClient(); + return useMemo(() => new LanguagesClient(apiClient), [apiClient]); } diff --git a/packages/hooks/src/useOrganizationsClient.ts b/packages/hooks/src/useOrganizationsClient.ts index d1224d4f..2b642de9 100644 --- a/packages/hooks/src/useOrganizationsClient.ts +++ b/packages/hooks/src/useOrganizationsClient.ts @@ -1,26 +1,10 @@ 'use client'; -import { useContext, useMemo } from 'react'; -import { YouVersionContext } from './context'; -import { ApiClient, OrganizationsClient } from '@youversion/platform-core'; +import { useMemo } from 'react'; +import { OrganizationsClient } from '@youversion/platform-core'; +import { useApiClient } from './internal/useApiClient'; export function useOrganizationsClient(): OrganizationsClient { - const context = useContext(YouVersionContext); - - return useMemo(() => { - if (!context?.appKey) { - throw new Error( - 'YouVersion context not found. Make sure your component is wrapped with YouVersionProvider and an API key is provided.', - ); - } - - return new OrganizationsClient( - new ApiClient({ - appKey: context.appKey, - apiHost: context.apiHost, - installationId: context.installationId, - additionalHeaders: context.additionalHeaders, - }), - ); - }, [context?.apiHost, context?.appKey, context?.installationId, context?.additionalHeaders]); + const apiClient = useApiClient(); + return useMemo(() => new OrganizationsClient(apiClient), [apiClient]); } diff --git a/packages/ui/package.json b/packages/ui/package.json index 2577c163..a66cc07b 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -62,7 +62,7 @@ "react-i18next": "^17.0.0", "tailwind-merge": "3.3.1", "tw-animate-css": "1.4.0", - "xstate": "5.32.5" + "xstate": "5.32.4" }, "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 1ae3bb5d..c80a2579 100644 --- a/packages/ui/src/components/YouVersionAuthButton.tsx +++ b/packages/ui/src/components/YouVersionAuthButton.tsx @@ -20,7 +20,7 @@ interface SignInAuthProps { scopes?: AuthenticationScopes[]; /** * YouVersion data-exchange permissions to request at sign-in (e.g. `highlights`). - * These are distinct from OIDC `scopes` and are sent as `requested_permissions[]`. + * These are distinct from OIDC `scopes` and are sent as `requested_permissions`. */ permissions?: SignInWithYouVersionPermissionValues[]; } diff --git a/packages/ui/src/components/YouVersionProvider.test.tsx b/packages/ui/src/components/YouVersionProvider.test.tsx index 64850208..835c72c7 100644 --- a/packages/ui/src/components/YouVersionProvider.test.tsx +++ b/packages/ui/src/components/YouVersionProvider.test.tsx @@ -4,6 +4,7 @@ import { describe, it, expect, vi } from 'vitest'; import { render, screen } from '@testing-library/react'; import React from 'react'; +import { YouVersionPlatformConfiguration } from '@youversion/platform-core'; import { YouVersionProvider } from '@/components/YouVersionProvider'; const baseProviderMock = @@ -46,6 +47,26 @@ describe('UI YouVersionProvider', () => { expect(lastCall?.additionalHeaders).toBeUndefined(); }); + it('mirrors appName and signInPromptMessage onto the UI-bundled config', () => { + YouVersionPlatformConfiguration.appName = undefined; + YouVersionPlatformConfiguration.signInPromptMessage = undefined; + + render( + +
hello
+ , + ); + + expect(YouVersionPlatformConfiguration.appName).toBe('SDK Demo'); + expect(YouVersionPlatformConfiguration.signInPromptMessage).toBe( + 'Save your highlights to your YouVersion account.', + ); + }); + it.each([ ['undefined', undefined], ['empty string', ''], diff --git a/packages/ui/src/components/YouVersionProvider.tsx b/packages/ui/src/components/YouVersionProvider.tsx index b3fc6cb7..e5b4a691 100644 --- a/packages/ui/src/components/YouVersionProvider.tsx +++ b/packages/ui/src/components/YouVersionProvider.tsx @@ -1,4 +1,5 @@ import React, { type ComponentProps, useEffect } from 'react'; +import { YouVersionPlatformConfiguration } from '@youversion/platform-core'; import { YouVersionProvider as BaseYouVersionProvider } from '@youversion/platform-react-hooks'; import { syncBrowserLanguageFromNavigator } from '@/i18n'; import { YvStyles } from '@/lib/yv-styles'; @@ -17,6 +18,14 @@ export function YouVersionProvider( syncBrowserLanguageFromNavigator(); }, []); + // UI tsup inlines `@youversion/platform-core`, so this singleton is a different + // copy from the one hooks syncs. BibleReader reads appName / signInPromptMessage + // from *this* copy — keep it in sync with the provider props. + useEffect(() => { + YouVersionPlatformConfiguration.appName = props.appName; + YouVersionPlatformConfiguration.signInPromptMessage = props.signInPromptMessage; + }, [props.appName, props.signInPromptMessage]); + // Guard against a missing/empty app key here (rather than letting the base // provider throw) so consumers of the UI package see a styled message instead // of a blank page. The visible panel is intentionally generic; the actionable diff --git a/packages/ui/src/components/bible-reader-controlled.test.tsx b/packages/ui/src/components/bible-reader-controlled.test.tsx index c49711c3..6ac8fdc7 100644 --- a/packages/ui/src/components/bible-reader-controlled.test.tsx +++ b/packages/ui/src/components/bible-reader-controlled.test.tsx @@ -201,12 +201,16 @@ function selectVerse(container: HTMLElement, verse: number) { fireEvent.click(getVerseEl(container, verse)); } -/** rgba() string the reader paints for a highlight fill (0.35 alpha). */ +/** + * Color the reader paints for a highlight fill. These tests run in light mode + * (theme mocked to 'light'), where the fill is full opacity (Swift parity); + * jsdom serializes a fully opaque color to `rgb(...)`. + */ function fillFor(hex: string): string { const r = parseInt(hex.slice(0, 2), 16); const g = parseInt(hex.slice(2, 4), 16); const b = parseInt(hex.slice(4, 6), 16); - return `rgba(${r}, ${g}, ${b}, 0.35)`; + return `rgb(${r}, ${g}, ${b})`; } function getApplyButtons() { diff --git a/packages/ui/src/components/bible-reader-highlights-machine.ts b/packages/ui/src/components/bible-reader-highlights-machine.ts index 1901b00d..8b8143d5 100644 --- a/packages/ui/src/components/bible-reader-highlights-machine.ts +++ b/packages/ui/src/components/bible-reader-highlights-machine.ts @@ -52,6 +52,7 @@ import { stashPendingHighlight, type PendingHighlight, } from '@/lib/pending-highlight'; +import { getHttpStatus } from '@youversion/platform-core'; import { Result } from 'better-result'; import { assign, enqueueActions, fromPromise, setup, type DoneActorEvent } from 'xstate'; @@ -83,12 +84,6 @@ type WriteOp = { 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 = { @@ -192,14 +187,14 @@ function versesInRun(run: VerseRun): number[] { return verses; } -/** Pulls an HTTP status off a thrown ApiClient error (possibly wrapped). */ +/** + * Pulls an HTTP status off a thrown ApiClient error (possibly wrapped in our own + * `BibleReaderHighlightError`). Defers to core's `getHttpStatus` for the actual + * status contract so the error shape stays owned by the client layer. + */ 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; + if (error instanceof BibleReaderHighlightError) return getHttpStatus(error.cause); + return getHttpStatus(error); } /** @@ -365,6 +360,7 @@ export const bibleReaderHighlightsMachine = setup({ scopeIsDifferent: ({ context, event }) => event.type === 'SCOPE_CHANGED' && !scopesEqual(context.scope, event.scope), queueHasWork: ({ context }) => context.queue.length > 0, + signedOut: ({ context }) => !context.isAuthenticated, // ── TAP_COLOR fork ── tapInert: ({ event }) => event.type === 'TAP_COLOR' && event.verses.length === 0, @@ -489,7 +485,6 @@ export const bibleReaderHighlightsMachine = setup({ scope: context.scope, token, paint: true, - reprompt: true, }; enqueue.raise({ type: 'ENQUEUE', op }); }), @@ -531,7 +526,6 @@ export const bibleReaderHighlightsMachine = setup({ scope: context.scope, token, paint: true, - reprompt: false, }; enqueue.raise({ type: 'ENQUEUE', op }); }), @@ -560,6 +554,14 @@ export const bibleReaderHighlightsMachine = setup({ claimVerses(current, pending.verses, token, pending.color), ); } + // Resume-applied writes route through the SAME failure handling as a + // user-initiated apply: a 401/403 re-stashes the pending highlight (from + // the op's own scope, so a cross-scope return still re-prompts on the + // right passage) and re-opens the permission dialog, rather than silently + // dropping the user's original color tap. Being an `apply` is what earns + // that re-prompt at the consumer site (see `settleWrite`). + // The clear above already dropped the intent for network/5xx — settle + // must not clear again (sibling permission-lost entries may be present). const op: WriteOp = { kind: 'apply', color: pending.color, @@ -567,10 +569,6 @@ export const bibleReaderHighlightsMachine = setup({ 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 }); } @@ -588,7 +586,7 @@ export const bibleReaderHighlightsMachine = setup({ * 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). + * network/5xx → revert overlay only). */ settleWrite: enqueueActions(({ enqueue, event }) => { // Wired only to the processWrite `onDone`; the done event is the actor's @@ -636,23 +634,27 @@ export const bibleReaderHighlightsMachine = setup({ enqueue(({ context: current }) => current.services.current.invalidateHighlightsPermission(), ); - if (op.kind === 'apply' && op.reprompt) { + // Only an apply keeps the pending highlight + re-prompts: a user apply + // and a resume-applied pending both re-stash + re-open the permission + // dialog so the original tap is never silently lost. Removes deliberately + // don't re-prompt (fixing the old deferred wart): with no pending + // highlight, a re-prompt's post-grant resume was a no-op — invalidate the + // cache only, and the next apply re-enters the flow. + if (op.kind === 'apply') { // Keep this highlight pending and re-prompt the just-in-time dialog. // Append (not replace): a sibling batch that already lost permission may // hold a different color/verses, and both intents must survive the grant. enqueue(() => appendPendingHighlight(opToPending(op))); enqueue.raise({ type: 'PERMISSION_LOST' }); } - // Remove failures invalidate the cache but never re-prompt (the old - // deferred wart is fixed here): with no pending highlight, a re-prompt's - // post-grant resume was a no-op. The next apply re-enters the flow. } // Network / 5xx must NOT clear the stash: a user apply that reaches the // queue via `startApplyWrite` never stashed anything of its own, so there // is nothing here to drop — and any entries present belong to a sibling // batch that lost permission moments earlier and must survive the grant. - // (Tap-flow stashes can't linger past this point either: they are followed - // by a full-page redirect, and dialog cancel/decline run `clearPending`.) + // Resume-applied writes already consumed their pending at + // `applyPendingHighlight` enqueue time (intentional 5xx intent-drop), so + // a blunt clear here would only risk wiping those siblings. }), // ── Dialog side effects (fire-and-forget redirects, matching the hook) ── @@ -762,6 +764,14 @@ export const bibleReaderHighlightsMachine = setup({ }, }, permissionDialog: { + // The host can sign the user out while this dialog is open. A + // confirm would then start a data exchange that rejects + // unauthenticated, so route back to idle and drop the pending + // intent as soon as the sign-out lands (assignAuth on the root + // AUTH_CHANGED updates isAuthenticated, then this always fires). + // permissionDialog is only ever entered while authenticated, so + // this guard never misfires on entry. + always: [{ guard: 'signedOut', target: 'idle', actions: 'clearPending' }], on: { CONFIRM_PERMISSION: { target: 'idle', actions: 'startDataExchange' }, CANCEL_PERMISSION: { target: 'idle', actions: 'clearPending' }, diff --git a/packages/ui/src/components/bible-reader.tsx b/packages/ui/src/components/bible-reader.tsx index a7c1ae5b..86ae2616 100644 --- a/packages/ui/src/components/bible-reader.tsx +++ b/packages/ui/src/components/bible-reader.tsx @@ -637,6 +637,7 @@ function Content() { const { highlightedVerses, + highlightsInteractive, apply: applyHighlight, remove: removeHighlight, permissionDialogOpen, @@ -659,10 +660,11 @@ function Content() { : undefined, }); - // Color row is interactive when self-contained is live, or always in - // controlled mode (YPE-3705: controlled bypasses HIGHLIGHTS_LIVE). Copy / - // Share are always available. - const highlightsEnabled = isHighlightsControlled || isHighlightsLive(); + // Color row: controlled mode always shows it (YPE-3705 bypasses HIGHLIGHTS_LIVE). + // Self-contained needs the live flag AND an auth provider — without a provider + // the machine is inert (taps noop), so hide dead swatches for copy/share-only. + // Copy / Share are always available. + const highlightsEnabled = isHighlightsControlled || (isHighlightsLive() && highlightsInteractive); // Copy shown to the sign-in dialog. Falls back to a neutral label when the // integrator hasn't set `YouVersionPlatformConfiguration.appName`. const signInAppName = YouVersionPlatformConfiguration.appName ?? t('signInAppNameFallback'); diff --git a/packages/ui/src/components/icons/youversion-platform-logo.test.tsx b/packages/ui/src/components/icons/youversion-platform-logo.test.tsx new file mode 100644 index 00000000..69f58581 --- /dev/null +++ b/packages/ui/src/components/icons/youversion-platform-logo.test.tsx @@ -0,0 +1,24 @@ +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { YouVersionPlatformLogo } from './youversion-platform-logo'; + +describe('YouVersionPlatformLogo', () => { + it('renders an img-role svg using the required accessible label', () => { + render(); + expect(screen.getByRole('img', { name: 'YouVersion Platform' })).toBeTruthy(); + }); + + it('uses the caller-provided (localized) label verbatim', () => { + render(); + expect(screen.getByRole('img', { name: 'Plateforme YouVersion' })).toBeTruthy(); + // No hardcoded English default leaks through. + expect(screen.queryByRole('img', { name: 'YouVersion Platform' })).toBeNull(); + }); + + it('forwards svg props such as className', () => { + render(); + expect(screen.getByRole('img', { name: 'YouVersion Platform' }).getAttribute('class')).toBe( + 'custom-class', + ); + }); +}); diff --git a/packages/ui/src/components/icons/youversion-platform-logo.tsx b/packages/ui/src/components/icons/youversion-platform-logo.tsx new file mode 100644 index 00000000..c8d5a204 --- /dev/null +++ b/packages/ui/src/components/icons/youversion-platform-logo.tsx @@ -0,0 +1,34 @@ +import type { ComponentProps, ReactElement } from 'react'; + +/** + * YouVersion Platform wordmark — matches the Swift SDK's + * `YouVersionPlatformLogo.imageset` (light + dark fills). + */ +type YouVersionPlatformLogoProps = ComponentProps<'svg'> & { + theme?: 'light' | 'dark'; + /** + * Localized accessible label for the wordmark (rendered with `role="img"`). + * Required with no default so callers must supply a translated string rather + * than shipping a hardcoded English label. + */ + 'aria-label': string; +}; + +const FILL = { + light: '#121212', + dark: '#EBDBC8', +} as const; + +const WORDMARK_PATH = + 'M32.7334 14.2031C32.7334 15.2113 32.903 15.9364 33.2363 16.3613C33.5669 16.7805 34.0893 16.9941 34.7891 16.9941C35.2084 16.9941 35.6111 16.9161 35.9805 16.7578C37.1831 16.2466 37.9883 15.0357 37.9883 13.7441V5.38379H41.4023V19.6504H38.4805L38.1553 17.2939C38.1268 17.3388 38.0929 17.3955 38.0537 17.46L38.0527 17.4609V17.4619H38.0518V17.4639H38.0508C37.8639 17.7711 37.5651 18.262 37.2383 18.6025C36.7467 19.108 36.2218 19.4304 35.6357 19.6582C35.0498 19.8859 34.4167 19.9999 33.7559 20C32.7783 20 31.9503 19.7944 31.292 19.3945C30.6309 18.9945 30.1336 18.3995 29.8086 17.6328C29.4865 16.8746 29.3223 15.9277 29.3223 14.8252V5.38379H32.7334V14.2031ZM20.0967 5.03906C20.9826 5.03906 21.8468 5.2081 22.6689 5.54688C23.4884 5.88576 24.2165 6.38051 24.8359 7.0166C25.4525 7.65264 25.9552 8.44427 26.333 9.36914C26.708 10.2913 26.8994 11.3554 26.8994 12.5303C26.8994 13.7024 26.708 14.7664 26.333 15.6914C25.9553 16.6163 25.4525 17.4079 24.8359 18.0439C24.2165 18.6828 23.4884 19.1722 22.6689 19.5C21.8496 19.8277 20.9827 19.9941 20.0967 19.9941C19.208 19.9941 18.35 19.8277 17.5391 19.5C16.728 19.1694 15.9998 18.6801 15.3721 18.0439C14.7444 17.4079 14.2416 16.6163 13.875 15.6914C13.5084 14.7664 13.3223 13.7024 13.3223 12.5303C13.3223 11.3581 13.5084 10.2941 13.875 9.36914C14.2417 8.44426 14.7444 7.65265 15.3721 7.0166C15.9998 6.3805 16.728 5.88576 17.5391 5.54688C18.3499 5.21092 19.2108 5.03911 20.0967 5.03906ZM60.2383 6.58887C61.8299 5.21943 64 4.68357 66.125 5.27246C67.9579 5.78083 69.0718 6.96101 69.6885 8.56348C69.8274 8.93014 69.9445 9.32214 70.0361 9.73047C70.3084 10.9527 70.3185 12.1863 70.1074 13.5029H61.0273C61.1551 14.5388 61.4552 15.4552 62.0771 16.1523C62.3327 16.4412 62.6437 16.6948 63.0215 16.9004C64.3659 17.6309 66.0551 17.2634 66.9551 16.0439C67.0967 15.8551 67.2304 15.6444 67.3525 15.4111C68.2746 15.6555 69.1495 15.886 70.2021 16.1582C69.9383 16.8415 69.5857 17.4891 69.0996 18.0391C68.2246 19.0307 67.0076 19.6304 65.7188 19.8721C62.9966 20.3831 60.2221 19.1498 58.7832 16.7832C57.2527 14.2638 57.3604 10.7777 58.8408 8.2666C59.2241 7.61664 59.6967 7.05551 60.2383 6.58887ZM108.344 5.02246C109.23 5.02246 110.094 5.19144 110.916 5.53027C111.735 5.86916 112.464 6.36389 113.083 7C113.7 7.63605 114.202 8.42767 114.58 9.35254C114.955 10.2747 115.146 11.3387 115.146 12.5137C115.146 13.6858 114.955 14.7498 114.58 15.6748C114.202 16.5997 113.7 17.3913 113.083 18.0273C112.464 18.6662 111.735 19.1556 110.916 19.4834C110.097 19.8111 109.23 19.9775 108.344 19.9775C107.455 19.9775 106.596 19.8112 105.785 19.4834C104.974 19.1528 104.247 18.6634 103.619 18.0273C102.991 17.3912 102.489 16.5998 102.122 15.6748C101.755 14.7498 101.569 13.6859 101.569 12.5137C101.569 11.3415 101.755 10.2775 102.122 9.35254C102.489 8.42767 102.991 7.63604 103.619 7C104.247 6.36401 104.974 5.86914 105.785 5.53027C106.596 5.19418 107.458 5.02248 108.344 5.02246ZM87.3779 5.04688C88.725 4.99696 90.1357 5.33104 91.2217 6.15039C92.0939 6.80039 92.6916 7.71959 93.1055 8.7168C92.6535 8.82742 92.1865 8.95452 91.7207 9.08203C91.2275 9.21704 90.735 9.35253 90.2607 9.4668C90.2607 9.4668 90.1022 9.14978 90.0244 9.01367C89.4661 8.0112 88.3416 7.43633 87.2002 7.67773C86.5142 7.82215 85.767 8.30542 85.6641 9.04688C85.6085 9.45515 85.7553 9.88592 86.0469 10.1748C86.4081 10.5311 87.0153 10.6709 87.5254 10.7881H87.5264C87.5847 10.8015 87.642 10.815 87.6973 10.8281C87.9269 10.8826 88.1623 10.9336 88.4004 10.9854C89.631 11.253 90.9339 11.5361 91.8857 12.3691C92.9134 13.2691 93.4054 14.5443 93.2139 15.9053C93.0833 16.8136 92.6667 17.6892 92.0195 18.3447C90.9362 19.4391 89.3721 19.9416 87.8555 19.9639C86.8139 19.9778 85.7444 19.7694 84.8027 19.3223C83.4444 18.6778 82.5193 17.5526 81.9609 16.1748C82.5713 16.0144 83.1298 15.8651 83.6719 15.7197L83.6748 15.7188L83.6758 15.7178C84.0522 15.6168 84.4211 15.5178 84.7939 15.4189C85.6273 16.9828 86.8359 17.6032 88.4609 17.2754C89.1803 17.1282 89.8521 16.6361 89.9883 15.8779C90.1744 14.8335 89.3162 14.2054 88.4023 13.9971C88.2542 13.9631 88.1057 13.93 87.957 13.8965C86.8288 13.6419 85.6901 13.3848 84.6299 12.9111C81.6025 11.561 81.8555 7.66341 84.1582 6.06348C85.097 5.41082 86.2419 5.08854 87.3779 5.04688ZM202.797 4.66309C203.78 4.66309 204.676 4.83227 205.484 5.16895C206.306 5.49215 207.006 5.97676 207.585 6.62305C208.164 7.25597 208.615 8.04372 208.938 8.98633C209.262 9.91553 209.423 10.9862 209.423 12.1982C209.423 13.4101 209.262 14.4941 208.938 15.4502C208.629 16.3927 208.184 17.1876 207.605 17.834C207.026 18.4804 206.325 18.9719 205.504 19.3086C204.696 19.6452 203.793 19.8135 202.797 19.8135C201.814 19.8134 200.912 19.6522 200.091 19.3291C199.283 18.9924 198.582 18.507 197.989 17.874C197.41 17.2278 196.96 16.4335 196.637 15.4912C196.314 14.5485 196.151 13.4708 196.151 12.2588C196.151 11.0468 196.314 9.96904 196.637 9.02637C196.973 8.08405 197.431 7.2898 198.01 6.64355C198.602 5.99727 199.303 5.50561 200.11 5.16895C200.932 4.83233 201.827 4.66314 202.797 4.66309ZM50.375 15.6387L54.5049 1.0498H58.1855L52.3828 19.6504H48.3662L42.5635 1.0498H46.2412L50.375 15.6387ZM7.27441 9.86426L10.8359 1.03613H14.5498L9.02734 13.6553V19.6387H5.51953V13.6553L0 1.03613H3.71094L7.27441 9.86426ZM99.1436 19.6328H95.6943V5.36621H99.1436V19.6328ZM125.211 5.02246C126.216 5.0225 127.063 5.21979 127.732 5.61133C128.402 6.00577 128.916 6.59173 129.255 7.3584C129.591 8.11951 129.764 9.06423 129.767 10.167V19.6309H126.377V10.8135C126.377 9.79423 126.196 9.05776 125.841 8.63281C125.485 8.21093 124.938 7.9971 124.208 7.99707C123.225 7.99707 122.269 8.45832 121.655 9.23047C121.197 9.80543 120.955 10.5221 120.955 11.2998V19.6328H117.566V5.36621H120.508L120.819 7.66406C121.55 6.12246 122.638 5.59407 123.344 5.34961C123.944 5.14128 124.569 5.02246 125.211 5.02246ZM79.2695 5.08008C80.5286 4.91478 81.2701 5.23434 81.3291 5.25977C81.331 5.26059 81.3326 5.2606 81.333 5.26074L80.8721 8.5166C80.8721 8.5166 80.1632 8.34149 79.5576 8.33594C78.6217 8.33048 77.7076 8.64178 77.041 9.375C76.4467 10.0278 76.1133 11.1555 76.1133 12.0859V19.6143H72.6748V5.34766H75.6387L75.9746 7.79199C76.5996 6.36977 77.839 5.26619 79.2695 5.08008ZM144.186 5.02734C145.249 5.02734 146.125 5.14123 146.812 5.37012C147.512 5.59905 148.058 5.90927 148.448 6.2998C148.852 6.69021 149.128 7.14789 149.276 7.67285C149.438 8.18459 149.519 8.7305 149.519 9.30957C149.519 9.91556 149.438 10.4947 149.276 11.0469C149.115 11.5989 148.832 12.0835 148.428 12.501C148.024 12.9184 147.478 13.2488 146.791 13.4912C146.104 13.7335 145.236 13.8545 144.186 13.8545H141.196V19.4502H139.357V5.02734H144.186ZM153.458 17.7939H160.933L160.649 19.4502H151.6V5.02734H153.458V17.7939ZM174.729 19.4502H172.71L171.194 15.4307H165.014L163.498 19.4502H161.56L167.175 5.02734H169.135L174.729 19.4502ZM184.31 6.68359H179.543V19.4502H177.664V6.68359H172.896V5.02734H184.31V6.68359ZM194.805 6.66309H187.715V11.3906H194.36V13.0664H187.715V19.4502H185.856V5.02734H194.805V6.66309ZM216.427 5.02734C217.504 5.02734 218.386 5.14822 219.073 5.39062C219.76 5.63302 220.299 5.94934 220.689 6.33984C221.093 6.71689 221.369 7.14805 221.518 7.63281C221.666 8.11758 221.739 8.60919 221.739 9.10742C221.739 10.0232 221.544 10.8048 221.153 11.4512C220.776 12.0973 220.15 12.6023 219.275 12.9658L222.305 19.4502H220.225L217.497 13.4102C217.322 13.4371 217.134 13.4572 216.932 13.4707H213.538V19.4502H211.7V5.02734H216.427ZM231.008 17.2285L235.735 5.02734H237.957V19.4502H236.24V8.07715L231.715 19.4502H230.2L225.756 8.11816V19.4502H224.06V5.02734H226.362L231.008 17.2285ZM202.797 6.2793C202.016 6.27935 201.33 6.42141 200.737 6.7041C200.158 6.9869 199.667 7.39082 199.263 7.91602C198.872 8.44122 198.575 9.07378 198.373 9.81445C198.185 10.5416 198.091 11.3496 198.091 12.2383C198.091 13.1406 198.185 13.9625 198.373 14.7031C198.575 15.4303 198.872 16.0568 199.263 16.582C199.653 17.0936 200.138 17.4907 200.717 17.7734C201.309 18.0562 202.003 18.1972 202.797 18.1973C203.564 18.1973 204.238 18.0562 204.817 17.7734C205.41 17.4907 205.901 17.0937 206.292 16.582C206.683 16.0569 206.979 15.4303 207.181 14.7031C207.383 13.9624 207.483 13.1406 207.483 12.2383C207.483 11.3496 207.383 10.5416 207.181 9.81445C206.979 9.07385 206.675 8.44119 206.271 7.91602C205.881 7.39085 205.396 6.9869 204.817 6.7041C204.238 6.4213 203.564 6.2793 202.797 6.2793ZM20.0967 7.88379C19.0913 7.8839 18.294 8.30612 17.7246 9.13379C17.1526 9.96981 16.8614 11.114 16.8613 12.5303C16.8613 13.9303 17.1496 15.0643 17.7246 15.9004C18.2912 16.7307 19.0914 17.1503 20.0967 17.1504C21.1022 17.1504 21.9056 16.7308 22.4834 15.9004C23.0667 15.0615 23.3613 13.9275 23.3613 12.5303C23.3613 11.114 23.0665 9.97259 22.4834 9.13379C21.9056 8.30324 21.1022 7.88379 20.0967 7.88379ZM108.341 7.86621C107.335 7.8663 106.538 8.28867 105.969 9.11621C105.397 9.95228 105.105 11.0971 105.105 12.5137C105.105 13.9134 105.394 15.0467 105.969 15.8828C106.535 16.7133 107.335 17.1327 108.341 17.1328C109.346 17.1328 110.15 16.7134 110.728 15.8828C111.311 15.044 111.608 13.9106 111.605 12.5137C111.605 11.0971 111.311 9.95506 110.728 9.11621C110.15 8.28576 109.346 7.86621 108.341 7.86621ZM165.579 13.8145H170.629L168.084 7.12793L165.579 13.8145ZM141.196 12.2793H144.165C144.865 12.2793 145.445 12.2118 145.902 12.0771C146.36 11.9425 146.718 11.7471 146.974 11.4912C147.229 11.2354 147.404 10.9321 147.498 10.582C147.606 10.2184 147.66 9.81452 147.66 9.37012C147.66 8.91225 147.606 8.50834 147.498 8.1582C147.39 7.80808 147.202 7.51799 146.933 7.28906C146.663 7.06028 146.299 6.89187 145.842 6.78418C145.397 6.66305 144.832 6.60256 144.146 6.60254H141.196V12.2793ZM213.538 11.875H216.608C217.255 11.875 217.787 11.8145 218.204 11.6934C218.622 11.5587 218.952 11.3764 219.194 11.1475C219.437 10.9051 219.605 10.6158 219.699 10.2793C219.793 9.94263 219.841 9.56493 219.841 9.14746C219.841 8.74364 219.786 8.38677 219.679 8.07715C219.571 7.76751 219.383 7.50448 219.113 7.28906C218.858 7.06032 218.508 6.89188 218.063 6.78418C217.619 6.66298 217.053 6.60254 216.366 6.60254H213.538V11.875ZM66.0469 8.12207C64.9774 7.48597 63.8549 7.51389 62.791 8.125C62.7722 8.13529 62.7547 8.14711 62.7373 8.1582C62.7266 8.16502 62.7156 8.17142 62.7051 8.17773C62.2024 8.48602 61.8361 8.87786 61.5723 9.33887C61.2639 9.87219 61.0942 10.4944 61.0137 11.1777H67.2441C67.3719 10.2444 67.0578 9.15015 66.4912 8.50293C66.3551 8.34737 66.208 8.21651 66.0469 8.12207ZM97.4053 0C97.9912 2.27994e-05 98.4826 0.177726 98.8604 0.530273C99.2409 0.88305 99.4355 1.34175 99.4355 1.89453C99.4355 2.44727 99.2409 2.89971 98.8604 3.24414C98.4826 3.58576 97.9913 3.75877 97.4053 3.75879C96.8192 3.75879 96.3298 3.58581 95.9492 3.24414C95.5689 2.89976 95.375 2.44709 95.375 1.89453C95.375 1.34179 95.5687 0.883042 95.9492 0.530273C96.3298 0.177497 96.8192 0 97.4053 0Z'; + +export function YouVersionPlatformLogo({ + theme = 'light', + ...props +}: YouVersionPlatformLogoProps): ReactElement { + return ( + + + + ); +} diff --git a/packages/ui/src/components/sign-in-dialog.tsx b/packages/ui/src/components/sign-in-dialog.tsx index e065d905..9e6211ba 100644 --- a/packages/ui/src/components/sign-in-dialog.tsx +++ b/packages/ui/src/components/sign-in-dialog.tsx @@ -1,7 +1,7 @@ import type { FC } from 'react'; import { useTranslation } from 'react-i18next'; import i18n from '@/i18n'; -import { YouVersionLogo } from './icons/youversion-logo'; +import { YouVersionPlatformLogo } from './icons/youversion-platform-logo'; import { Button } from './ui/button'; import { Dialog, DialogContent, DialogDescription, DialogTitle } from './ui/dialog'; @@ -52,9 +52,10 @@ export const SignInDialog: FC = ({ {t('signInIntroducing')} - 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 af7e61a7..15b2d32f 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 @@ -14,17 +14,15 @@ import { HighlightsClient, YouVersionAPIUsers, YouVersionPlatformConfiguration, - type YouVersionUserInfo, } from '@youversion/platform-core'; import { YouVersionAuthContext, YouVersionContext } from '@youversion/platform-react-hooks'; import type { ReactNode } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; import { readPendingHighlights, stashPendingHighlight } from '@/lib/pending-highlight'; +import { mockUserInfo } from '@/test/highlights-test-utils'; 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 }) { @@ -222,6 +220,37 @@ describe('highlight auth flow — just-in-time (signed in, no permission)', () = expect(result.current.permissionDialogOpen).toBe(false); expect(readPendingHighlights()).toEqual([]); }); + + it('closes the dialog and clears pending when the host signs the user out mid-flow', () => { + const updateToken = vi + .spyOn(DataExchangeClient.prototype, 'updateToken') + .mockResolvedValue('dx-token'); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + act(() => { + result.current.apply('fffe00', [16]); + }); + expect(result.current.permissionDialogOpen).toBe(true); + expect(readPendingHighlights()).toHaveLength(1); + + // The host signs the user out while the confirm dialog is still open. A + // confirm now would start a data exchange that rejects unauthenticated, so + // the flow must route back out of the dialog and drop the pending intent. + signedIn = false; + rerender(); + + expect(result.current.permissionDialogOpen).toBe(false); + expect(readPendingHighlights()).toEqual([]); + + // A confirm after the auto-close is a no-op: no data exchange is started. + act(() => { + result.current.confirmPermissionDialog(); + }); + expect(updateToken).not.toHaveBeenCalled(); + }); }); describe('highlight auth flow — data-exchange return', () => { @@ -230,6 +259,9 @@ describe('highlight auth flow — data-exchange return', () => { setLocation( 'https://host.example/read?data_exchange_status=granted&granted_permissions=highlights', ); + // Record the initiator as the redirect leg would have — the callback only + // saves a grant for the user who started the exchange. + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); // Pre-stash a pending highlight as the confirm path would have. stashPendingHighlight({ verses: [16], @@ -262,6 +294,57 @@ describe('highlight auth flow — data-exchange return', () => { expect(readPendingHighlights()).toEqual([]); }); + it('re-stashes pending and re-prompts when the resumed write fails 401 (async session hydration)', async () => { + // Same granted-return path as above, but the resumed POST comes back 401. + // The user's original color tap must NOT be silently lost: the pending is + // re-stashed (from the op's own scope), the permission cache is invalidated, + // and the permission dialog re-opens — the same handling as a user apply. + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + signedIn = false; + setLocation( + 'https://host.example/read?data_exchange_status=granted&granted_permissions=highlights', + ); + // Record the initiator as the redirect leg would have (see test above). + YouVersionPlatformConfiguration.saveDataExchangeInitiator(); + stashPendingHighlight({ + verses: [16], + color: 'fffe00', + versionId: 111, + book: 'JHN', + chapter: '3', + timestamp: Date.now(), + }); + const createHighlight = vi + .spyOn(HighlightsClient.prototype, 'createHighlight') + .mockRejectedValue(httpError(401)); + + const { result, rerender } = renderHook(() => useBibleReaderHighlights(options), { + wrapper: Providers, + }); + + signedIn = true; + rerender(); + + await waitFor(() => { + expect(result.current.permissionDialogOpen).toBe(true); + }); + // The resumed write was attempted, then failed. + expect(createHighlight).toHaveBeenCalledWith({ + version_id: 111, + passage_id: 'JHN.3.16', + color: 'fffe00', + }); + // Server truth wins: cache invalidated, and the pending is re-stashed from the + // op's own scope so a post-grant confirm can resume it. + expect(YouVersionPlatformConfiguration.hasPermission('highlights')).toBe(false); + expect(readPendingHighlights()[0]).toMatchObject({ + verses: [16], + color: 'fffe00', + versionId: 111, + chapter: '3', + }); + }); + 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 diff --git a/packages/ui/src/components/use-bible-reader-highlights.dom-vapor.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.dom-vapor.test.tsx new file mode 100644 index 00000000..74001328 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.dom-vapor.test.tsx @@ -0,0 +1,133 @@ +/** + * @vitest-environment jsdom + * + * DOM-level reproduction of the live vapor flash, mirroring the coordinator's + * MutationObserver instrumentation on the verse wrapper's `style` attribute. + * The earlier suites assert the hook's `highlightedVerses` output; this one + * renders the REAL `Verse.Html` (whose `useLayoutEffect` imperatively paints + * `backgroundColor` on `.yv-v[v]`) driven by the REAL adapter, and records every + * background-color mutation. A one-frame resurrection that only shows up as an + * imperative repaint — not in the hook's returned map — is caught here. + */ +import { StrictMode, useEffect, useState } from 'react'; +import { act, render, waitFor } from '@testing-library/react'; +import { + HighlightsClient, + YouVersionPlatformConfiguration, + type Collection, + type Highlight, +} from '@youversion/platform-core'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { collection, Providers } from '@/test/highlights-test-utils'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; +import { Verse } from './verse'; + +const options = { versionId: 111, book: 'JHN', chapter: '1' }; +const CHAPTER_HTML = + '

2' + + 'In the beginning was the Word.

'; + +beforeEach(() => { + vi.restoreAllMocks(); + localStorage.clear(); + sessionStorage.clear(); + setHighlightsLive(true); + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); +}); + +function Reader({ removeRef }: { removeRef: { current: (() => void) | null } }) { + const [selected, setSelected] = useState([2]); + const api = useBibleReaderHighlights(options); + useEffect(() => { + removeRef.current = () => { + api.remove('fffe00', selected); + // Popover close → selection clear on a SEPARATE tick (Radix close), so the + // clear re-render lands after the optimistic overlay commit — modeling the + // real reader rather than a single batched update. + setTimeout(() => setSelected([]), 0); + }; + }); + return ( + undefined} + /> + ); +} + +describe('vapor flash — real Verse.Html DOM paint (MutationObserver on style)', () => { + it('the verse-2 background is never repainted to yellow after the optimistic unpaint', async () => { + const withRow = () => collection([{ version_id: 111, passage_id: 'JHN.1.2', color: 'fffe00' }]); + // The post-remove refetch is held unresolved to widen the settle→response + // window the live flash lives in; it never settles. + const heldRefetch = new Promise>(vi.fn()); + let removed = false; + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockImplementation(() => (removed ? heldRefetch : Promise.resolve(withRow()))); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + // Real network gap: settle lands on a macrotask, well after the optimistic + // render commits and paints — the window the live flash lives in. + .mockImplementation(() => new Promise((r) => setTimeout(r, 5))); + + const removeRef: { current: (() => void) | null } = { current: null }; + const { container } = render( + + + + + , + ); + + const verseEl = () => container.querySelector('.yv-v[v="2"]'); + // Wait until server truth has painted verse 2 yellow. + await waitFor(() => { + const bg = verseEl()?.style.backgroundColor ?? ''; + expect(bg).not.toBe(''); + }); + const mountFetches = getHighlights.mock.calls.length; + + // Instrument: record every background-color the verse-2 wrapper takes on + // from here (after the optimistic unpaint we expect it to stay transparent). + const paints: string[] = []; + const el = verseEl()!; + const observer = new MutationObserver(() => { + paints.push(el.style.backgroundColor); + }); + observer.observe(el, { attributes: true, attributeFilter: ['style'] }); + + act(() => { + removed = true; + removeRef.current?.(); + }); + + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(getHighlights.mock.calls.length).toBeGreaterThan(mountFetches)); + await act(async () => { + await new Promise((r) => setTimeout(r, 20)); + }); + observer.disconnect(); + + // The first mutation must be the optimistic unpaint (→ ''); no later mutation + // may bring yellow back while the refetch is still in flight. + const yellowAfterUnpaint = paints.filter((bg) => bg !== ''); + expect( + yellowAfterUnpaint, + `verse 2 repainted non-transparent after removal: ${JSON.stringify(paints)}`, + ).toEqual([]); + // Final DOM state: transparent. + expect(verseEl()?.style.backgroundColor).toBe(''); + }); +}); 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 c17acb9b..7b13ff8a 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,42 +9,18 @@ * mount never fetched highlights at all. */ import { act, renderHook, waitFor } from '@testing-library/react'; -import { - HighlightsClient, - YouVersionPlatformConfiguration, - type Collection, - type Highlight, - type YouVersionUserInfo, -} from '@youversion/platform-core'; -import { YouVersionAuthContext, YouVersionContext } from '@youversion/platform-react-hooks'; +import { HighlightsClient, YouVersionPlatformConfiguration } from '@youversion/platform-core'; import type { ReactNode } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { collection, mockUserInfo, Providers as BaseProviders } from '@/test/highlights-test-utils'; 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; +// Signed-in state flips between rerenders; the wrapper re-reads `signedIn`. function Providers({ children }: { children: ReactNode }) { - return ( - - - {children} - - - ); + return {children}; } const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; diff --git a/packages/ui/src/components/use-bible-reader-highlights.strictmode-vapor.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.strictmode-vapor.test.tsx new file mode 100644 index 00000000..68d12d29 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.strictmode-vapor.test.tsx @@ -0,0 +1,134 @@ +/** + * @vitest-environment jsdom + * + * Faithful reproduction of the LIVE vapor flash reported by the coordinator: + * a SERVER-TRUTH highlight (fresh page load, no in-session apply) that, when + * removed, disappears optimistically, REAPPEARS at DELETE-settle time (before + * any refetch response), then disappears when the refetch lands. + * + * Ingredients that distinguish this from the earlier passing suites: + * 1. React.StrictMode (double-invoked effects / actor lifecycle). + * 2. A parent `selectedVerses` state cleared right after the remove + * (the popover close → selection-clear re-render). + * 3. The refetch held UNRESOLVED to widen the settle→response window. + * 4. Source is initial-fetch server truth, never an in-session apply. + * + * We record every committed `highlightedVerses` (the value verse.tsx paints + * from), so a resurrection frame is captured. + */ +import { StrictMode, useState, useEffect } from 'react'; +import { act, render, waitFor } from '@testing-library/react'; +import { + HighlightsClient, + YouVersionPlatformConfiguration, + type Collection, + type Highlight, +} from '@youversion/platform-core'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { collection, Providers } from '@/test/highlights-test-utils'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; + +const options = { versionId: 111, book: 'JHN', chapter: '1' }; + +beforeEach(() => { + vi.restoreAllMocks(); + localStorage.clear(); + sessionStorage.clear(); + setHighlightsLive(true); + // Signed-in-from-first-render: server truth arrives via the initial fetch. + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); +}); + +/** Harness mirroring BibleReader: a selection state cleared right after remove. */ +function Harness({ + frames, + removeRef, +}: { + frames: Record[]; + removeRef: { current: (() => void) | null }; +}) { + const [selected, setSelected] = useState([2]); + const api = useBibleReaderHighlights(options); + // Record the exact value verse.tsx would paint from, each committed render. + frames.push(api.highlightedVerses); + // Expose the popover "remove" action: remove + close/clear selection, exactly + // as BibleReader.handleClearHighlight → closeAndClearSelection does. + useEffect(() => { + removeRef.current = () => { + api.remove('fffe00', selected); + setSelected([]); + }; + }); + return
; +} + +describe('vapor flash — StrictMode + server-truth remove + selection-clear + held refetch', () => { + it('never repaints verse 2 between remove-click and the (held) refetch response', async () => { + // StrictMode double-mounts → the mount fires TWO fetches; only the + // POST-remove refetch is the one we hold. Phase-gate the mock so every + // mount fetch resolves server truth (verse 2) and the settle refetch stays + // unresolved to widen the settle→response window the live repaint lives in. + const withRow = () => collection([{ version_id: 111, passage_id: 'JHN.1.2', color: 'fffe00' }]); + // The post-remove refetch is held unresolved to widen the settle→response + // window the live repaint lives in; it never settles. + const heldRefetch = new Promise>(vi.fn()); + let removed = false; + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + .mockImplementation(() => (removed ? heldRefetch : Promise.resolve(withRow()))); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockResolvedValue(undefined); + + const frames: Record[] = []; + const removeRef: { current: (() => void) | null } = { current: null }; + + render( + + + + + , + ); + + // Wait for server truth to render verse 2 highlighted. + await waitFor(() => { + expect(frames.at(-1)).toEqual({ 2: 'fffe00' }); + }); + const mountFetches = getHighlights.mock.calls.length; + + const startFrame = frames.length; + + // Tap the remove checkmark → optimistic unpaint + selection clear. + act(() => { + removed = true; + removeRef.current?.(); + }); + + // Let the DELETE settle (fires the held refetch) and flush microtasks — + // this is the window the live repro repaints in (before any GET response). + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(getHighlights.mock.calls.length).toBeGreaterThan(mountFetches)); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + const window = frames.slice(startFrame); + const resurrected = window.filter((f) => f[2] === 'fffe00'); + expect( + resurrected, + `verse 2 resurrected in ${resurrected.length}/${window.length} frame(s): ` + + JSON.stringify(window), + ).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 6e9b87e1..b789d7e1 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.test.tsx +++ b/packages/ui/src/components/use-bible-reader-highlights.test.tsx @@ -4,13 +4,11 @@ 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 { - YouVersionPlatformConfiguration, - type YouVersionUserInfo, -} from '@youversion/platform-core'; +import { YouVersionPlatformConfiguration } from '@youversion/platform-core'; import type { ReactNode } from 'react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { mockUserInfo } from '@/test/highlights-test-utils'; import { useBibleReaderHighlights } from './use-bible-reader-highlights'; vi.mock('@youversion/platform-react-hooks', async () => { @@ -21,8 +19,6 @@ vi.mock('@youversion/platform-react-hooks', async () => { }; }); -const mockUserInfo = { id: 'user-1', name: 'Test User' } as unknown as YouVersionUserInfo; - function makeCollection(data: Highlight[]): Collection { return { data, next_page_token: null }; } @@ -135,6 +131,28 @@ describe('useBibleReaderHighlights — auth guarding', () => { expect(mocked.createHighlight).not.toHaveBeenCalled(); }); + it('is non-interactive with no auth provider, even with the flag on (inert color row)', () => { + mockUseHighlights(); + + // Flag on but no auth provider: the machine is disabled, so a color tap can + // never do anything. The color-swatch row must not render — this flag is + // what BibleReader ANDs with the feature flag to hide it. + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions)); + expect(result.current.highlightsInteractive).toBe(false); + }); + + it('is interactive when an auth provider is mounted (flag on), even signed out', () => { + mockUseHighlights(); + signedIn = false; + + // Signed out but with an auth provider present: a tap still enters the + // sign-in flow, so the row stays interactive. + const { result } = renderHook(() => useBibleReaderHighlights(defaultOptions), { + wrapper: AuthWrapper, + }); + expect(result.current.highlightsInteractive).toBe(true); + }); + it('clears rendered highlights immediately when the user signs out', () => { mockUseHighlights({ highlights: makeCollection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), diff --git a/packages/ui/src/components/use-bible-reader-highlights.ts b/packages/ui/src/components/use-bible-reader-highlights.ts index 03799d02..1542bf22 100644 --- a/packages/ui/src/components/use-bible-reader-highlights.ts +++ b/packages/ui/src/components/use-bible-reader-highlights.ts @@ -72,6 +72,15 @@ export type UseBibleReaderHighlightsReturn = { 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 highlighting can actually function in this mount — i.e. a + * `YouVersionAuthProvider` is present so a color tap can enter the auth flow + * and writes can reach the API. `false` for copy/share-only integrators (no + * auth provider), where the machine sits in `disabled` and taps resolve to + * `noop`. The caller ANDs this with the feature flag to decide whether to + * render the (otherwise inert) color-swatch row. + */ + highlightsInteractive: boolean; /** Whether the just-in-time permission confirm dialog is open. */ permissionDialogOpen: boolean; /** Controlled open-change for the permission confirm dialog. */ @@ -238,10 +247,24 @@ export function useBibleReaderHighlights({ // 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], - ); + // + // `useApiData` swaps `highlights` for a fresh object on every refetch, even + // when the content is byte-identical. Parsing off that identity would mint a + // new `serverColors` each time and cascade a new `highlightedVerses` reference + // → a chapter-wide verse-style re-sweep (verse.tsx keys a useLayoutEffect on + // it). Hold the prior parsed reference when the content is unchanged so the + // downstream memos stay reference-stable across no-op refetches. (This is + // separate from `lastSentServerColorsRef`, which dedups machine sends.) + const parsedServerColorsRef = useRef(null); + const serverColors = useMemo(() => { + const parsed = parseServerColors(highlights, versionId, chapterUsfm); + const previous = parsedServerColorsRef.current; + if (previous !== null && serverColorsEqual(previous, parsed)) { + return previous; + } + parsedServerColorsRef.current = parsed; + return parsed; + }, [highlights, versionId, chapterUsfm]); const lastSentServerColorsRef = useRef(null); useEffect(() => { if ( @@ -369,6 +392,12 @@ export function useBibleReaderHighlights({ return { highlightedVerses, + // Interactivity mirrors the machine's enabled/disabled gate: with no auth + // provider the machine is inert and the color row must not render. The flag + // is ANDed in by the caller. (`live` also folds in `isAuthenticated`, which + // we intentionally exclude here — a signed-out tap still enters the sign-in + // flow, so the row stays interactive.) + highlightsInteractive: hasAuthProvider, permissionDialogOpen, signInDialogOpen, ...api, diff --git a/packages/ui/src/components/use-bible-reader-highlights.vapor.test.tsx b/packages/ui/src/components/use-bible-reader-highlights.vapor.test.tsx new file mode 100644 index 00000000..7094d746 --- /dev/null +++ b/packages/ui/src/components/use-bible-reader-highlights.vapor.test.tsx @@ -0,0 +1,278 @@ +/** + * @vitest-environment jsdom + * + * Vapor-flash reproduction (YPE-1034). The existing integration suite only + * asserts the FINAL rendered map after a remove settles. The reported bug is a + * TRANSIENT frame: the removed verse disappears (optimistic), REAPPEARS for a + * split second, then disappears forever. To catch a one-frame regression this + * suite records EVERY committed render of `highlightedVerses` and asserts the + * removed verse never reappears at any frame between "remove tapped" and + * "server truth converges". + */ +import { act, renderHook, waitFor } from '@testing-library/react'; +import { + HighlightsClient, + YouVersionPlatformConfiguration, + type Collection, + type Highlight, +} from '@youversion/platform-core'; +import type { ReactNode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { HIGHLIGHTS_LIVE, setHighlightsLive } from '@/lib/feature-flags'; +import { + collection, + deferred, + mockUserInfo, + Providers as BaseProviders, +} from '@/test/highlights-test-utils'; +import { useBibleReaderHighlights } from './use-bible-reader-highlights'; + +let signedIn = false; + +// Signed-in state flips between rerenders; the wrapper re-reads `signedIn`. +function Providers({ children }: { children: ReactNode }) { + return {children}; +} + +const defaultOptions = { versionId: 111, book: 'JHN', chapter: '3' }; + +beforeEach(() => { + vi.restoreAllMocks(); + signedIn = false; + setHighlightsLive(true); +}); + +afterEach(() => { + vi.restoreAllMocks(); + setHighlightsLive(HIGHLIGHTS_LIVE); + localStorage.clear(); + sessionStorage.clear(); +}); + +/** + * Records `highlightedVerses` on EVERY committed render (this testing-library + * build has no `result.all`). The hook runs inside a real render, so `renderLog` + * captures the exact frames the DOM would paint. + */ +function mountFlipped(renderLog: Record[]) { + localStorage.clear(); + YouVersionPlatformConfiguration.saveUserInfo({ id: 'user-1', name: 'Test User' }); + YouVersionPlatformConfiguration.saveGrantedPermissions(['highlights']); + // Bearer token for tests that drive the REAL client via a global.fetch stub. + // Harmless for tests that spy on HighlightsClient.prototype directly. + localStorage.setItem('accessToken', 'test-access-token'); + const view = renderHook( + () => { + const api = useBibleReaderHighlights(defaultOptions); + renderLog.push(api.highlightedVerses); + return api; + }, + { wrapper: Providers }, + ); + signedIn = true; + view.rerender(); + return view; +} + +describe('vapor flash — removed verse must never reappear at any frame', () => { + it('stale refetch (read-replica lag still contains the row): no frame repaints verse 16', async () => { + const getDeferred = deferred>(); + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + // mount GET + .mockResolvedValueOnce( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ) + // post-DELETE refetch: controlled, resolves STALE (still has the row) + .mockReturnValueOnce(getDeferred.promise); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockResolvedValue(undefined); + + const renderLog: Record[] = []; + const { result } = mountFlipped(renderLog); + await waitFor(() => expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' })); + + // The point at which we START watching for a resurrection. + act(() => { + result.current.remove('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({}); + + const startFrame = renderLog.length; + + // Let the DELETE settle (fires the refetch), then resolve the STALE GET. + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + await act(async () => { + getDeferred.resolve( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ); + await Promise.resolve(); + }); + + // Inspect EVERY committed frame from the remove onward. + const frames = renderLog.slice(startFrame); + const resurrected = frames.filter((f) => f[16] === 'fffe00'); + expect( + resurrected, + `verse 16 reappeared in ${resurrected.length} frame(s): ${JSON.stringify(frames)}`, + ).toEqual([]); + expect(result.current.highlightedVerses).toEqual({}); + }); + + it('fresh refetch (no row): no frame repaints verse 16', async () => { + const getDeferred = deferred>(); + vi.spyOn(HighlightsClient.prototype, 'getHighlights') + .mockResolvedValueOnce( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ) + .mockReturnValueOnce(getDeferred.promise); + const deleteHighlight = vi + .spyOn(HighlightsClient.prototype, 'deleteHighlight') + .mockResolvedValue(undefined); + + const renderLog: Record[] = []; + const { result } = mountFlipped(renderLog); + await waitFor(() => expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' })); + + act(() => { + result.current.remove('fffe00', [16]); + }); + const startFrame = renderLog.length; + + await waitFor(() => expect(deleteHighlight).toHaveBeenCalledTimes(1)); + await act(async () => { + getDeferred.resolve(collection([])); + await Promise.resolve(); + }); + + const frames = renderLog.slice(startFrame); + const resurrected = frames.filter((f) => f[16] === 'fffe00'); + expect(resurrected, `verse 16 reappeared: ${JSON.stringify(frames)}`).toEqual([]); + expect(result.current.highlightedVerses).toEqual({}); + }); + + it('END-TO-END through the real client: a successful DELETE returning 200 empty-JSON must not flash the removed verse (live vapor)', async () => { + // The actual live mechanism, exercised through the REAL HighlightsClient + + // ApiClient (only global.fetch is stubbed). The server DELETES the row and + // returns `200 application/json` with an EMPTY body. Before the core fix, + // `response.json()` threw on that empty body, so `deleteHighlight` rejected + // on a real success → settleWrite reverted the optimistic removal → the + // verse repainted from raw server truth until the refetch, then vanished. + // With the fix the DELETE resolves, the overlay holds, and no frame flashes. + vi.spyOn(console, 'error').mockImplementation(vi.fn()); + // (mountFlipped seeds the bearer token the real client reads from localStorage.) + + // Wire format uses `bible_id` (mapped to `version_id` by the client). + const wireRow = { bible_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }; + let deletedOnServer = false; + const jsonHeaders = { 'content-type': 'application/json' }; + + vi.spyOn(global, 'fetch').mockImplementation((input, init) => { + const url = typeof input === 'string' ? input : (input as Request).url; + const method = (init?.method ?? 'GET').toUpperCase(); + if (url.includes('/v1/highlights') && method === 'DELETE') { + deletedOnServer = true; + // The bug shape: 200 OK, JSON content-type, EMPTY body. + return Promise.resolve(new Response('', { status: 200, headers: jsonHeaders })); + } + if (url.includes('/v1/highlights') && method === 'GET') { + const data = deletedOnServer ? [] : [wireRow]; + return Promise.resolve( + new Response(JSON.stringify({ data, next_page_token: null }), { + status: 200, + headers: jsonHeaders, + }), + ); + } + return Promise.resolve(new Response('{}', { status: 200, headers: jsonHeaders })); + }); + + const renderLog: Record[] = []; + const { result } = mountFlipped(renderLog); + await waitFor(() => expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' })); + + act(() => { + result.current.remove('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({}); + const startFrame = renderLog.length; + + await waitFor(() => expect(deletedOnServer).toBe(true)); + await act(async () => { + await new Promise((r) => setTimeout(r, 10)); + }); + + const frames = renderLog.slice(startFrame); + const resurrected = frames.filter((f) => f[16] === 'fffe00'); + expect( + resurrected, + `verse 16 resurrected in ${resurrected.length} frame(s): ${JSON.stringify(frames)}`, + ).toEqual([]); + expect(result.current.highlightedVerses).toEqual({}); + }); + + it('out-of-order fetches through real useApiData: fresh reflects removal, then a NEWER refetch returns a stale replica row — no resurrection', async () => { + // The one real-world path to deliver fresh→stale to the machine through + // useApiData's requestSeq guard: two refetches where the SECOND (newer) + // request hits a lagging replica. Refetch #1 (settle of the remove) reflects + // the removal; refetch #2 (settle of a later apply on verse 20) is stale and + // still carries verse 16. The held remove overlay must win. + const getHighlights = vi + .spyOn(HighlightsClient.prototype, 'getHighlights') + // mount + .mockResolvedValueOnce( + collection([{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]), + ) + // refetch #1: fresh, reflects the DELETE of 16 + .mockResolvedValueOnce(collection([])) + // refetch #2: STALE replica — verse 16 is back (plus the just-applied 20) + .mockResolvedValue( + collection([ + { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, + { version_id: 111, passage_id: 'JHN.3.20', color: '5dff79' }, + ]), + ); + vi.spyOn(HighlightsClient.prototype, 'deleteHighlight').mockResolvedValue(undefined); + vi.spyOn(HighlightsClient.prototype, 'createHighlight').mockResolvedValue({ + version_id: 111, + passage_id: 'JHN.3.20', + color: '5dff79', + }); + + const renderLog: Record[] = []; + const { result } = mountFlipped(renderLog); + await waitFor(() => expect(result.current.highlightedVerses).toEqual({ 16: 'fffe00' })); + + act(() => { + result.current.remove('fffe00', [16]); + }); + expect(result.current.highlightedVerses).toEqual({}); + const startFrame = renderLog.length; + + // Refetch #1 lands (fresh). + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(2)); + await act(async () => { + await Promise.resolve(); + }); + + // A later apply on verse 20 fires refetch #2, which returns the stale replica. + act(() => { + result.current.apply('5dff79', [20]); + }); + await waitFor(() => expect(getHighlights).toHaveBeenCalledTimes(3)); + await act(async () => { + await Promise.resolve(); + }); + + const frames = renderLog.slice(startFrame); + const resurrected = frames.filter((f) => f[16] === 'fffe00'); + expect( + resurrected, + `verse 16 resurrected in ${resurrected.length} frame(s): ${JSON.stringify(frames)}`, + ).toEqual([]); + // Verse 20 (freshly applied) is legitimately present; verse 16 must be gone. + expect(result.current.highlightedVerses[16]).toBeUndefined(); + }); +}); diff --git a/packages/ui/src/components/verse-action-popover.test.tsx b/packages/ui/src/components/verse-action-popover.test.tsx index 672f8673..619c03aa 100644 --- a/packages/ui/src/components/verse-action-popover.test.tsx +++ b/packages/ui/src/components/verse-action-popover.test.tsx @@ -1,6 +1,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen, fireEvent } from '@testing-library/react'; -import { VerseActionPopover, HIGHLIGHT_COLORS, type HighlightColor } from './verse-action-popover'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { + VerseActionPopover, + HIGHLIGHT_COLORS, + computeScrollFade, + type HighlightColor, +} from './verse-action-popover'; describe('VerseActionPopover', () => { const defaultProps = { @@ -47,6 +52,27 @@ describe('VerseActionPopover', () => { }); }); + describe('AC1b: Initial focus on open', () => { + // The bar opens from a mouse/tap on non-focusable verse text. If Radix + // autofocused the first swatch, Chromium would treat that programmatic focus + // as keyboard-like and paint a stray `:focus-visible` ring on the swatch. We + // redirect initial focus to the (non-tabbable) content element instead, so + // the ring only appears once the user actually Tabs to a swatch. + it('moves initial focus to the popover content, not the first swatch', async () => { + render(); + + const dialog = screen.getByRole('dialog'); + const firstSwatch = screen + .getAllByRole('button') + .find((btn) => btn.getAttribute('aria-label')?.includes('Apply'))!; + + await waitFor(() => { + expect(document.activeElement).toBe(dialog); + }); + expect(document.activeElement).not.toBe(firstSwatch); + }); + }); + describe('AC2: Apply highlight', () => { it('should call onHighlight when color circle clicked', () => { const onHighlight = vi.fn(); @@ -220,7 +246,7 @@ describe('VerseActionPopover', () => { expect(applyButtons).toHaveLength(5); }); - it('should call onClearHighlight with color when X circle clicked', () => { + it('should call onClearHighlight with color when clear swatch clicked', () => { const onClearHighlight = vi.fn(); const activeHighlights = new Set([HIGHLIGHT_COLORS[0]]); const selectedVerses = [1]; @@ -508,6 +534,187 @@ describe('VerseActionPopover', () => { }); }); + describe('Swatch fill preview (theme-aware)', () => { + // The first apply swatch (no active highlights) is the first canonical color, + // yellow `fffe00` → rgb(255, 254, 0). + function firstApplySwatch() { + return applyButtons()[0]!; + } + + it('paints swatches at full opacity in light mode (matches the applied fill)', () => { + render(); + const bg = firstApplySwatch().style.backgroundColor; + // Full-strength: never the dimmed dark-mode alpha. + expect(bg).not.toContain('0.3'); + expect(bg).toBeTruthy(); + }); + + it('dims swatches to the dark-mode alpha (0.3) in dark mode', () => { + render(); + // Same 0.3 alpha the applied highlight paints in dark mode (verse.tsx). + expect(firstApplySwatch().style.backgroundColor).toBe('rgba(255, 254, 0, 0.3)'); + }); + + it('uses a dark inner stroke in light mode and a light one in dark mode', () => { + const { unmount } = render(); + expect(firstApplySwatch().style.border).toContain('rgba(18, 18, 18, 0.2)'); + unmount(); + + render(); + expect(firstApplySwatch().style.border).toContain('rgba(255, 255, 255, 0.2)'); + }); + + it('keeps the active-swatch checkmark legible on the dimmed dark-mode fill', () => { + render( + ([HIGHLIGHT_COLORS[0]])} + selectedVerses={[1]} + highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} + />, + ); + const check = screen.getByRole('button', { name: /Clear highlight/ }).querySelector('svg'); + expect(check?.getAttribute('class')).toContain('yv:text-white'); + }); + + it('keeps the light-mode checkmark in the Text/Everdark color', () => { + render( + ([HIGHLIGHT_COLORS[0]])} + selectedVerses={[1]} + highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} + />, + ); + const check = screen.getByRole('button', { name: /Clear highlight/ }).querySelector('svg'); + expect(check?.getAttribute('class')).toContain('yv:text-(--yv-gray-50)'); + }); + }); + + describe('Viewport width cap + scrollable swatch row', () => { + // Mobile bug: several verses with different highlight colors accumulate many + // swatches and grow the pill past the viewport, cutting content off with no + // way to reach it. The pill is now capped to the viewport and the swatch row + // scrolls horizontally inside it. + it('caps the popover content to the min of the Radix available width and the live viewport', () => { + render(); + const dialog = screen.getByRole('dialog'); + // The `min(..., calc(100vw-24px))` term is what keeps the cap honest when + // the viewport shrinks under an open popover — Radix's own var goes stale + // on a plain window resize. + expect(dialog.className).toContain( + 'yv:max-w-[min(var(--radix-popover-content-available-width),calc(100vw-24px))]', + ); + }); + + it('makes the swatch row horizontally scrollable with a hidden scrollbar', () => { + render(); + const swatchRow = screen.getByRole('group', { name: 'Highlight colors' }); + expect(swatchRow.className).toContain('yv:overflow-x-auto'); + expect(swatchRow.className).toContain('yv:scrollbar-hide'); + // `min-w-0` is what lets the flex child shrink so overflow-x engages + // instead of the pill just growing wider. + expect(swatchRow.className).toContain('yv:min-w-0'); + }); + + it('keeps swatches from squishing when the row is capped', () => { + render(); + const applyButton = screen + .getAllByRole('button') + .find((btn) => btn.getAttribute('aria-label')?.includes('Apply'))!; + expect(applyButton.className).toContain('yv:shrink-0'); + }); + + // Regression guard for the live-browser defect: the scroll listener must be + // wired to the swatch row's actual mount. Radix's Portal/Presence commits + // the Content DOM in a deferred pass, so an effect keyed on `open` runs + // against a still-null ref and never re-runs — no listener, no fade. Keying + // the effect on the state-held node (callback ref) fixes it. jsdom has no + // layout, so we fake overflow metrics and drive a real scroll event: the + // mask only appears if the handler is actually attached and firing. This + // test is red against the pre-fix wiring (listener never attached). + it('engages the edge-fade mask on scroll once the row overflows', () => { + render( + (HIGHLIGHT_COLORS)} + selectedVerses={[1, 2, 3, 4, 5]} + highlightedVerses={{ + 1: HIGHLIGHT_COLORS[0], + 2: HIGHLIGHT_COLORS[1], + 3: HIGHLIGHT_COLORS[2], + 4: HIGHLIGHT_COLORS[3], + 5: HIGHLIGHT_COLORS[4], + }} + />, + ); + const swatchRow = screen.getByRole('group', { name: 'Highlight colors' }); + + // No layout in jsdom → no mask at rest. + expect(swatchRow.getAttribute('style') ?? '').not.toContain('linear-gradient'); + + // Fake an overflowing row scrolled to the middle, then fire a real scroll + // event. Only an attached handler will read these and apply the mask. + Object.defineProperty(swatchRow, 'scrollWidth', { configurable: true, value: 500 }); + Object.defineProperty(swatchRow, 'clientWidth', { configurable: true, value: 200 }); + Object.defineProperty(swatchRow, 'scrollLeft', { + configurable: true, + writable: true, + value: 120, + }); + fireEvent.scroll(swatchRow); + + // Both edges have hidden content → a two-sided fade mask is applied. + expect(swatchRow.getAttribute('style') ?? '').toContain('linear-gradient'); + }); + }); + + describe('computeScrollFade (edge-fade toggle logic)', () => { + // jsdom has no layout, so exercise the pure helper with mocked scroll + // metrics rather than asserting rendered pixels. + it('fades neither edge when the row fits (no overflow)', () => { + expect(computeScrollFade({ scrollLeft: 0, scrollWidth: 200, clientWidth: 200 })).toEqual({ + start: false, + end: false, + }); + }); + + it('fades only the end edge when scrolled fully to the start', () => { + expect(computeScrollFade({ scrollLeft: 0, scrollWidth: 500, clientWidth: 200 })).toEqual({ + start: false, + end: true, + }); + }); + + it('fades both edges when scrolled somewhere in the middle', () => { + expect(computeScrollFade({ scrollLeft: 120, scrollWidth: 500, clientWidth: 200 })).toEqual({ + start: true, + end: true, + }); + }); + + it('fades only the start edge when scrolled fully to the end', () => { + expect(computeScrollFade({ scrollLeft: 300, scrollWidth: 500, clientWidth: 200 })).toEqual({ + start: true, + end: false, + }); + }); + + it('tolerates sub-pixel slack at both bounds', () => { + // Half a pixel shy of each bound still counts as "at the edge". + expect(computeScrollFade({ scrollLeft: 0.5, scrollWidth: 500, clientWidth: 200 })).toEqual({ + start: false, + end: true, + }); + expect(computeScrollFade({ scrollLeft: 299.5, scrollWidth: 500, clientWidth: 200 })).toEqual({ + start: true, + end: false, + }); + }); + }); + describe('Highlights disabled (flag off)', () => { it('hides the color row and remove circles but keeps Copy / Share', () => { render( diff --git a/packages/ui/src/components/verse-action-popover.tsx b/packages/ui/src/components/verse-action-popover.tsx index ac91bf1f..543de90f 100644 --- a/packages/ui/src/components/verse-action-popover.tsx +++ b/packages/ui/src/components/verse-action-popover.tsx @@ -6,6 +6,7 @@ import { cn } from '../lib/utils'; import { BoxStackIcon } from './icons/box-stack'; import { BoxArrowUpIcon } from './icons/box-arrow-up'; import { CheckIcon } from './icons/check'; +import { hexToRgba, HIGHLIGHT_FILL_OPACITY_DARK } from './verse'; type Measurable = { getBoundingClientRect: () => DOMRect }; @@ -19,6 +20,47 @@ export const HIGHLIGHT_COLORS = ['fffe00', '5dff79', '00d6ff', 'ffc66f', 'ff95ef export type HighlightColor = (typeof HIGHLIGHT_COLORS)[number]; +/** Width, in px, of the fade applied at each overflowing edge of the swatch row. */ +const SCROLL_FADE_PX = 20; + +type ScrollMetrics = { scrollLeft: number; scrollWidth: number; clientWidth: number }; +type ScrollFade = { start: boolean; end: boolean }; + +/** + * Decide which edges of a horizontally scrollable row should fade. A fade only + * appears on an edge that has hidden content in that direction, so a row that + * fits (or is scrolled fully to one end) shows no fade on the exhausted side. + * Pure so it can be unit-tested without layout (jsdom reports zero sizes). + * Assumes LTR positive `scrollLeft`. + */ +export function computeScrollFade({ + scrollLeft, + scrollWidth, + clientWidth, +}: ScrollMetrics): ScrollFade { + const maxScroll = scrollWidth - clientWidth; + // Sub-pixel slack: rounding in real browsers can leave scrollLeft a hair off + // its bound, which would otherwise flicker a fade at a fully-scrolled edge. + const slack = 1; + if (maxScroll <= slack) return { start: false, end: false }; + return { + start: scrollLeft > slack, + end: scrollLeft < maxScroll - slack, + }; +} + +/** + * Build the `mask-image` that fades the overflowing edge(s). Returns `undefined` + * when neither edge fades so the element carries no mask at all (nothing to hint). + */ +function scrollFadeMask({ start, end }: ScrollFade): React.CSSProperties | undefined { + if (!start && !end) return undefined; + const left = start ? 'transparent' : '#000'; + const right = end ? 'transparent' : '#000'; + const mask = `linear-gradient(to right, ${left} 0, #000 ${SCROLL_FADE_PX}px, #000 calc(100% - ${SCROLL_FADE_PX}px), ${right} 100%)`; + return { maskImage: mask, WebkitMaskImage: mask }; +} + type VerseActionPopoverProps = { open: boolean; onOpenChange: (open: boolean) => void; @@ -50,31 +92,41 @@ type ColorCircleProps = { showRemove: boolean; label: string; onClick: () => void; + theme: 'light' | 'dark'; }; -function ColorCircle({ color, showRemove, label, onClick }: ColorCircleProps) { +function ColorCircle({ color, showRemove, label, onClick, theme }: ColorCircleProps) { + const isDark = theme === 'dark'; + // Preview the fill the way it will actually paint: full-strength in light mode, + // faded to the dark-mode alpha in dark mode (mirrors the applied-highlight + // treatment in verse.tsx). Light mode keeps the bare `#hex` so it serializes as + // a solid swatch. + const backgroundColor = isDark ? hexToRgba(color, HIGHLIGHT_FILL_OPACITY_DARK) : `#${color}`; + // Inner border matches the Figma "highlight stroke" (#121212 @ 20%), giving + // pale swatches definition on the light popover. On the dark popover a dark + // stroke would vanish against the dimmed fill, so flip it to a light stroke. + const border = isDark ? '1px solid rgba(255, 255, 255, 0.2)' : '1px solid rgba(18, 18, 18, 0.2)'; return ( ); } @@ -120,6 +172,28 @@ export const VerseActionPopover: FC = ({ }) => { const { t } = useTranslation(undefined, { i18n }); + // On open, Radix's FocusScope would autofocus the first swatch. Because the bar + // opens from a mouse/tap on non-focusable verse text, Chromium treats that + // delayed programmatic focus as keyboard-like and paints a `:focus-visible` + // ring on the swatch — a stray ring the user never asked for. Redirect the + // initial focus to the content container instead (see `onOpenAutoFocus`): focus + // still enters the popover (Escape closes; screen readers announce the dialog), + // but the ring only appears once the user actually Tabs to a swatch. + const contentRef = useRef(null); + + // The swatch row is capped to the viewport width (see the Content max-width + // below) and scrolls horizontally when it overflows. Track which edges have + // hidden content so we can fade only those edges, hinting there's more to + // scroll toward. Recomputed on scroll and on resize/content changes. + // + // The node is held in state (set via a callback ref) rather than a plain ref + // so the measuring effect is keyed on the element's actual mount/unmount. + // Radix's Portal/Presence commits the Content DOM in a deferred pass, so on + // the `open` flip the element isn't attached yet — an effect keyed on `open` + // alone would run against a null ref and never re-run to wire up the listener. + const [swatchRow, setSwatchRow] = useState(null); + const [scrollFade, setScrollFade] = useState({ start: false, end: false }); + // When the anchored verse scrolls out of the container, dock the bar to the // edge it exited through: scroll down (verse leaves the top) → dock top; scroll // up (verse leaves the bottom) → dock bottom. `null` = anchored (verse visible, @@ -226,15 +300,56 @@ export const VerseActionPopover: FC = ({ if (open) frozenView.current = live; const view = open ? live : frozenView.current; + // Measure the swatch row's overflow and keep the fade in sync. Keyed on the + // state-held node so it (re)attaches the listener the moment Radix commits the + // row into the DOM, and on the swatch count so a content-width change re-measures. + const swatchCount = view.colorCircles.length; + useEffect(() => { + const el = swatchRow; + if (!el) { + setScrollFade({ start: false, end: false }); + return; + } + const update = () => + setScrollFade( + computeScrollFade({ + scrollLeft: el.scrollLeft, + scrollWidth: el.scrollWidth, + clientWidth: el.clientWidth, + }), + ); + update(); + el.addEventListener('scroll', update, { passive: true }); + let observer: ResizeObserver | undefined; + if (typeof ResizeObserver !== 'undefined') { + observer = new ResizeObserver(update); + observer.observe(el); + } + return () => { + el.removeEventListener('scroll', update); + observer?.disconnect(); + }; + }, [swatchRow, swatchCount]); + return ( { + // Keep focus contained in the popover but off the first swatch: land + // it on the (non-tabbable) content element so no `:focus-visible` ring + // shows on pointer-open. The first Tab still moves to the first swatch, + // and because Tab is keyboard modality the ring correctly appears then. + event.preventDefault(); + contentRef.current?.focus({ preventScroll: true }); + }} onInteractOutside={(event) => { // Tapping another verse modifies the selection — it should re-anchor // the popover, not dismiss it. Only a tap truly outside the reader @@ -247,11 +362,21 @@ export const VerseActionPopover: FC = ({ side={view.side} sideOffset={view.sideOffset} align="center" + // Keep the pill off the screen edges; this also shrinks Radix's + // `--radix-popover-content-available-width`, which we cap to below. + collisionPadding={12} className={cn( 'yv:bg-card yv:text-popover-foreground', 'yv:rounded-full yv:drop-shadow-[0px_4.8432px_20px_rgba(0,0,0,0.19)]', 'yv:px-4 yv:py-2', 'yv:flex yv:items-center yv:gap-3', + // Never wider than the viewport (minus collision padding). When the + // swatch row can't fit, it scrolls inside instead of overflowing the + // screen. Radix sets `--radix-popover-content-available-width`, but + // only recomputes it on reposition — it goes stale on a plain window + // resize. `min()` with a live `100vw` term keeps the cap honest when + // the viewport shrinks under an open popover (24px = 2×collisionPadding). + 'yv:max-w-[min(var(--radix-popover-content-available-width),calc(100vw-24px))]', 'yv:z-50 yv:outline-hidden', 'yv:overflow-visible yv:relative', 'yv:origin-(--radix-popover-content-transform-origin)', @@ -289,7 +414,15 @@ export const VerseActionPopover: FC = ({ {highlightsEnabled && ( <>
@@ -298,18 +431,21 @@ export const VerseActionPopover: FC = ({ key={key} color={color} showRemove={showRemove} + theme={theme} label={showRemove ? t('clearHighlightAriaLabel') : t('applyHighlightAriaLabel')} onClick={() => (showRemove ? onClearHighlight(color) : onHighlight(color))} /> ))}
- {/* Separator */} -