refactor(api): narrow the published surface - #135
Conversation
- Remove `ensureFreshToken` from the auth context. `getAccessToken` runs the identical leeway-gated single-flight refresh and reports whether it worked, so the side-effect-only variant was strictly less useful. `HighlightQueueDrainHost` derives the drain's internal `DrainAuth.ensureFreshToken` from it; `drain.ts` is unchanged. - Stop exporting `SignOutGuardAuth`. It only named `useSignOutGuard`'s parameter, which structural inference already covers. - Stop exporting `isValidHighlightHex` and `KnownAuthPermission`. - Update AGENTS.md, ADR 0016, the READMEs, and the changeset. None of these shipped — all are new on this branch inside unreleased minors, so no changeset entry is needed for the removals.
cameronapak
left a comment
There was a problem hiding this comment.
Standards
No hard AGENTS violations. The four names are off the public barrels. Docs and the changeset drop those names. No extra changeset is needed. The names did not ship.
Judgement (Fowler smells):
- Middle Man (justified).
HighlightQueueDrainHostwrapsgetAccessTokenin a voidensureFreshToken.DrainAuthstill wants() => Promise<void>.drain.tsstays unchanged. - Duplicated Code.
isValidHighlightHexis copied intoverse-action-swatches.ts. The public core export is gone. A deep import is blocked. The hex rule can drift. - Divergent Change. The PR title is "narrow the published surface". The READMEs also add offline,
onHighlightError,refreshHighlights, and sign-out tutorials.
Spec
The spec asks for four un-exports, then a docs update:
- Remove
ensureFreshTokenfrom the auth context. - Stop the export of
SignOutGuardAuth,isValidHighlightHex, andKnownAuthPermission. - Update AGENTS.md, ADR 0016, the READMEs, and the changeset.
- Do not add a changeset entry for the removals. The names did not ship.
public barrels
|-- core/index.ts
| - ensureFreshToken # also off AuthContextValue
| - isValidHighlightHex
| - KnownAuthPermission
`-- ui/index.ts + native/index.ts
- SignOutGuardAuth
still internal
|-- DrainAuth.ensureFreshToken # void wrapper over getAccessToken
|-- highlights/isValidHighlightHex
`-- types.KnownAuthPermission
(a) Missing / partial: none.
The four names are off the public barrels. AuthContextValue no longer defines ensureFreshToken. The drain host derives a void wrapper from getAccessToken. drain.ts is unchanged. SignOutGuardAuth is file-local. isValidHighlightHex stays internal. KnownAuthPermission stays behind AuthPermission.
(b) Scope creep: the spec asked the docs to show the four un-exports. This PR also adds consumer tutorials that are not those four names (offline / queued, onHighlightError, refreshHighlights, useHighlightPermissionFlow). The code change is in scope. The docs and changeset are a highlights-feature dump.
(c) Implemented but wrong: none. The drain wrapper discards AccessTokenResult as specified. getAccessToken still owns the leeway-gated single-flight refresh.
Standards: 0 hard, 3 judgement (worst: Duplicated Code on the hex helper). Spec: 0 missing/wrong, 1 scope-creep (worst: README tutorials outside the four removals).
Code Reviewer bot, sent on behalf of Cam.
| /** Six-digit hex, no `#`. Case-insensitive at the boundary. */ | ||
| const HIGHLIGHT_HEX_PATTERN = /^[0-9a-f]{6}$/i | ||
|
|
||
| function isValidHighlightHex(color: string): boolean { |
There was a problem hiding this comment.
nitpick (non-blocking): This hex check is a copy of the helper in paint-projection.ts.
The public export is gone, so a deep import is blocked. If the hex rule changes, these two copies can drift.
Code Reviewer bot, sent on behalf of Cam.
There was a problem hiding this comment.
This is a trade off, do we want this to be publicly accessible as part of the public API or would we rather keep both helpers?
I dont think this function should change much if at all.
|
|
||
| On web, `BibleReader` keeps the React Web SDK's verse action popover, because native bottom sheets do not exist there. Its Copy and Share work. Its color swatches do not write. | ||
|
|
||
| #### Highlights made offline |
There was a problem hiding this comment.
note (non-blocking): The spec asked the docs to show the four un-exports.
This section (and the later sign-out / refresh tutorials) is extra consumer surface. The text is accurate. It is a second job in this PR.
Code Reviewer bot, sent on behalf of Cam.
ensureFreshTokenfrom the auth context.getAccessTokenruns the identical leeway-gated single-flight refresh and reports whether it worked, so the side-effect-only variant was strictly less useful.HighlightQueueDrainHostderives the drain's internalDrainAuth.ensureFreshTokenfrom it;drain.tsis unchanged.SignOutGuardAuth. It only nameduseSignOutGuard's parameter, which structural inference already covers.isValidHighlightHexandKnownAuthPermission.None of these shipped — all are new on this branch inside unreleased minors, so no changeset entry is needed for the removals.
Greptile Summary
This PR narrows the unreleased public API while preserving the queue drain’s internal refresh interface.
ensureFreshToken,KnownAuthPermission,isValidHighlightHex, andSignOutGuardAuthfrom their public surfaces.getAccessToken()internally.Confidence Score: 5/5
The PR appears safe to merge, with no concrete changed-code defect identified.
The narrowed symbols were confined to unreleased branch work, and the replacement queue-drain adapter preserves the prior refresh behavior while the private hex validator remains equivalent to core’s validation contract.
Important Files Changed
ensureFreshTokenthrough the auth context without changing the underlying refresh implementation.getAccessTokenwith behavior equivalent to the removed method.Reviews (1): Last reviewed commit: "refactor(api): narrow the published surf..." | Re-trigger Greptile
Context used (3)