diff --git a/.changeset/native-highlights-release.md b/.changeset/native-highlights-release.md index ae00a6a..9e3e7e9 100644 --- a/.changeset/native-highlights-release.md +++ b/.changeset/native-highlights-release.md @@ -13,7 +13,7 @@ Three native modules are new peer dependencies. They are autolinked, so a JS-onl npx expo install expo-network expo-clipboard expo-application ``` -`expo-network` is core's (the connectivity trigger for parked writes). `expo-clipboard` is UI's (the Copy fallback in the verse action sheet). `expo-application` is now a UI peer too — apps already using core have it. +`expo-network` is core's (the connectivity trigger for parked writes). `expo-clipboard` is UI's (the Copy fallback in the verse action sheet). `expo-application` is a new UI peer (the sign-in sheet's app display name) — core no longer depends on it. **The Bible reader's default serif font changes from Source Serif 4 to Untitled Serif**, YouVersion's brand serif, following the Web SDK. A DOM component's WebView now makes a new outbound request to `api.youversion.com` for the stylesheet plus woff2 fetches from `cdn.youversion.com`. There is no opt-out and no new prop; if those hosts are blocked, serif text falls back to Source Serif 4 with no layout break. Readers who had explicitly chosen Source Serif are migrated. Any other `fontFamily` you pass or persist is left untouched. @@ -74,7 +74,7 @@ The cached grant is a hint for choosing UI and skipping redundant prompts. The s One addition to the auth context: -- `getAccessToken()` — the accessor that reports whether the refresh worked. It refreshes only when the token is at or near expiry, cheap enough to await on every user gesture unlike `refreshNow()` which always hits the token endpoint, then resolves an `AccessTokenResult`: `{ status: 'ok', token, userId }`, or `{ status: 'unavailable', reason: 'signed-out' | 'refresh-failed' }`. It never rejects, makes no network call when there is no refresh token to spend, and concurrent callers join one refresh. The `userId` is read in the same synchronous block as the token, so a caller holding an identity it captured earlier can tell whether the token it just got still belongs to that user — `userInfo` read from a render lags the token by a render on sign-in. +- `getAccessToken(options?: GetAccessTokenOptions)` — the accessor that reports whether the refresh worked. It refreshes only when the token is at or near expiry, cheap enough to await on every user gesture unlike `refreshNow()` which always hits the token endpoint, then resolves an `AccessTokenResult`: `{ status: 'ok', token, userId }`, or `{ status: 'unavailable', reason: 'signed-out' | 'refresh-failed' }`. Pass `{ force: true }` after a 401 to mint unconditionally and learn whether that mint landed — a force caller joins any in-flight refresh, then mints again, so `ok` is a token minted after this call. A failed force is `refresh-failed` even if an unexpired leftover remains. It never rejects, makes no network call when there is no refresh token to spend, and concurrent non-force callers join one refresh. The `userId` is read in the same synchronous block as the token, so a caller holding an identity it captured earlier can tell whether the token it just got still belongs to that user — `userInfo` read from a render lags the token by a render on sign-in. `refresh-failed` leaves the tokens in storage: the session is intact and the user stays signed in. That matters because a token endpoint outage used to present to the user as a revoked permission. When the token was expired and the refresh failed for a reason that was not a revocation — a 5xx, a timeout, a captive portal — the write went out with the expired token anyway, came back 401, and the 401 read as a stale grant, so a valid `highlights` grant was invalidated and the user was asked to consent again; the re-consent minted with the same expired token and dead-ended as `not-permitted`. Both the highlights write path and `requestPermissions` now source their token from `getAccessToken()` and settle a `refresh-failed` as `transient` **without issuing the request**. @@ -111,4 +111,4 @@ Two new props carry selection across the bridge: ## Dependencies -The Web SDK dependencies move to 2.5.0 — `@youversion/platform-core` (core, from 2.3.0) and `@youversion/platform-react-ui` (UI, from 2.2.0), which brings `@youversion/platform-core` and `@youversion/platform-react-hooks` 2.5.0 with it, so a single copy of each resolves across the workspace. Beyond the serif font change noted above, it supplies the reader's controlled highlights mode, the data-exchange primitives behind the just-in-time grant, and a core `ApiClient` fix reading an empty-body 2xx (what a successful highlight DELETE returns) as success rather than failure. +The Web SDK dependencies move to 2.6.2 — `@youversion/platform-core` (core, from 2.3.0) and `@youversion/platform-react-ui` (UI, from 2.2.0), which brings `@youversion/platform-core` and `@youversion/platform-react-hooks` 2.6.2 with it, so a single copy of each resolves across the workspace. Beyond the serif font change noted above, it supplies the reader's controlled highlights mode, the data-exchange primitives behind the just-in-time grant, and a core `ApiClient` fix reading an empty-body 2xx (what a successful highlight DELETE returns) as success rather than failure. diff --git a/AGENTS.md b/AGENTS.md index ef52221..edc555b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,7 +116,7 @@ Read [ADR 0017](docs/adr/0017-native-verse-action-sheet.md) before you change an - The tray's edge fades are gated by `lib/verse-action-fade-gates.ts` (layer 1). `swatchTrayFadeGates({ trayWidth, contentWidth, scrollX })` answers both ends from one triple — leading on distance scrolled, trailing on distance remaining — with `FADE_GATE_PX` of slack so a fade retires at the edge it guards instead of staying up for sub-pixel overflow. The three measurements travel together on purpose; deriving either fade from one of them alone strands it on. - `onCopy` and `onShare` are native-only props on `BibleReader`. They fall back to `expo-clipboard` and RN `Share`. `shareData` rides in on `onVerseSelect`, so neither button costs a round-trip into the WebView. - The sheet is gated on `selection !== null && prompt === 'none' && !flow.isConfirming`, so it never competes with the sign-in or consent sheet. Displacement would call its `onClose`, which clears the selection a **Pending Highlight** is waiting on. -- Swatch presses route through core's `useHighlightPermissionFlow` for `apply`, and straight to `remove`. The reader adds only a sign-in prompt in front of the flow, because the flow calls `signIn()` with no UI of its own. That gate reads `auth !== null && !auth.isAuthenticated`. A `null` auth means the consumer configured none at all, which is not the same as signed out and must not raise a prompt. +- Swatch presses route through core's `useHighlightPermissionFlow` for `apply`, and straight to `remove`. The reader adds only a sign-in prompt in front of the flow, because the flow calls `signIn()` with no UI of its own. The gate is an `authGate`: `unconfigured` (`null` auth, no prompt), `settling` (`isLoading && !isAuthenticated`, hold the tap), `signed-out` (prompt), `ready` (apply). A `null` auth means the consumer configured none at all, which is not the same as signed out and must not raise a prompt. ### Version Picker Sheet @@ -173,7 +173,7 @@ Keep `apps/example/metro.config.js` minimal — just `getDefaultConfig(__dirname **UI** (`@youversion/platform-react-native-expo-ui`): `YouVersionProvider`, `BibleCard`, `BibleChapterPickerSheet`, `BibleReader`, `BibleReaderSettingsSheet`, `BibleTextView`, `BibleVersionPickerSheet`, `VerseOfTheDay`, and `YouVersionAuthButton`, plus `useSignOutGuard`, and types `BibleReaderHandle`, `HighlightWriteError`, plus the verse-selection payload types re-exported from the Web SDK (`BibleReaderVerseSelection`, `BibleReaderShareData`) so an `onVerseSelect` handler can be typed without depending on `@youversion/platform-react-ui` -**Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth` (its value carries `requestedPermissions` / `grantedPermissions` / `hasPermission` / `invalidatePermissions` / `requestPermissions` / `getAccessToken` alongside the sign-in surface), `useYVAuthOptional` (same value, `null` instead of a throw when `auth` is unconfigured — what SDK-internal callers that must work either way use), `useHighlights`, `useHighlightPermissionFlow`, `deriveServerColors`, `hasQueuedHighlightWrites`, `HIGHLIGHT_COLORS` / `isHighlightColor`, `mmkvStorage`, auth types (`AccessTokenResult`, `AuthConfig`, `AuthPermission`, `AuthScope`, `DataExchangeOutcome`, `DataExchangeFailureReason`, `YVUserInfo`), and highlights types (`Highlight`, `HighlightColor`, `HighlightScope`, `ServerColors`, `HighlightWriteOutcome` (`ok` / `queued` / `noop` / `error`), `HighlightWriteReason`, `HighlightsFetchError`, `UseHighlightsOptions`, `UseHighlightsResult`, `UseHighlightPermissionFlowResult`, `PermissionFlowError`, `PermissionFlowErrorReason`) +**Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth` (its value carries `requestedPermissions` / `grantedPermissions` / `hasPermission` / `invalidatePermissions` / `requestPermissions` / `getAccessToken` alongside the sign-in surface), `useYVAuthOptional` (same value, `null` instead of a throw when `auth` is unconfigured — what SDK-internal callers that must work either way use), `useHighlights`, `useHighlightPermissionFlow`, `deriveServerColors`, `hasQueuedHighlightWrites`, `HIGHLIGHT_COLORS` / `isHighlightColor`, `mmkvStorage`, auth types (`AccessTokenResult`, `GetAccessTokenOptions`, `AuthConfig`, `AuthPermission`, `AuthScope`, `DataExchangeOutcome`, `DataExchangeFailureReason`, `YVUserInfo`), and highlights types (`Highlight`, `HighlightColor`, `HighlightScope`, `ServerColors`, `HighlightWriteOutcome` (`ok` / `queued` / `noop` / `error`), `HighlightWriteReason`, `HighlightsFetchError`, `UseHighlightsOptions`, `UseHighlightsResult`, `UseHighlightPermissionFlowResult`, `PermissionFlowError`, `PermissionFlowErrorReason`) UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider`. Import Bible components from UI; import `useYVAuth` from core. @@ -199,8 +199,8 @@ UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider - `useYVAuth()` throws if `auth` was not configured on the provider. - `YouVersionAuthButton` (UI package) is the drop-in sign-in/sign-out button built on `useYVAuth`; use it for standard sign-in UI instead of hand-rolling a button. - Tokens in `expo-secure-store`; expiry and cached user info in MMKV (`packages/core/src/storage/`). -- `refreshNow()` always hits the token endpoint. `getAccessToken()` is the leeway-gated refresh, cheap enough to await on every user gesture, and the one a permission-sensitive pre-flight should use. Both are **single-flight by promise**: a second caller joins the in-flight refresh rather than returning early on the token that refresh exists to replace. Do not put that back to a boolean flag — the app foregrounding starts a refresh, and a tap a moment later would read the stale token and 401. -- `getAccessToken()` is the accessor that **reports whether the refresh worked**: it runs the leeway-gated single-flight refresh described above and resolves an `AccessTokenResult` — `{ status: 'ok', token, userId }`, or `{ status: 'unavailable', reason: 'signed-out' | 'refresh-failed' }`. The `userId` rides along because it is read in the **same synchronous block** as the token: the provider writes both refs together on sign-in, while a `userInfo` read through React state lags by a render, so a caller guarding on a captured identity must compare against this one or it will pass a check it should have failed. It never rejects, and `refresh-failed` keeps tokens in storage (session intact — a transient token-endpoint failure, not a sign-out). The highlights write path sources its token from it and settles `refresh-failed` as a `transient` outcome **without issuing the request** — before it existed, an expired token rode out to the API, 401'd, classified as `auth`, and `useHighlightPermissionFlow` misread that as a stale grant (invalidating it and re-prompting consent). +- `refreshNow()` always hits the token endpoint. `getAccessToken()` is the leeway-gated refresh, cheap enough to await on every user gesture, and the one a permission-sensitive pre-flight should use. Pass `{ force: true }` when a 401 has already proved the leeway read wrong — that hits the endpoint like `refreshNow` and reports whether the mint landed. Non-force callers are **single-flight by promise**: a second caller joins the in-flight refresh rather than returning early on the token that refresh exists to replace. A force caller joins that run, then starts its own mint, so `ok` means minted after this call — the drain needs a token minted after the 401, not whatever refresh was already in flight. Do not put the join back to a boolean flag — the app foregrounding starts a refresh, and a tap a moment later would read the stale token and 401. +- `getAccessToken()` is the accessor that **reports whether the refresh worked**: it runs the leeway-gated single-flight refresh described above and resolves an `AccessTokenResult` — `{ status: 'ok', token, userId }`, or `{ status: 'unavailable', reason: 'signed-out' | 'refresh-failed' }`. Without `force`, `ok` is not "freshly minted" — it may be an unexpired leftover. With `{ force: true }`, `ok` means the endpoint minted on this call (after joining any in-flight run), and `refresh-failed` includes a force that did not land even if an unexpired leftover remains. The `userId` rides along because it is read in the **same synchronous block** as the token: the provider writes both refs together on sign-in, while a `userInfo` read through React state lags by a render, so a caller guarding on a captured identity must compare against this one or it will pass a check it should have failed. It never rejects, and `refresh-failed` keeps tokens in storage (session intact — a transient token-endpoint failure, not a sign-out). The highlights write path sources its token from it and settles `refresh-failed` as a `transient` outcome **without issuing the request** — before it existed, an expired token rode out to the API, 401'd, classified as `auth`, and `useHighlightPermissionFlow` misread that as a stale grant (invalidating it and re-prompting consent). - OAuth browser session via `expo-web-browser`; redirect handling is app-owned (example: `apps/example/app/callback.tsx` + `Linking.createURL('callback')`). - Register the same `redirectUri` in the YouVersion Platform console as used in app code. @@ -216,7 +216,7 @@ UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider - Paint math lives in the pure, React-free `packages/core/src/highlights/optimistic.ts`, ported from the web highlights machine. `state.colors` is what the reader shows — server truth with unconfirmed edits already folded in, not a base plus an overlay. The colour-aware reconcile retirement rule is documented in [ADR 0013](docs/adr/0013-native-highlights-optimistic-layer.md); it reads like a bug in both directions and is defended only by its regression pair, so read the ADR before touching `shouldRetire`. That ADR's ownership tokens were retired by [ADR 0018](docs/adr/0018-highlight-write-queue.md) — the guarantee they gave is now a value comparison against the write queue. - Writes are persisted before they are sent. `packages/core/src/highlights/queue.ts` holds one entry per verse, `{ local, server }`, keyed per user + scope. A write that cannot reach the server, or that comes back 5xx, keeps its paint and resolves `{ status: 'queued' }` — those are the two failures that park. Only a server _refusal_ (401/403 or any other 4xx) reverts, using the entry's `server` side. **Cached Highlights are the paint, not raw server truth** — MMKV is written queue first, cache second, and the mount re-applies the queue over the cache to repair a crash between the two. See [ADR 0018](docs/adr/0018-highlight-write-queue.md). - Parked writes are sent by the **drain** (`highlights/drain.ts`), mounted at core's `YouVersionProvider` by `HighlightQueueDrainHost` inside `AuthProvider` — a parked write outlives the chapter that made it, and after a relaunch the queue is the only record of its scope. It wakes on mount, on a token change, on `AppState` returning to active, on the rising edge of `expo-network` connectivity, and on the two signals the write path raises directly (`drain-signals.ts`: a request reached the server, a write parked). Everything else is a per-verse in-memory backoff that widens on each consecutive failure and resets on success. Connectivity is a trigger, never a gate. -- The drain has exactly one drop path: a 401/403 earns an unconditional `refreshNow` and one retry, and a second auth refusal drops the entry and reverts the cache to the entry's `server` side. Nothing else drops — 5xx, non-auth 4xx, and unreachable all retry indefinitely. A refresh that is absent, throws, or ends the session drops nothing — the retry must go out under a token the refresh actually minted. The un-paint reaches a mounted reader through the queue's drop notification (`onWritesDropped`, raised only by `dropRejectedWrites`); a settling write already owns its own paint, so `dropWrites` stays silent. +- The drain has exactly one drop path: a 401/403 earns `getAccessToken({ force: true })` and one retry, and a second auth refusal drops the entry and reverts the cache to the entry's `server` side. Nothing else drops — 5xx, non-auth 4xx, and unreachable all retry indefinitely. A refresh that is absent, reports `refresh-failed`, throws, or ends the session drops nothing — the retry must go out under a token the refresh actually minted. The un-paint reaches a mounted reader through the queue's drop notification (`onWritesDropped`, raised only by `dropRejectedWrites`); a settling write already owns its own paint, so `dropWrites` stays silent. - `hasQueuedHighlightWrites(userId)` answers the one question sign-out asks the queue: does this user have writes the server never took? A plain MMKV prefix scan, read on that one gesture rather than subscribed to, and it never throws — an unreadable store answers "nothing to lose" rather than breaking the gesture that raises the prompt. Any key under the user's prefix counts, including a scope suffix `listQueuedScopes` would reject: the drain cannot send that entry, which makes it more certain to be lost, not less. - Sign-out purges the queue, in `clearAuthState` alongside the highlights cache and the grant cache (the revoked-refresh-token path routes through the same routine). `clearHighlightQueue()` drops **every** user's entries, not just the departing one's — one user is signed in at a time, so anything under another id was already left by a departure. Entries stay per-user keyed while signed in, and the drain re-reads auth per scope so a user change mid-pass cannot send the departed user's writes under the new token. - The drain skips verses a mounted `useHighlights` is currently sending (`highlights/claims.ts`, refcounted). Queue-first writes leave an entry in MMKV for the whole life of a write, so the queue alone cannot tell the drain what is already in hand. The deference is one-directional — the hook never waits on the drain. @@ -228,7 +228,7 @@ UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider - `useHighlightPermissionFlow({ versionId, book, chapter })` wraps `useHighlights` and guards **only `apply`** behind whatever the user is missing — sign-in, the `highlights` permission, or both. `remove` and everything else pass through untouched (a user with visible highlights already has the grant). It returns the whole `useHighlights` result plus `isConfirming` / `confirm()` / `decline()` / `flowError`. - The branch point, the exactly-once re-prompt bound, the in-memory pending highlight, and the choice of a hand-rolled reducer over `xstate` are all decided in [ADR 0016](docs/adr/0016-highlight-permission-flow.md). Read it before changing any of them; each has a cheaper-looking alternative that the ADR rejects for a stated reason. - State lives in the pure, React-free reducer in `packages/core/src/highlights/permission-flow.ts`. **Every event invalid for the current step is a no-op** — that is the mechanism that stops a browser round-trip landing after a `RESET` from resurrecting a discarded highlight, not defensive noise. The hook adds a generation token on top so a late continuation cannot resolve a superseded caller's promise. -- A pending highlight carries the `scope` it was tapped in, and that `scope` is **load-bearing**. The generation token only protects flows that already exist, so the two windows before one opens — the pre-flight `getAccessToken()` round-trip, and a straight-through write that comes back `auth` — compare the claimed scope against the current one before replaying. Both are regression-tested; verse numbers replayed into the wrong chapter paint text the user never selected. +- A pending highlight carries the `scope` it was tapped in, and that `scope` is **load-bearing**. The generation token only protects flows that already exist, so the windows before one opens — bootstrap settle (`isLoading && !isAuthenticated`), and a straight-through write that comes back `auth` — compare the claimed scope against the current one before replaying. Both are regression-tested; verse numbers replayed into the wrong chapter paint text the user never selected. `settleThenApply` is the one await in front of `apply`'s branch; the common case stays sync so the optimistic paint lands on the tap. - A scope change dispatches `RESET` during render (same "adjust state when props change" pattern as `use-highlights.ts`). - After awaiting `signIn()`, auth state is re-read via a **forced render** (`nextCommittedRender`), not straight off the ref: `signIn` resolves in a microtask while React schedules its re-render on a macrotask, so reading the ref immediately is guaranteed to be too early. The "signs in, then applies" test fails if that is removed. - Ordinary highlights deliberately **do not** go through the reducer — modelling every tap as an exclusive flow step would serialize concurrent writes that `useHighlights` supports. Only a flow is exclusive; an overlapping tap during one gets a `transient` outcome rather than being queued behind a browser session. @@ -244,7 +244,7 @@ UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider Native modules and app-owned framework packages are peer dependencies. Consumers must install peer dependencies from both `packages/ui/package.json` and `packages/core/package.json` with Expo-compatible versions. Expo SDK 56 apps should also include `@expo/dom-webview` for Expo DOM Components and `react-native-worklets` when using Reanimated 4. -The verse action sheet added two UI peers: `expo-clipboard` (the Copy fallback) and `expo-application` (the app's display name in the sign-in prompt). A consumer upgrading into this version must install `expo-clipboard` and rebuild the dev client. `expo-application` was already a core dependency, so no new autolinked module reaches an app that already had core. +The verse action sheet added two UI peers: `expo-clipboard` (the Copy fallback) and `expo-application` (the app's display name in the sign-in prompt). A consumer upgrading into this version must install both and rebuild the dev client. Core no longer depends on `expo-application` — it was removed when installation IDs stopped using the device id. The highlight write queue's drain added one core peer: `expo-network`, the rising-edge connectivity trigger. It is autolinked, so a consumer upgrading into this version must install it and rebuild the dev client — a JS-only reload leaves a `Cannot find native module` redbox. diff --git a/CONTEXT.md b/CONTEXT.md index c7d8014..e86b915 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -143,8 +143,8 @@ What an `apply` or `remove` resolves to: `ok` with the verses that landed, `queu _Avoid_: Branching on `message` (generic outside development builds); routing write failures through the hook's `error`; a separate `partial` status (the two verse arrays already say it); reading `ok` as "saved" and `queued` as a failure (both mean the paint stays) **Access Token Result**: -What `getAccessToken()` resolves to, and the only thing in the SDK that can tell a refresh that worked from one that did not — `refreshToken` swallows failure by design, so a caller reading the token afterwards cannot. Either `ok` with the `token` and the `userId` that owns it, or `unavailable` with a `reason`. The two reasons are different situations, not degrees of the same one: `signed-out` means there is no session, while `refresh-failed` means the session is intact and only the token endpoint is unreachable — tokens stay in storage and the user stays signed in. The `userId` rides along because it is read in the same synchronous block as the token; `userInfo` read through a render lags it, so a caller guarding on a captured identity that compares against the lagging one passes a check it should have failed. -_Avoid_: Collapsing `refresh-failed` into `signed-out` (it would sign out a user who is merely offline); treating an `ok` token as freshly minted (it may be an unexpired one no refresh was owed for); reading the owner from `userInfo` alongside a token from here +What `getAccessToken()` resolves to, and the only thing in the SDK that can tell a refresh that worked from one that did not — `refreshToken` swallows failure by design, so a caller reading the token afterwards cannot. Either `ok` with the `token` and the `userId` that owns it, or `unavailable` with a `reason`. The two reasons are different situations, not degrees of the same one: `signed-out` means there is no session, while `refresh-failed` means the session is intact and only the token endpoint is unreachable — tokens stay in storage and the user stays signed in. Without `{ force: true }`, `ok` is not a freshly minted token — it may be an unexpired leftover no refresh was owed for. With `force: true`, `ok` means the endpoint minted on this call: a force caller joins any in-flight refresh, then mints again, so the token is newer than the 401 that provoked the force. `refresh-failed` includes a force that did not land even if an unexpired leftover remains. The `userId` rides along because it is read in the same synchronous block as the token; `userInfo` read through a render lags it, so a caller guarding on a captured identity that compares against the lagging one passes a check it should have failed. +_Avoid_: Collapsing `refresh-failed` into `signed-out` (it would sign out a user who is merely offline); treating an unforced `ok` token as freshly minted; reading the owner from `userInfo` alongside a token from here **Granted Permissions**: What the user actually granted at sign-in, read off the OAuth **app redirect** and cached per user. A three-state signal, not a list: `null` = no `granted_permissions` key at all, so nothing was requested and nothing is known; `[]` = requested and **denied**; populated = granted. Requesting a permission (`AuthConfig.permissions`) is a separate thing from being granted it. Values the SDK does not recognize are kept verbatim rather than narrowed to the known permission union. diff --git a/apps/example/package.json b/apps/example/package.json index ce0cd2f..cad8000 100644 --- a/apps/example/package.json +++ b/apps/example/package.json @@ -24,6 +24,7 @@ "@youversion/platform-react-native-expo-core": "workspace:*", "@youversion/platform-react-native-expo-ui": "workspace:*", "expo": "56.0.12", + "expo-application": "56.0.3", "expo-build-properties": "56.0.20", "expo-clipboard": "56.0.4", "expo-dev-client": "56.0.20", diff --git a/docs/adr/0016-highlight-permission-flow.md b/docs/adr/0016-highlight-permission-flow.md index 69b6106..973c869 100644 --- a/docs/adr/0016-highlight-permission-flow.md +++ b/docs/adr/0016-highlight-permission-flow.md @@ -37,7 +37,7 @@ It runs inside `useHighlights.runWrite`, next to the existing `waitForAuthSettle `hasPermission` reads the local grant cache and needs no token, so nothing about the branch decision required the refresh to come first. `runWrite` already re-reads the current token at send time — deliberately, so a mid-write refresh does not fail the write — which is the same place the fresh one lands. -Two things follow. `apply` is now synchronous up to the branch, so the guard that compared `pending.scope` across the pre-flight await is gone: the window it covered no longer exists. And `remove`, plus any direct `useHighlights` consumer, gets the same protection `apply` used to get alone. +Two things follow. `apply` is synchronous up to the branch in the common case. The one exception is bootstrap: `hasPermission` is seeded from MMKV before `accessToken` lands, so writing on that hint while `isLoading && !isAuthenticated` still reverts if the session is gone. Wait only in that window. A token in hand is settled even if `isLoading` is still true. The claimed-scope guard returns for that await only. `remove`, plus any direct `useHighlights` consumer, still gets the same send-path refresh `apply` used to get alone. **Re-prompt exactly once, then go terminal.** @@ -53,6 +53,7 @@ The reducer carries a `retried` flag from `confirming` onward. A write refused w | ----------------------------------------------------- | --------------------------------------------- | | A live flow spans a scope change | Render-time `RESET` plus the generation token | | A straight-through write is out and comes back `auth` | Claimed scope versus the current scope | +| Bootstrap is `isLoading && !isAuthenticated` | Claimed scope versus the current scope | The second is the one a generation token cannot cover: no flow exists yet, so there is no waiting caller to abandon. @@ -70,4 +71,4 @@ The accepted residual is the one ADR 0014 already named: a grant the server disa `getAccessToken` joins an in-flight refresh rather than skipping it, so awaiting it does mean the token is current. A failed refresh still leaves the old token in place, which is why the corrective path exists at all and must not be removed as redundant. -A write now settles no faster than before — the refresh round-trip moved, it did not disappear. What changed is that the user stops waiting on it. Anything added in front of `apply`'s branch, or in front of the claim in `useHighlights.startWrite`, puts the delay back. +A write now settles no faster than before — the refresh round-trip moved, it did not disappear. What changed is that the user stops waiting on it. Anything added in front of `apply`'s branch in the common case, or in front of the claim in `useHighlights.startWrite`, puts the delay back. The bootstrap wait is the documented exception, and it is gated on `isLoading && !isAuthenticated`, never on `isLoading` alone. diff --git a/docs/adr/0017-native-verse-action-sheet.md b/docs/adr/0017-native-verse-action-sheet.md index 97ff37d..f682836 100644 --- a/docs/adr/0017-native-verse-action-sheet.md +++ b/docs/adr/0017-native-verse-action-sheet.md @@ -124,7 +124,7 @@ The reader calls `flow.apply(color, verses)` for an apply and `flow.highlights.r The reader keeps one thing the flow does not have: **a sign-in prompt**. `useHighlightPermissionFlow` calls `auth.signIn()` directly in its sign-in branch, and its only prompt state, `isConfirming`, is the **Data Exchange** consent. Handing a signed-out tap straight to `flow.apply` launches OAuth with no explanation of why. The reader instead holds the intent in a ref and shows `SignInWithYouVersionSheet`. On confirm it hands the intent to `flow.apply`. The flow then runs sign-in, falls through to consent if the grant is still missing, and writes, without the user reselecting the verse. Every dismissal path discards the intent. -The signed-out read is `auth !== null && !auth.isAuthenticated`, not `!auth?.isAuthenticated`. `useYVAuthOptional()` returns `null` when the consumer configured no `auth` at all. That is not the same as signed out, because there is nothing to sign in to. For a null auth, `flow.apply` warns once and falls through to the unguarded write, which reports `not-signed-in`. Prompting would open a sheet whose only outcome is the outcome the user already had. +The signed-out read is `auth !== null && !auth.isAuthenticated && !auth.isLoading`. `isLoading && !isAuthenticated` holds the tap until bootstrap settles — a stored session is not `isAuthenticated` until the token lands (`accessToken !== null`, not seeded `userInfo`). `useYVAuthOptional()` returns `null` when the consumer configured no `auth` at all. That is not the same as signed out, because there is nothing to sign in to. For a null auth, `flow.apply` warns once and falls through to the unguarded write, which reports `not-signed-in`. Prompting would open a sheet whose only outcome is the outcome the user already had. ### One sheet at a time, by construction @@ -145,7 +145,7 @@ The action sheet's `isOpen` is `selection !== null && prompt === 'none' && !flow - **Swatch labels do not name their color.** Web's two labels, `applyHighlightAriaLabel` and `clearHighlightAriaLabel`, do not either. Carrying the color means coining five color-name keys with no upstream source, which the localization rules forbid from this repo. A screen-reader user hears "Apply highlight" five times. The testIDs already carry the color. Fix this once the copy table has color names. The ticket already records accessibility criteria as blocked on the swatch aria-label i18n ticket. - `reference` falls back to the USFM book code until `useBooks` resolves inside the WebView. Selecting immediately after a chapter load is how that shows up. The fix is to hold the payload until books load, which is upstream work. - `clearSelectionSignal` adds a DOM prop update on every sheet exit. Android has a standing `DomWebView.injectJavaScript` rejection when a prop update reaches an unmounted WebView. This work does not cause that rejection, but it does make the rejection easier to hit. -- `expo-clipboard` is a new **peer dependency**. Consumers who take this version must install it and rebuild their dev client. `expo-application` is also now a UI peer, because the sign-in sheet reads the app's display name. Core already depended on it, so no new autolinked module reaches an app that already had core. +- `expo-clipboard` and `expo-application` are new **peer dependencies**. Consumers who take this version must install both and rebuild their dev client. `expo-application` is a UI peer because the sign-in sheet reads the app's display name. Core no longer depends on it. - The sheet is not exported, so its layout is not public API and can change without a breaking release. ## Amendment (2026-08-12): non-palette paint/clear (YPE-4494) diff --git a/docs/adr/0018-highlight-write-queue.md b/docs/adr/0018-highlight-write-queue.md index 1e1aa67..031ddd0 100644 --- a/docs/adr/0018-highlight-write-queue.md +++ b/docs/adr/0018-highlight-write-queue.md @@ -78,7 +78,7 @@ With no timestamps, any rule is a policy rather than a comparison. Local-wins pr A 401/403 that survives a forced token refresh and one retry is the server stating this write will never be accepted. Keeping it would leave the verse painted on this device forever, showing a highlight that exists nowhere else — and with no pending-state surface on the public API, the user could never learn it is not real. A permanent local-only phantom is worse than a silent un-paint, so the entry is dropped. This keeps auth entirely out of the queue's business, matching the tap-time rule that a 401/403 reverts and reports rather than parking. -The refresh is unconditional (`refreshNow`), not leeway-gated: the pass already ran `ensureFreshToken`, so a refusal means that read was wrong. Only a _second_ refusal drops, and only under a token the refresh genuinely minted. A refresh that is absent, throws, or ends the session the write belongs to drops nothing and takes the ordinary backoff — the drain must not be what decides a departed user's write was refused for good. +The refresh is unconditional (`getAccessToken({ force: true })`), not leeway-gated: the pass already ran `ensureFreshToken`, so a refusal means that read was wrong. Only a _second_ refusal drops, and only under a token the refresh genuinely minted. A refresh that is absent, reports `refresh-failed`, throws, or ends the session the write belongs to drops nothing and takes the ordinary backoff — the drain must not be what decides a departed user's write was refused for good. `refreshNow` cannot carry this: it never throws, and a silent failure leaves the leftover token in place, which the drain would then send again and treat a second 401 as definitive. Note that a dead session does not reach this path: `refreshToken` clears auth state on a revoked refresh token. Both it and `signOut` route through `clearAuthState`, which purges the queue alongside the highlights cache and the grant cache. The purge takes **every** user's entries, not just the departing one's — only a departure can leave an entry under an id that is not current, and one signed-in user at a time means anything else is already abandoned. The queue's distinct MMKV prefix means this is an explicit call rather than something inherited from a prefix match. diff --git a/packages/core/package.json b/packages/core/package.json index 5634aea..ddc7dd8 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -102,7 +102,7 @@ "jest-expo": "56.0.5" }, "dependencies": { - "@youversion/platform-core": "2.5.0", + "@youversion/platform-core": "2.6.2", "zod": "4.4.3" } } diff --git a/packages/core/src/auth/__tests__/auth-provider.test.tsx b/packages/core/src/auth/__tests__/auth-provider.test.tsx index ee513e5..978e048 100644 --- a/packages/core/src/auth/__tests__/auth-provider.test.tsx +++ b/packages/core/src/auth/__tests__/auth-provider.test.tsx @@ -838,6 +838,42 @@ describe('AuthProvider — getAccessToken', () => { }) }) + it('force:true hits the endpoint even when the token is beyond leeway', async () => { + mockLoadTokens.mockResolvedValue({ + accessToken: 'stored-access', + refreshToken: 'stored-refresh', + expiryDate: new Date(Date.now() + 60 * 60 * 1000), + }) + mockRefreshTokens.mockResolvedValue({ ...validTokens, access_token: 'forced-access' }) + + renderProvider() + await waitFor(() => expect(getText('isLoading')).toBe('false')) + + const result = await act(async () => latestAuth!.getAccessToken({ force: true })) + + expect(result).toEqual({ status: 'ok', token: 'forced-access', userId: null }) + expect(mockRefreshTokens).toHaveBeenCalled() + }) + + it('force:true reports refresh-failed when the mint does not land, even if a leftover is unexpired', async () => { + mockLoadTokens.mockResolvedValue({ + accessToken: 'stored-access', + refreshToken: 'stored-refresh', + expiryDate: new Date(Date.now() + 60 * 60 * 1000), + }) + mockRefreshTokens.mockRejectedValue(new Error('Network request failed')) + + renderProvider() + await waitFor(() => expect(getText('isLoading')).toBe('false')) + + const result = await act(async () => latestAuth!.getAccessToken({ force: true })) + + expect(result).toEqual({ status: 'unavailable', reason: 'refresh-failed' }) + expect(getText('isAuthenticated')).toBe('true') + expect(getText('accessToken')).toBe('stored-access') + expect(mockSaveTokens).not.toHaveBeenCalledWith(clearedTokens) + }) + it('joins an in-flight refresh: concurrent callers share one HTTP call and get the new token', async () => { mockLoadTokens.mockResolvedValue({ accessToken: 'expired-access', @@ -871,6 +907,38 @@ describe('AuthProvider — getAccessToken', () => { expect(await second).toEqual({ status: 'ok', token: 'new-access', userId: null }) expect(mockRefreshTokens).toHaveBeenCalledTimes(1) }) + + it('force:true remints after joining an in-flight refresh', async () => { + mockLoadTokens.mockResolvedValue({ + accessToken: 'expired-access', + refreshToken: 'r', + expiryDate: new Date(Date.now() - 1000), + }) + + let resolveFirst: (v: TokenResponse) => void = () => {} + mockRefreshTokens + .mockImplementationOnce( + () => + new Promise((r) => { + resolveFirst = r + }), + ) + .mockResolvedValueOnce({ ...validTokens, access_token: 'forced-access' }) + + renderProvider() + await waitFor(() => expect(mockRefreshTokens).toHaveBeenCalledTimes(1)) + await waitFor(() => expect(latestAuth).not.toBeNull()) + + const forced = latestAuth!.getAccessToken({ force: true }) + + await act(async () => { + resolveFirst({ ...validTokens, access_token: 'joined-access' }) + await forced + }) + + expect(await forced).toEqual({ status: 'ok', token: 'forced-access', userId: null }) + expect(mockRefreshTokens).toHaveBeenCalledTimes(2) + }) }) describe('AuthProvider — AppState wiring', () => { diff --git a/packages/core/src/auth/auth-context.tsx b/packages/core/src/auth/auth-context.tsx index 48a453b..34c9b85 100644 --- a/packages/core/src/auth/auth-context.tsx +++ b/packages/core/src/auth/auth-context.tsx @@ -4,9 +4,15 @@ import type { AuthPermission, YVUserInfo } from './types' /** * What {@link AuthContextValue.getAccessToken} resolves to. `refresh-failed` - * means the token is expired and the refresh did not land (endpoint down, - * network out) — the session itself is still intact, so treat it as transient, - * not as signed out. + * means the refresh did not land (endpoint down, network out) — the session + * itself is still intact, so treat it as transient, not as signed out. + * + * Without `{ force: true }`, `ok` is not "freshly minted": it may be an + * unexpired leftover no refresh was owed for. With `force: true`, `ok` means + * the endpoint minted on this call: a force caller joins any in-flight refresh, + * then mints again, so the token is newer than the 401 that provoked the force. + * `refresh-failed` includes a force that did not land even if an unexpired + * leftover remains. * * `userId` is whose token this is, read in the same synchronous block as the * token itself. A caller that captured an identity earlier must compare against @@ -18,6 +24,11 @@ export type AccessTokenResult = | { status: 'ok'; token: string; userId: string | null } | { status: 'unavailable'; reason: 'signed-out' | 'refresh-failed' } +export type GetAccessTokenOptions = { + /** Hit the token endpoint even when the current token is still inside leeway. */ + force?: boolean +} + export type AuthContextValue = { isAuthenticated: boolean accessToken: string | null @@ -28,26 +39,31 @@ export type AuthContextValue = { refreshNow: () => Promise /** * Resolve a token that is verifiably fresh, or say why one is unavailable. - * Refreshes **only if the token is at or near expiry**, unlike - * {@link refreshNow}, which always hits the token endpoint — cheap enough to - * await on every user gesture. + * Refreshes **only if the token is at or near expiry**, unless + * `{ force: true }` — that always hits the token endpoint, like + * {@link refreshNow}, and reports whether the mint landed. Cheap enough to + * await on every user gesture without `force`. * * It reports whether the refresh worked, so a caller can tell "refreshed" from * "still expired" and stop a doomed request before it 401s and gets misread as * a revoked grant. Exists so an expired token cannot be mistaken for a missing - * permission (Swift's `hasValidToken()` parity). + * permission (Swift's `hasValidToken()` parity). A forced call that fails + * reports `refresh-failed` even when an unexpired leftover is still in hand — + * that leftover is what a 401 just refused. * * Await it on the **send** path, immediately before an auth-sensitive request * — not in front of whatever the user just tapped. When a refresh is due this * costs a full token round-trip, so anything optimistic should have painted - * already. `useHighlights` is the worked example. + * already. `useHighlights` is the worked example. The highlight write-queue + * drain is the worked example of `{ force: true }`. * - * Single-flight: a refresh already in flight is **joined**, not skipped, so - * once this resolves the token is the current one. It never rejects, and a - * failed refresh also surfaces through {@link error}, exactly as the periodic - * refresh does. + * Single-flight for a non-force caller: a refresh already in flight is + * **joined**, not skipped, so once this resolves the token is the current one. + * A force caller joins that run, then starts its own mint. It never rejects, + * and a failed refresh also surfaces through {@link error}, exactly as the + * periodic refresh does. */ - getAccessToken: () => Promise + getAccessToken: (options?: GetAccessTokenOptions) => Promise isLoading: boolean /** * What the app **asked for** on its `auth` config — never what was granted. diff --git a/packages/core/src/auth/auth-provider.tsx b/packages/core/src/auth/auth-provider.tsx index b861c3d..65deb2f 100644 --- a/packages/core/src/auth/auth-provider.tsx +++ b/packages/core/src/auth/auth-provider.tsx @@ -5,7 +5,12 @@ import { toMessage } from '../error-message' import { clearHighlightQueue, clearHighlightsCache } from '../highlights' import { getOrSetInstallationId } from '../installation-id' import { mmkvStorage } from '../storage/mmkv-storage' -import { AuthContext, type AccessTokenResult, type AuthContextValue } from './auth-context' +import { + AuthContext, + type AccessTokenResult, + type AuthContextValue, + type GetAccessTokenOptions, +} from './auth-context' import { MMKV_AUTH_KEYS, REFRESH_LEEWAY_SECONDS } from './constants' import { requestDataExchange, type AuthIdentity, type DataExchangeOutcome } from './data-exchange' import { createDataExchangeApi } from './data-exchange-api' @@ -21,6 +26,10 @@ import { signInWithPKCE } from './pkce-flow' import { loadTokens, saveTokens, type StoredTokens } from './token-storage' import type { AuthConfig, AuthPermission, YVUserInfo } from './types' +// `getAccessToken({ force: true })` only reads `'failed'`. The other members +// are `refreshToken`'s own control flow (leeway skip, revoke, mint). +type RefreshOutcome = 'ok' | 'skipped' | 'failed' | 'signed-out' + // Stable empty reference, so an unconfigured `permissions` does not give the // context value a new identity on every render. const NO_PERMISSIONS: readonly AuthPermission[] = [] @@ -53,7 +62,7 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // second caller can *join* it. A caller that awaits `refreshToken` needs a // usable token when it resolves; a boolean flag can only tell it to give up // and carry on with the expired one it was trying to replace. - const refreshPromiseRef = useRef | null>(null) + const refreshPromiseRef = useRef | null>(null) // The access token as a ref, alongside the state, for the one read that has to // see past the render closure: data exchange refreshes before minting, and the // token it must send is the one that refresh just wrote, not the one this @@ -146,15 +155,15 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth }, [invalidatePermissions, setIdentity]) const refreshToken = useCallback( - async (options?: { force?: boolean }): Promise => { + async (options?: { force?: boolean }): Promise => { const currentRefreshToken = refreshTokenRef.current if (!currentRefreshToken) { - return + return 'signed-out' } const expiresAt = expiryRef.current?.getTime() ?? 0 if (!options?.force && expiresAt > Date.now() + REFRESH_LEEWAY_SECONDS * 1000) { - return + return 'skipped' } // Join an in-flight refresh rather than stepping over it. The trigger is @@ -163,27 +172,39 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // pre-flight on the very token this refresh exists to replace, and the // write that followed would 401. // - // A `force` caller joins too. It asked for a token minted now, and the run - // it joins was minted now. + // A `force` caller joins that run, then mints again. The drain needs a + // token minted *after* the 401 that provoked the force, not whatever + // refresh was already in flight when the refusal landed. const inFlight = refreshPromiseRef.current if (inFlight !== null) { - return inFlight + const joined = await inFlight + if (!options?.force) { + return joined + } } - const run = (async () => { + // Re-read after a join: the run we waited on can rotate the refresh token. + const tokenToSpend = refreshTokenRef.current + if (tokenToSpend === null) { + return 'signed-out' + } + + const run = (async (): Promise => { try { const response = await refreshTokens({ apiHost, appKey, - refreshToken: currentRefreshToken, + refreshToken: tokenToSpend, }) await setAuthState({ accessToken: response.access_token, refreshToken: response.refresh_token, expiryDate: new Date(Date.now() + Number(response.expires_in) * 1000), }) + return 'ok' } catch (e) { - if (e instanceof TokenEndpointError && e.isRevoked) { + const revoked = e instanceof TokenEndpointError && e.isRevoked + if (revoked) { // Clearing is best-effort: it ends in a Keychain write, which can // reject. Everything downstream of this refresh — getAccessToken, // requestPermissions — is documented never to throw, so a storage @@ -194,7 +215,10 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // In-memory state is already cleared; only the persisted copy lost. } } + // After `clearAuthState`, which nulls `error`. One write for both + // branches so a revoke and a failed mint report the same way. setError(e instanceof Error ? e : new Error(String(e))) + return revoked ? 'signed-out' : 'failed' } })() @@ -203,7 +227,7 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // that started the run clears it; joiners return the promise untouched. refreshPromiseRef.current = run try { - await run + return await run } finally { refreshPromiseRef.current = null } @@ -315,44 +339,55 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth await clearAuthState() }, [clearAuthState]) - const refreshNow = useCallback(() => refreshToken({ force: true }), [refreshToken]) + const refreshNow = useCallback(async () => { + await refreshToken({ force: true }) + }, [refreshToken]) const requestedPermissions = config.permissions ?? NO_PERMISSIONS - // The leeway-gated refresh with its outcome attached. `refreshToken` swallows - // failure by design, so a caller reading the token afterwards cannot tell "refreshed" + // The refresh with its outcome attached. `refreshToken` swallows failure by + // design, so a caller reading the token afterwards cannot tell "refreshed" // from "still expired" — this re-reads the refs it left behind and says which. - // Non-forced on purpose: the leeway gate already refreshes exactly when the - // token needs it, and joining an in-flight refresh comes free. - const getAccessToken = useCallback(async (): Promise => { - if (refreshTokenRef.current === null) { - return { status: 'unavailable', reason: 'signed-out' } - } + // Non-forced: the leeway gate already refreshes exactly when the token needs + // it. Forced: always hit the endpoint, and a failure is `refresh-failed` even + // if an unexpired leftover remains — that leftover is what a 401 just refused. + const getAccessToken = useCallback( + async (options?: GetAccessTokenOptions): Promise => { + if (refreshTokenRef.current === null) { + return { status: 'unavailable', reason: 'signed-out' } + } - await refreshToken() - - // Re-read the refs, never closure state: the refresh may have replaced the - // token, or found it revoked and cleared the session via clearAuthState. - // Token and owner in one synchronous block — `setAuthState`/`clearAuthState` - // write both, so a caller checking the owner it captured cannot be handed a - // token from the other side of a sign-in. - const token = accessTokenRef.current - const userId = userInfoRef.current?.id ?? null - if (refreshTokenRef.current === null || token === null) { - return { status: 'unavailable', reason: 'signed-out' } - } + const force = options?.force === true + const outcome = await refreshToken({ force }) + + // Re-read the refs, never closure state: the refresh may have replaced the + // token, or found it revoked and cleared the session via clearAuthState. + // Token and owner in one synchronous block — `setAuthState`/`clearAuthState` + // write both, so a caller checking the owner it captured cannot be handed a + // token from the other side of a sign-in. + const token = accessTokenRef.current + const userId = userInfoRef.current?.id ?? null + if (refreshTokenRef.current === null || token === null) { + return { status: 'unavailable', reason: 'signed-out' } + } - // Actual expiry, not the leeway window the refresh triggers on: a token - // inside the window is still one the server takes, and refusing it here - // would fail writes that would have succeeded. A corrupt stored expiry is - // NaN, which fails every comparison — hence the finite check. - const expiresAt = expiryRef.current?.getTime() ?? 0 - if (!Number.isFinite(expiresAt) || expiresAt <= Date.now()) { - return { status: 'unavailable', reason: 'refresh-failed' } - } + if (force && outcome === 'failed') { + return { status: 'unavailable', reason: 'refresh-failed' } + } - return { status: 'ok', token, userId } - }, [refreshToken]) + // Actual expiry, not the leeway window the refresh triggers on: a token + // inside the window is still one the server takes, and refusing it here + // would fail writes that would have succeeded. A corrupt stored expiry is + // NaN, which fails every comparison — hence the finite check. + const expiresAt = expiryRef.current?.getTime() ?? 0 + if (!Number.isFinite(expiresAt) || expiresAt <= Date.now()) { + return { status: 'unavailable', reason: 'refresh-failed' } + } + + return { status: 'ok', token, userId } + }, + [refreshToken], + ) const hasPermission = useCallback( (permission: AuthPermission) => grantedPermissions?.includes(permission) ?? false, diff --git a/packages/core/src/auth/index.ts b/packages/core/src/auth/index.ts index 7c0736d..16cf260 100644 --- a/packages/core/src/auth/index.ts +++ b/packages/core/src/auth/index.ts @@ -1,4 +1,4 @@ -export type { AccessTokenResult } from './auth-context' +export type { AccessTokenResult, GetAccessTokenOptions } from './auth-context' export type { DataExchangeFailureReason, DataExchangeOutcome } from './data-exchange' export type { AuthConfig, diff --git a/packages/core/src/highlights/__tests__/highlight-queue-drain-host.test.tsx b/packages/core/src/highlights/__tests__/highlight-queue-drain-host.test.tsx index a6ab9d5..7bb970f 100644 --- a/packages/core/src/highlights/__tests__/highlight-queue-drain-host.test.tsx +++ b/packages/core/src/highlights/__tests__/highlight-queue-drain-host.test.tsx @@ -52,7 +52,6 @@ beforeEach(() => { userInfo: { id: 'user-1' }, accessToken: 'token-1', getAccessToken: jest.fn(), - refreshNow: jest.fn(), }) mockStartDrain.mockReturnValue(drain) mockAddNetworkStateListener.mockImplementation((listener) => { @@ -81,7 +80,7 @@ describe('HighlightQueueDrainHost', () => { userId: 'user-1', accessToken: 'token-1', ensureFreshToken: expect.any(Function), - refreshNow: expect.any(Function), + getAccessToken: expect.any(Function), }) }) @@ -94,7 +93,7 @@ describe('HighlightQueueDrainHost', () => { userId: null, accessToken: null, ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, }) }) diff --git a/packages/core/src/highlights/__tests__/highlight-queue-drain.test.ts b/packages/core/src/highlights/__tests__/highlight-queue-drain.test.ts index b58f6ac..98d3e7c 100644 --- a/packages/core/src/highlights/__tests__/highlight-queue-drain.test.ts +++ b/packages/core/src/highlights/__tests__/highlight-queue-drain.test.ts @@ -80,12 +80,16 @@ function signedIn(overrides: Partial = {}): () => DrainAuth { userId: USER, accessToken: 'token-1', ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, ...overrides, } return () => auth } +function mintedToken(token = 'token-1'): NonNullable { + return jest.fn(async () => ({ status: 'ok' as const, token, userId: USER })) +} + function queueApply(scope: HighlightScope, verses: number[], color: string | null) { enqueueWrites({ userId: USER, scope, verses, color, currentColors: {} }) } @@ -274,17 +278,17 @@ describe('startHighlightQueueDrain', () => { ['the server answers a non-auth 4xx', UNPROCESSABLE], ])('keeps the entry, and so the paint, when %s', async (_case, error) => { queueApply(JHN3, [16], YELLOW) - const refreshNow = jest.fn(async () => undefined) + const getAccessToken = mintedToken() const { api, calls } = createApi({ onCreate: () => err(error) }) - const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ refreshNow }) }) + const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ getAccessToken }) }) drain.drainNow() await settle() drain.stop() // One attempt, no forced refresh: only an auth refusal earns a second look. expect(calls).toHaveLength(1) - expect(refreshNow).not.toHaveBeenCalled() + expect(getAccessToken).not.toHaveBeenCalled() expect(getQueuedWrites(USER, JHN3)).toEqual({ 16: { local: YELLOW, server: null } }) expect(getCachedHighlights(USER, JHN3)).toBeNull() }) @@ -298,17 +302,18 @@ describe('startHighlightQueueDrain', () => { it('mints a fresh token and states the write once more', async () => { queueApply(JHN3, [16], YELLOW) - const refreshNow = jest.fn(async () => undefined) + const getAccessToken = mintedToken('fresh') const { api, calls } = createApi({ onCreate: refusingOnce(() => ok({}) as CreateResult), }) - const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ refreshNow }) }) + const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ getAccessToken }) }) drain.drainNow() await settle() drain.stop() - expect(refreshNow).toHaveBeenCalledTimes(1) + expect(getAccessToken).toHaveBeenCalledTimes(1) + expect(getAccessToken).toHaveBeenCalledWith({ force: true }) expect(calls).toEqual([ { kind: 'create', passageId: 'JHN.3.16', color: YELLOW }, { kind: 'create', passageId: 'JHN.3.16', color: YELLOW }, @@ -325,8 +330,9 @@ describe('startHighlightQueueDrain', () => { userId: USER, accessToken: 'stale', ensureFreshToken: null, - refreshNow: async () => { + getAccessToken: async () => { auth = { ...auth, accessToken: 'fresh' } + return { status: 'ok', token: 'fresh', userId: USER } }, } const { api } = createApi({ onCreate: refusingOnce(() => ok({}) as CreateResult) }) @@ -345,15 +351,16 @@ describe('startHighlightQueueDrain', () => { ['403', FORBIDDEN], ])('drops the entry when a %s survives the forced refresh', async (_status, error) => { queueApply(JHN3, [16], YELLOW) - const refreshNow = jest.fn(async () => undefined) + const getAccessToken = mintedToken('fresh') const { api, calls } = createApi({ onCreate: () => err(error) }) - const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ refreshNow }) }) + const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ getAccessToken }) }) drain.drainNow() await settle() drain.stop() - expect(refreshNow).toHaveBeenCalledTimes(1) + expect(getAccessToken).toHaveBeenCalledTimes(1) + expect(getAccessToken).toHaveBeenCalledWith({ force: true }) expect(calls).toHaveLength(2) expect(getQueuedWrites(USER, JHN3)).toEqual({}) }) @@ -375,7 +382,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, - getAuth: signedIn({ refreshNow: jest.fn(async () => undefined) }), + getAuth: signedIn({ getAccessToken: mintedToken() }), }) drain.drainNow() await settle() @@ -394,7 +401,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, - getAuth: signedIn({ refreshNow: jest.fn(async () => undefined) }), + getAuth: signedIn({ getAccessToken: mintedToken() }), }) drain.drainNow() await settle() @@ -413,7 +420,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, - getAuth: signedIn({ refreshNow: jest.fn(async () => undefined) }), + getAuth: signedIn({ getAccessToken: mintedToken() }), }) drain.drainNow() await settle() @@ -429,7 +436,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, - getAuth: signedIn({ refreshNow: jest.fn(async () => undefined) }), + getAuth: signedIn({ getAccessToken: mintedToken() }), }) drain.drainNow() await settle() @@ -439,6 +446,24 @@ describe('startHighlightQueueDrain', () => { expect(getQueuedWrites(USER, JHN3)).toEqual({ 16: { local: YELLOW, server: null } }) }) + it('keeps the entry when the forced refresh reports refresh-failed', async () => { + queueApply(JHN3, [16], YELLOW) + const getAccessToken = jest.fn(async () => ({ + status: 'unavailable' as const, + reason: 'refresh-failed' as const, + })) + const { api, calls } = createApi({ onCreate: () => err(REFUSED) }) + + const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ getAccessToken }) }) + drain.drainNow() + await settle() + drain.stop() + + expect(getAccessToken).toHaveBeenCalledWith({ force: true }) + expect(calls).toHaveLength(1) + expect(getQueuedWrites(USER, JHN3)).toEqual({ 16: { local: YELLOW, server: null } }) + }) + it('keeps the entry when the forced refresh throws', async () => { queueApply(JHN3, [16], YELLOW) const { api, calls } = createApi({ onCreate: () => err(REFUSED) }) @@ -446,7 +471,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ - refreshNow: jest.fn(async () => { + getAccessToken: jest.fn(async () => { throw new Error('network down') }), }), @@ -463,7 +488,7 @@ describe('startHighlightQueueDrain', () => { queueApply(JHN3, [16], YELLOW) const { api, calls } = createApi({ onCreate: () => err(REFUSED) }) - const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ refreshNow: null }) }) + const drain = startHighlightQueueDrain({ api, getAuth: signedIn({ getAccessToken: null }) }) drain.drainNow() await settle() drain.stop() @@ -480,8 +505,9 @@ describe('startHighlightQueueDrain', () => { userId: USER, accessToken: 'token-1', ensureFreshToken: null, - refreshNow: async () => { - auth = { userId: null, accessToken: null, ensureFreshToken: null, refreshNow: null } + getAccessToken: async () => { + auth = { userId: null, accessToken: null, ensureFreshToken: null, getAccessToken: null } + return { status: 'unavailable', reason: 'signed-out' } }, } const { api, calls } = createApi({ onCreate: () => err(REFUSED) }) @@ -512,7 +538,7 @@ describe('startHighlightQueueDrain', () => { const drain = startHighlightQueueDrain({ api, - getAuth: signedIn({ refreshNow: jest.fn(async () => undefined) }), + getAuth: signedIn({ getAccessToken: mintedToken() }), }) drain.drainNow() await settle() @@ -592,7 +618,7 @@ describe('startHighlightQueueDrain', () => { userId: USER, accessToken: 'token-1', ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, } const drain = startHighlightQueueDrain({ api, getAuth: () => auth }) // Signs out during the refresh that precedes the first scope. @@ -604,10 +630,10 @@ describe('startHighlightQueueDrain', () => { userId: 'user-2', accessToken: 'token-2', ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, } }, - refreshNow: null, + getAccessToken: null, } drain.drainNow() await settle() @@ -624,7 +650,7 @@ describe('startHighlightQueueDrain', () => { userId: USER, accessToken: 'token-1', ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, } const { api, calls } = createApi({ onCreate: () => { @@ -632,7 +658,7 @@ describe('startHighlightQueueDrain', () => { userId: 'user-2', accessToken: 'token-2', ensureFreshToken: null, - refreshNow: null, + getAccessToken: null, } return ok({} as never) as CreateResult }, diff --git a/packages/core/src/highlights/__tests__/highlight-write-queue.test.tsx b/packages/core/src/highlights/__tests__/highlight-write-queue.test.tsx index 64c79c5..1b315c3 100644 --- a/packages/core/src/highlights/__tests__/highlight-write-queue.test.tsx +++ b/packages/core/src/highlights/__tests__/highlight-write-queue.test.tsx @@ -621,7 +621,7 @@ describe('a parked write the drain drops', () => { userId, accessToken: 'token-1', ensureFreshToken: null, - refreshNow: async () => undefined, + getAccessToken: async () => ({ status: 'ok' as const, token: 'token-1', userId }), }), }) await act(async () => { diff --git a/packages/core/src/highlights/__tests__/use-highlight-permission-flow.test.tsx b/packages/core/src/highlights/__tests__/use-highlight-permission-flow.test.tsx index 97bea50..a581e99 100644 --- a/packages/core/src/highlights/__tests__/use-highlight-permission-flow.test.tsx +++ b/packages/core/src/highlights/__tests__/use-highlight-permission-flow.test.tsx @@ -54,7 +54,7 @@ const mockSignIn = jest.fn, []>() const mockRequestPermissions = jest.fn, [readonly AuthPermission[]]>() const mockInvalidatePermissions = jest.fn() -type AuthState = { signedIn: boolean; permissions: string[] } +type AuthState = { signedIn: boolean; permissions: string[]; isLoading?: boolean } /** `null` models a provider with no `auth` configured at all. */ let currentAuth: AuthState | null = null @@ -87,7 +87,7 @@ function authValue(state: AuthState): AuthContextValue { ? ({ status: 'ok', token: 'token-1', userId: 'user-1' } as const) : ({ status: 'unavailable', reason: 'signed-out' } as const), ), - isLoading: false, + isLoading: state.isLoading ?? false, // An app that reaches this flow has asked for `highlights` — the reader // never mounts the fetch otherwise (`shouldFetchHighlights`). What the user // then granted is `state.permissions` below, which is what this suite @@ -203,6 +203,88 @@ beforeEach(() => { // ── The pre-flight ─────────────────────────────────────────────────────────── describe('pre-flight', () => { + it('does not write or sign in while auth is still loading', async () => { + currentAuth = { signedIn: false, permissions: [], isLoading: true } + + const { result } = renderFlow() + const { promise } = await startApply(result) + + expect(mockWriteApply).not.toHaveBeenCalled() + expect(mockSignIn).not.toHaveBeenCalled() + expect(result.current.isConfirming).toBe(false) + + let settled = false + void promise.then(() => { + settled = true + }) + await act(async () => undefined) + expect(settled).toBe(false) + }) + + it('writes once bootstrap settles with a grant, without opening sign-in', async () => { + currentAuth = { signedIn: false, permissions: [], isLoading: true } + + const { result } = renderFlow() + const { promise } = await startApply(result) + expect(mockWriteApply).not.toHaveBeenCalled() + + await act(async () => { + setAuth(signedInWithGrant) + }) + + await expect(promise).resolves.toEqual({ status: 'ok', verses: [16] }) + expect(mockWriteApply).toHaveBeenCalledWith(YELLOW, [16]) + expect(mockSignIn).not.toHaveBeenCalled() + expect(mockRequestPermissions).not.toHaveBeenCalled() + }) + + it('holds a cached-grant tap until bootstrap settles, instead of writing', async () => { + currentAuth = { signedIn: false, permissions: ['highlights'], isLoading: true } + + const { result } = renderFlow() + const { promise } = await startApply(result) + expect(mockWriteApply).not.toHaveBeenCalled() + + await act(async () => { + setAuth(signedOut) + }) + + expect(mockSignIn).toHaveBeenCalledTimes(1) + expect(mockWriteApply).not.toHaveBeenCalled() + await expect(promise).resolves.toEqual({ status: 'noop' }) + }) + + it('releases the wait once a token is in hand, even if isLoading is still true', async () => { + currentAuth = { signedIn: false, permissions: ['highlights'], isLoading: true } + + const { result } = renderFlow() + const { promise } = await startApply(result) + expect(mockWriteApply).not.toHaveBeenCalled() + + await act(async () => { + setAuth({ signedIn: true, permissions: ['highlights'], isLoading: true }) + }) + + await expect(promise).resolves.toEqual({ status: 'ok', verses: [16] }) + expect(mockWriteApply).toHaveBeenCalledWith(YELLOW, [16]) + expect(mockSignIn).not.toHaveBeenCalled() + }) + + it('starts sign-in once bootstrap settles signed out', async () => { + currentAuth = { signedIn: false, permissions: [], isLoading: true } + + const { result } = renderFlow() + const { promise } = await startApply(result) + + await act(async () => { + setAuth(signedOut) + }) + + expect(mockSignIn).toHaveBeenCalledTimes(1) + expect(mockWriteApply).not.toHaveBeenCalled() + await expect(promise).resolves.toEqual({ status: 'noop' }) + }) + it('writes straight through when the permission is already granted', async () => { const { result } = renderFlow() const { promise } = await startApply(result) diff --git a/packages/core/src/highlights/drain.ts b/packages/core/src/highlights/drain.ts index 614c4ee..ba8e2b9 100644 --- a/packages/core/src/highlights/drain.ts +++ b/packages/core/src/highlights/drain.ts @@ -6,6 +6,7 @@ * relaunch nothing in memory remembers the scope. */ +import type { AccessTokenResult, GetAccessTokenOptions } from '../auth' import { nextBackoffDelay } from './backoff' import { mergeCachedHighlights } from './cache' import { isWriteClaimed } from './claims' @@ -19,8 +20,8 @@ export type DrainAuth = { userId: string | null accessToken: string | null ensureFreshToken: (() => Promise) | null - /** Unconditional mint, for the one retry a refusal earns. */ - refreshNow: (() => Promise) | null + /** Outcome-reporting accessor. Forced mint for the one retry a refusal earns. */ + getAccessToken: ((options?: GetAccessTokenOptions) => Promise) | null } export type HighlightQueueDrain = { @@ -120,21 +121,24 @@ export function startHighlightQueueDrain(deps: { /** * A fresh token for the retry, or `null` if there is no route to one — no - * refresh to call, one that threw, or one that ended the session this write - * belongs to. Only a refusal of a genuinely minted token may revert. + * accessor, a refresh that failed or threw, or one that ended the session + * this write belongs to. Only a refusal of a genuinely minted token may revert. */ async function forcedToken(userId: string): Promise { - const refreshNow = getAuth().refreshNow - if (refreshNow === null) { + const getAccessToken = getAuth().getAccessToken + if (getAccessToken === null) { return null } + let result: AccessTokenResult try { - await refreshNow() + result = await getAccessToken({ force: true }) } catch { return null } - const next = getAuth() - return stopped || next.userId !== userId ? null : next.accessToken + if (result.status !== 'ok') { + return null + } + return stopped || result.userId !== userId ? null : result.token } async function sendColor( diff --git a/packages/core/src/highlights/highlight-queue-drain-host.tsx b/packages/core/src/highlights/highlight-queue-drain-host.tsx index 5cfd32d..a23981c 100644 --- a/packages/core/src/highlights/highlight-queue-drain-host.tsx +++ b/packages/core/src/highlights/highlight-queue-drain-host.tsx @@ -23,10 +23,11 @@ export default function HighlightQueueDrainHost() { const userId = auth?.userInfo?.id ?? null const accessToken = auth?.accessToken ?? null const getAccessToken = auth?.getAccessToken ?? null - const refreshNow = auth?.refreshNow ?? null // The drain wants the refresh side effect only — it re-reads the token per - // scope from this same context afterwards. + // scope from this same context afterwards. The forced retry uses the same + // accessor with `{ force: true }` so a silent mint failure cannot look like + // a freshly minted token. const ensureFreshToken = useMemo( () => getAccessToken === null @@ -37,11 +38,11 @@ export default function HighlightQueueDrainHost() { [getAccessToken], ) - const authRef = useRef({ userId, accessToken, ensureFreshToken, refreshNow }) + const authRef = useRef({ userId, accessToken, ensureFreshToken, getAccessToken }) // Declared before the effects that read it: effects run in order, so the drain // starts against real values rather than the mount-time snapshot. useEffect(() => { - authRef.current = { userId, accessToken, ensureFreshToken, refreshNow } + authRef.current = { userId, accessToken, ensureFreshToken, getAccessToken } }) const api = useMemo( diff --git a/packages/core/src/highlights/use-highlight-permission-flow.ts b/packages/core/src/highlights/use-highlight-permission-flow.ts index 79fc3b7..6212dcd 100644 --- a/packages/core/src/highlights/use-highlight-permission-flow.ts +++ b/packages/core/src/highlights/use-highlight-permission-flow.ts @@ -151,6 +151,8 @@ export function useHighlightPermissionFlow( // just awaited (see `nextCommittedRender`). const [, bumpRender] = useState(0) const renderWaitersRef = useRef<(() => void)[]>([]) + const authSettledWaitersRef = useRef<(() => void)[]>([]) + const isUnmountedRef = useRef(false) /** * Resolve whoever is waiting on a flow that ended without an answer for them — @@ -192,10 +194,20 @@ export function useHighlightPermissionFlow( for (const resolve of waiters) { resolve() } + + if (auth === null || auth.isAuthenticated || !auth.isLoading) { + const settled = authSettledWaitersRef.current + authSettledWaitersRef.current = [] + for (const resolve of settled) { + resolve() + } + } }) - useEffect( - () => () => { + useEffect(() => { + isUnmountedRef.current = false + return () => { + isUnmountedRef.current = true // On unmount, settle first (so any flushed continuation finds nothing to // answer) and then release the waiters, rather than leaving a caller's // promise pending for the lifetime of the app. @@ -205,9 +217,13 @@ export function useHighlightPermissionFlow( for (const resolve of waiters) { resolve() } - }, - [settleAbandoned], - ) + const settled = authSettledWaitersRef.current + authSettledWaitersRef.current = [] + for (const resolve of settled) { + resolve() + } + } + }, [settleAbandoned]) /** * Resolves after React has committed a render, so `authRef` reflects state that @@ -229,6 +245,19 @@ export function useHighlightPermissionFlow( [], ) + const waitForAuthNotLoading = useCallback((): Promise => { + const current = authRef.current + // Same gate as the reader and as `waitForAuthSettled`: a token in hand is + // settled even if `isLoading` is still true. A hung bootstrap refresh + // leaves that flag up indefinitely (`use-highlights.ts`). + if (current === null || current.isAuthenticated || !current.isLoading) { + return Promise.resolve() + } + return new Promise((resolve) => { + authSettledWaitersRef.current.push(resolve) + }) + }, []) + /** * Reduce and dispatch in one step, returning the next state synchronously. * @@ -455,6 +484,49 @@ export function useHighlightPermissionFlow( [startFlow], ) + const branchApply = useCallback( + ( + current: NonNullable, + color: string, + verses: number[], + scope: PendingHighlight['scope'], + ): Promise => { + if (current.hasPermission(HIGHLIGHTS_PERMISSION)) { + return applyThroughGrant(color, verses) + } + const pending: PendingHighlight = { color, verses, scope } + return startFlow( + { type: 'TAP', pending, branch: current.isAuthenticated ? 'consent' : 'sign-in' }, + pending, + ) + }, + [applyThroughGrant, startFlow], + ) + + /** + * Bootstrap is still settling. Do not treat that as signed-out (a stored + * session would get a false prompt) and do not write yet (a signed-out + * launch would paint, revert, and drop the tap). Re-read after settle. + */ + const settleThenApply = useCallback( + async (color: string, verses: number[]): Promise => { + const claimedScope = highlightsRef.current.scope + await waitForAuthNotLoading() + if (isUnmountedRef.current) { + return { status: 'noop' } + } + if (scopeKey(claimedScope) !== scopeKey(highlightsRef.current.scope)) { + return { status: 'noop' } + } + const after = authRef.current + if (after === null) { + return highlightsRef.current.apply(color, verses) + } + return branchApply(after, color, verses, claimedScope) + }, + [branchApply, waitForAuthNotLoading], + ) + // Deliberately not `async`. Every branch already returns a promise, and the // whole point of this function is that nothing is awaited before the write is // handed off, so an `async` wrapper here would only invite one back. @@ -466,6 +538,13 @@ export function useHighlightPermissionFlow( return highlightsRef.current.apply(color, verses) } + // Cached `hasPermission` is seeded before the token lands. Writing on + // that hint during bootstrap still reverts if the session is gone. Wait + // only while loading *and* unsigned — a token in hand is settled. + if (!current.isAuthenticated && current.isLoading) { + return settleThenApply(color, verses) + } + // Nothing is awaited in front of this branch, so the optimistic paint // inside `applyThroughGrant` lands in the same tick as the tap. // `hasPermission` reads the local grant cache, and the token freshness the @@ -473,20 +552,9 @@ export function useHighlightPermissionFlow( // the claim rather than in front of it (ADR 0016). Putting a refresh here // instead would make the common case wait on a token round-trip before a // single pixel changed. - if (current.hasPermission(HIGHLIGHTS_PERMISSION)) { - return applyThroughGrant(color, verses) - } - - // Records the passage the user actually tapped, so nothing replayed after - // sign-in or consent can paint verses onto text they never selected. - const pending: PendingHighlight = { color, verses, scope: highlightsRef.current.scope } - - return startFlow( - { type: 'TAP', pending, branch: current.isAuthenticated ? 'consent' : 'sign-in' }, - pending, - ) + return branchApply(current, color, verses, highlightsRef.current.scope) }, - [applyThroughGrant, startFlow], + [branchApply, settleThenApply], ) const confirm = useCallback((): void => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0166825..45d91b0 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -5,6 +5,7 @@ export { default as YouVersionProvider } from './youversion-provider' export { useYVAuth, useYVAuthOptional } from './auth' export type { AccessTokenResult, + GetAccessTokenOptions, AuthConfig, AuthPermission, AuthScope, diff --git a/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx b/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx index 24597c2..b148042 100644 --- a/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx +++ b/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx @@ -100,7 +100,7 @@ type AuthValue = NonNullable> * the passthrough provider mounts no `AuthProvider` — means something different * and is covered by its own case below. */ -function stubAuth(isAuthenticated: boolean) { +function stubAuth(isAuthenticated: boolean, isLoading = false) { const value: AuthValue = { isAuthenticated, accessToken: isAuthenticated ? 'test-token' : null, @@ -114,7 +114,7 @@ function stubAuth(isAuthenticated: boolean) { ? ({ status: 'ok', token: 'test-token', userId: null } as const) : ({ status: 'unavailable', reason: 'signed-out' } as const), ), - isLoading: false, + isLoading, requestedPermissions: ['highlights'], grantedPermissions: null, hasPermission: () => false, @@ -409,6 +409,97 @@ describe('BibleReader — the sign-in pre-step', () => { expect(rawRemove).toHaveBeenCalledWith(BLUE, [1, 2]) }) + it('holds the tap while auth is still loading', async () => { + stubAuth(false, true) + render(, { wrapper }) + + await selectVerses() + await press(`bible-verse-action-swatch-apply-${GREEN}`) + + expect(screen.queryByTestId('sign-in-with-youversion-sheet')).toBeNull() + expect(highlightPermissionFlowApply).not.toHaveBeenCalled() + }) + + it('opens sign-in once bootstrap settles signed out', async () => { + stubAuth(false, true) + const { rerender } = render(, { + wrapper, + }) + + await selectVerses() + await press(`bible-verse-action-swatch-apply-${GREEN}`) + + stubAuth(false) + await act(async () => { + rerender() + }) + + expect(screen.getByTestId('sign-in-with-youversion-sheet')).toBeTruthy() + expect(highlightPermissionFlowApply).not.toHaveBeenCalled() + }) + + it('applies once bootstrap settles signed in', async () => { + stubAuth(false, true) + const { rerender } = render(, { + wrapper, + }) + + await selectVerses() + await press(`bible-verse-action-swatch-apply-${GREEN}`) + + stubAuth(true) + await act(async () => { + rerender() + }) + + expect(screen.queryByTestId('sign-in-with-youversion-sheet')).toBeNull() + expect(highlightPermissionFlowApply).toHaveBeenCalledWith(GREEN, [1, 2]) + }) + + it('drops a held tap when the user selects different verses', async () => { + stubAuth(false, true) + const { rerender } = render(, { + wrapper, + }) + + await selectVerses() + await press(`bible-verse-action-swatch-apply-${GREEN}`) + + await selectVerses({ + ...SELECTION, + verses: [3], + passageIds: ['JHN.1.3'], + reference: 'John 1:3', + }) + + stubAuth(false) + await act(async () => { + rerender() + }) + + expect(screen.queryByTestId('sign-in-with-youversion-sheet')).toBeNull() + expect(highlightPermissionFlowApply).not.toHaveBeenCalled() + }) + + it('keeps a held tap when the action sheet clears the selection', async () => { + stubAuth(false, true) + const { rerender } = render(, { + wrapper, + }) + + await selectVerses() + await press(`bible-verse-action-swatch-apply-${GREEN}`) + await selectVerses({ ...SELECTION, verses: [], passageIds: [], reference: '' }) + + stubAuth(false) + await act(async () => { + rerender() + }) + + expect(screen.getByTestId('sign-in-with-youversion-sheet')).toBeTruthy() + expect(highlightPermissionFlowApply).not.toHaveBeenCalled() + }) + it('goes straight to the flow for a signed-in user', async () => { stubAuth(true) render(, { wrapper }) diff --git a/packages/ui/src/native/bible-reader.tsx b/packages/ui/src/native/bible-reader.tsx index e70dff1..340ed9b 100644 --- a/packages/ui/src/native/bible-reader.tsx +++ b/packages/ui/src/native/bible-reader.tsx @@ -17,7 +17,7 @@ import type { import * as Clipboard from 'expo-clipboard' import * as WebBrowser from 'expo-web-browser' import type { Ref } from 'react' -import { useCallback, useImperativeHandle, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState } from 'react' import { Platform, Share, StyleSheet, View } from 'react-native' import { useSafeAreaInsets } from 'react-native-safe-area-context' import { useShallow } from 'zustand/react/shallow' @@ -78,6 +78,18 @@ type PendingSwatchIntent = { color: HighlightColor; verses: number[]; scope: Hig */ type PromptState = { kind: 'none' } | { kind: 'sign-in'; scope: HighlightScope } +type AuthGate = 'unconfigured' | 'settling' | 'signed-out' | 'ready' + +function resolveAuthGate(auth: ReturnType): AuthGate { + if (auth === null) return 'unconfigured' + // A token in hand is ready even if `isLoading` is still true. `isAuthenticated` + // is `accessToken !== null`, not the seeded `userInfo`. A stored session is + // therefore *not* authenticated during the loading window. + if (auth.isAuthenticated) return 'ready' + if (auth.isLoading) return 'settling' + return 'signed-out' +} + /** Stable identity, so the render-time discard cannot re-trigger itself. */ const NO_PROMPT: PromptState = { kind: 'none' } @@ -312,10 +324,16 @@ export function BibleReader({ const handleVerseSelect = useCallback( async (next: BibleReaderVerseSelection) => { + // A new non-empty selection is the user picking other verses. Drop a + // held tap so settle cannot prompt for the old ones. An empty payload is + // our own `closeVerseActions` after the swatch press — keep the hold. + if (next.verses.length > 0 && prompt.kind === 'none' && pendingIntentRef.current !== null) { + pendingIntentRef.current = null + } setVerseSelection(next.verses.length > 0 ? next : null) await onVerseSelect?.(next) }, - [onVerseSelect], + [onVerseSelect, prompt.kind], ) const closeVerseActions = useCallback(() => { @@ -335,10 +353,20 @@ export function BibleReader({ ) const applyHighlight = highlightPermissionFlow.apply + const authGate = resolveAuthGate(auth) - // A `null` auth means the consumer configured no auth at all, which is not the - // same as signed out. There is nothing to sign in to, so there is no prompt. - const needsSignIn = auth !== null && !auth.isAuthenticated + const replayPendingIntent = useCallback(() => { + const pending = pendingIntentRef.current + pendingIntentRef.current = null + if (!pending) return + // Backstop for the during-render discard above. A confirm that races a + // controlled location change must not hand verse numbers to the current + // location-scoped flow. + if (!sameScope(pending.scope, { versionId, book, chapter })) return + void applyHighlight(pending.color, pending.verses).then((outcome) => + reportHighlightWriteError(outcome, onHighlightError), + ) + }, [applyHighlight, onHighlightError, versionId, book, chapter]) const handleSwatchPress = useCallback( (swatch: VerseActionSwatch) => { @@ -354,29 +382,44 @@ export function BibleReader({ ) return } - if (needsSignIn) { - // The flow calls `signIn()` with no UI of its own, so the reader owns - // this pre-step: hold the intent, ask, hand it over on confirm. - pendingIntentRef.current = { - color: swatch.color, - verses, - scope: { versionId, book, chapter }, + switch (authGate) { + case 'settling': + case 'signed-out': { + // The flow calls `signIn()` with no UI of its own, so the reader owns + // this pre-step: hold the intent, ask, hand it over on confirm. + // While bootstrap is still settling, hold the intent and wait. Opening + // the sheet now would prompt a stored session; applying now would drop + // a signed-out tap after the write reverts. + pendingIntentRef.current = { + color: swatch.color, + verses, + scope: { versionId, book, chapter }, + } + if (authGate === 'signed-out') { + setPrompt({ kind: 'sign-in', scope: { versionId, book, chapter } }) + } + return + } + case 'unconfigured': + case 'ready': + // Fire-and-forget: the paint is optimistic inside `useHighlights`, so + // the verse changes color on this frame instead of after the round-trip. + void applyHighlight(swatch.color, verses).then((outcome) => + reportHighlightWriteError(outcome, onHighlightError), + ) + return + default: { + const _exhaustive: never = authGate + return _exhaustive } - setPrompt({ kind: 'sign-in', scope: { versionId, book, chapter } }) - return } - // Fire-and-forget: the paint is optimistic inside `useHighlights`, so the - // verse changes color on this frame instead of after the round-trip. - void applyHighlight(swatch.color, verses).then((outcome) => - reportHighlightWriteError(outcome, onHighlightError), - ) }, [ verseSelection, closeVerseActions, removeHighlight, applyHighlight, - needsSignIn, + authGate, onHighlightError, versionId, book, @@ -385,20 +428,11 @@ export function BibleReader({ ) const handleSignInConfirm = useCallback(() => { - const pending = pendingIntentRef.current - pendingIntentRef.current = null setPrompt(NO_PROMPT) // Straight back into the flow, which signs in, asks for consent if the grant // is still missing, and writes. The user never reselects the verse. - if (!pending) return - // Backstop for the during-render discard above. A confirm that races a - // controlled location change must not hand verse numbers to the current - // location-scoped flow. - if (!sameScope(pending.scope, { versionId, book, chapter })) return - void applyHighlight(pending.color, pending.verses).then((outcome) => - reportHighlightWriteError(outcome, onHighlightError), - ) - }, [applyHighlight, onHighlightError, versionId, book, chapter]) + replayPendingIntent() + }, [replayPendingIntent]) // "No Thanks", a swipe-down, a backdrop tap, and displacement all land here. // Every one discards the intent, and nothing is written. @@ -407,6 +441,29 @@ export function BibleReader({ setPrompt(NO_PROMPT) }, []) + useEffect(() => { + if (authGate === 'unconfigured' || authGate === 'settling') return + if (prompt.kind !== 'none') return + const pending = pendingIntentRef.current + if (pending === null) return + if (!sameScope(pending.scope, { versionId, book, chapter })) { + pendingIntentRef.current = null + return + } + switch (authGate) { + case 'signed-out': + setPrompt({ kind: 'sign-in', scope: pending.scope }) + return + case 'ready': + replayPendingIntent() + return + default: { + const _exhaustive: never = authGate + return _exhaustive + } + } + }, [authGate, prompt.kind, versionId, book, chapter, replayPendingIntent]) + const handleOpenBibleThemeSettings = useCallback(() => { setIsSettingsSheetOpen(true) }, []) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8198a35..f9958a2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -56,6 +56,9 @@ importers: expo: specifier: 56.0.12 version: 56.0.12(@babel/core@7.29.0)(@expo/dom-webview@56.0.5)(@expo/metro-runtime@56.0.15)(expo-router@56.2.11)(react-dom@19.2.3(react@19.2.3))(react-native-web@0.21.2(react-dom@19.2.3(react@19.2.3))(react@19.2.3))(react-native-webview@13.16.1(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3))(react-native-worklets@0.8.3(@babel/core@7.29.0)(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3)(typescript@6.0.3) + expo-application: + specifier: 56.0.3 + version: 56.0.3(expo@56.0.12) expo-build-properties: specifier: 56.0.20 version: 56.0.20(expo@56.0.12) @@ -136,8 +139,8 @@ importers: packages/core: dependencies: '@youversion/platform-core': - specifier: 2.5.0 - version: 2.5.0 + specifier: 2.6.2 + version: 2.6.2 expo: specifier: '>=56.0.0 <57.0.0' version: 56.0.12(@babel/core@7.29.0)(@expo/dom-webview@56.0.6)(@expo/metro-runtime@56.0.15)(expo-router@56.2.11)(react-dom@19.2.5(react@19.2.5))(react-native-web@0.21.2(react-dom@19.2.5(react@19.2.5))(react@19.2.5))(react-native-webview@13.16.1(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5))(react-native-worklets@0.8.3(@babel/core@7.29.0)(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5)(typescript@6.0.3) @@ -3000,14 +3003,6 @@ packages: xstate: optional: true - '@youversion/platform-core@2.5.0': - resolution: {integrity: sha512-hcmM0LQ+r00CkJBDAwh9yRGoxbucl9D4rLCJa6BaCOJ7c7+f9tQ61JaIIUO0/XqO71mZu+TCVDazXVlM9t5WLw==} - peerDependencies: - linkedom: ^0.18.12 - peerDependenciesMeta: - linkedom: - optional: true - '@youversion/platform-core@2.6.2': resolution: {integrity: sha512-es6t2loTEODsaCbY6NA+gE7YrrA49anTh0MtyiurbWzTHALn+ultnT3Ok0FrjJ7fq87d90eDWgINHnUlym1dmA==} peerDependencies: @@ -10705,10 +10700,6 @@ snapshots: transitivePeerDependencies: - '@types/react' - '@youversion/platform-core@2.5.0': - dependencies: - zod: 4.1.12 - '@youversion/platform-core@2.6.2': dependencies: zod: 4.1.12 @@ -12029,7 +12020,7 @@ snapshots: expo-application@56.0.3(expo@56.0.12): dependencies: - expo: 56.0.12(@babel/core@7.29.0)(@expo/dom-webview@56.0.5)(@expo/metro-runtime@56.0.15)(expo-router@56.2.11)(react-dom@19.2.5(react@19.2.5))(react-native-web@0.21.2(react-dom@19.2.5(react@19.2.5))(react@19.2.5))(react-native-webview@13.16.1(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5))(react-native-worklets@0.8.3(@babel/core@7.29.0)(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.5))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.5))(react@19.2.5)(typescript@6.0.3) + expo: 56.0.12(@babel/core@7.29.0)(@expo/dom-webview@56.0.5)(@expo/metro-runtime@56.0.15)(expo-router@56.2.11)(react-dom@19.2.3(react@19.2.3))(react-native-web@0.21.2(react-dom@19.2.3(react@19.2.3))(react@19.2.3))(react-native-webview@13.16.1(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3))(react-native-worklets@0.8.3(@babel/core@7.29.0)(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3))(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3)(typescript@6.0.3) expo-asset@56.0.19(expo@56.0.12)(react-native@0.85.3(@babel/core@7.29.0)(@react-native/jest-preset@0.85.3(@babel/core@7.29.0)(react@19.2.3))(@react-native/metro-config@0.85.3(@babel/core@7.29.0))(@types/react@19.2.14)(react@19.2.3))(react@19.2.3)(typescript@6.0.3): dependencies: