PT-4369: Gate automatic sync on first-run consent - #2700
katherinejensen00 wants to merge 8 commits into
Conversation
1796c61 to
a61b1db
Compare
a61b1db to
d4a25a2
Compare
a634274 to
8d46d94
Compare
8d46d94 to
9c5a7cd
Compare
jolierabideau
left a comment
jolierabideau
left a comment
There was a problem hiding this comment.
Review notes — PT-4369 gating
Reviewed at 9c5a7cd against merge-base d0786b6. Read the ADR and the new rule file first, which
made this much faster — thank you for both. No blockers from me. The fix is real: I grepped for
every automatic sync starter (syncProjects, sendReceiveProjects, runScheduledSessionSync,
syncOnProjectSwitch, syncOpenProjects) and found exactly four in core, all now gated;
finalizeProjectSwitch inherits by delegation, and the user-initiated syncs are correctly left
alone. Fail-closed logic is right at every gate (literal === true, so rejected/undefined/
non-boolean/PlatformError reads all skip — no !== false anywhere), each gate has a test with a
positive control, and the power-mode regression test really does pin what it says it does: hoisting
the gate makes requestNoRetry never fire.
Most of what follows is product questions and scope, not correctness.
Worth deciding before merge
1. "Don't sync yet" opens all four gates immediately. Declining calls completeFirstRun() →
markFirstRunComplete() → platform.firstRunComplete = true, which is the one flag every gate
reads. So a fresh user who declines and then closes a window or quits in the same session gets
their open writable projects Send/Received. None of the three non-startup gates reads
platform.syncOnStartup either, so the new toggle can't suppress them. The ADR is candid that this
is the epic owner's ruling and names the fix if it must hold (a session-scoped declined flag) — so
I'm not calling it an oversight. But PT-4369's DoD says "consent gating verified for both 'Sync' and
'Don't sync yet' choices", so I'd want that sign-off explicit rather than discovered in an ADR
paragraph, and the ticket wording amended if the current behaviour is intended.
Independent of that call: completeFirstRun's TSDoc says the decline "defers only this session's
sync", which is the opposite of what the code does. That one needs fixing either way.
2. hiddenInterfaceModes discards isModeKnown — project-or-other-settings-list.component.tsx:44.
useInterfaceMode() returns 'simple' both when the mode is really simple and when the read hasn't
settled, and the hook's own TSDoc says simple-only UI must gate on isModeKnown or "simple-only
affordances render for power users until the setting resolves". With no localStorage seed (fresh
install, cleared storage) a Power-mode user sees "Perform automatic sync at startup" render and then
vanish, and could click it in that window. Reading the mode in this component rather than passing it
from SettingsTab looks right to me otherwise — isHidden filtering already lives only here.
3. The two copies of the gate state different rules for an unreadable interfaceMode. The
extension copy treats an unreadable mode as Simple and applies the gate; the main-process callers
treat a non-'simple' read as not-Simple and return before the gate. Both skip today, so there's no
live divergence — but the rule is written two ways and won't propagate. Related: isFirstRunComplete()
has no mode check at all and depends on each caller having already returned for Power mode, while
its extension twin bundles the check — and the rule file calls the extension copy "the reference
copy". Someone adding a trigger by following the rule literally and calling isFirstRunComplete()
reintroduces exactly the trap the rule warns about. Either state the mode pre-check as a precondition
in both the rule and the TSDoc, or fold it into isFirstRunComplete(). (I confirmed the
can't-import-from-src/main reasoning — extensions/tsconfig.json declares no such aliases. Though
platform-bible-utils is an allowed universal import and already imported there, if you'd rather
have one implementation.)
4. The ADR contradicts the code it describes — Architecture-Decisions.md:847 says the startup
and project-switch skips log at debug "which packaged builds drop", and concludes they're only
partly visible in a support log. Both log at info (startup-tasks.ts:366, and
platform-scripture-editor.utils.ts:1162) — which is better than the ADR claims, but the sentence
will mislead whoever next debugs a session that didn't sync. Looks like it predates the logLevel
plumbing.
5. On your two open questions. I'd split setting.component.tsx: the change shape is better than
the mutated local it replaces and the bug is real, but it touches every settings row in the app and
lands in a PR whose reviewers are looking at sync gating. It separates cleanly from the
hiddenInterfaceModes filter in the sibling file. On hiddenInterfaceModes itself — it's permanent
public API on SettingBase (type, JSON schema, dist/ artifacts, visible to every extension author)
added to serve one internal setting. Leaving syncOnStartup hidden and just stopping the wizard from
writing it would remove the whole limb, with "a way back" shipping separately. Worth a deliberate
call rather than a side effect of the bugfix.
6. The newly visible toggle governs 1 of 4 triggers. A user who turns off "Perform automatic sync
at startup" is still synced at quit, window close, and project switch, and neither label nor
description says so. The opt-out also isn't fail-closed in the same direction as the consent gate —
an unreadable syncOnStartup defaults to syncing. Also, the cross-boundary TSDoc in
papi-shared-types.ts:363 says "User-configurable from settings" / "Read by startup-tasks on each
launch" with no mention that Power mode neither surfaces nor reads it; that text publishes to the
papi-dts docs site, so worth fixing at source and regenerating.
Testing
The unit coverage behind the gates is genuinely good. What's missing is the ticket's own sequence:
every e2e edit here is the label rename plus a dontSyncYetButton locator, so nothing drives Step 2
→ "Unrestricted" and asserts no sync notification appears before Step 4, and the "Sync" branch has no
e2e at all. That matters more than usual given the repro was never reproduced — the only thing
linking fix to report is the code trace. The spec runs in demo mode where firstRunComplete is never
persisted, so the wizard-active window is exactly the gated state and a negative assertion looks
feasible there.
Also: the rule file says there's no chokepoint and "a new automatic trigger that forgets it
reintroduces the bug silently". There's precedent for enforcing that mechanically —
SendReceiveWriteLockCoverageTests scans the tree and fails on uncovered write sites. A source-scan
test would turn the rule into enforcement.
Smaller things
- Backward-facing
PT-4369tags at five sites (platform-scripture-editor.utils.ts:1133,
core-settings-info.data.test.ts:12,sync-consent-step.component.test.tsx:127,
first-run-store.test.ts:316in a test name,first-run.spec.ts:13). Per
forward-facing-comments.mdthese belong in the commit message; each reads fine with the ID cut. - The gate rationale is restated in six places (util TSDoc, two inline blocks, rule file, ADR, plus
the "say only what is known" log caveat three times). One canonical statement plus a pointer per
site would be easier to keep true — keeping genuinely local notes like "gate goes after
cancelSyncso the consent step's own sync stays cancellable". setting.component.tsx:343— the newuseMemocan't ever hit its cache:debouncedHandleChange
is recreated every render (debounce(...)at :265 is unmemoized), so deps change every render.
Memoizing it would also stop the 500ms debounce being replaced on each render.setting.component.tsx:362/:371— the label fix names the control, butdescriptionis
tooltip-only on a non-focusable<Label>(no keyboard/SR path, noaria-describedby), and the
error block has norole="alert"/aria-live/aria-invalid. This PR makes a description
load-bearing for the first time — thesyncOnStartupscope is stated only there. Cheap now that
controlIdexists.- The
interfaceLanguagea11y gap is honestly documented, but has no ticket, so nothing will surface
it again —TODO(PT-XXXX)would fix that. Its test also asserts the gap stays and reads as
intended behaviour; the Testing-Guide'stest.failstripwire fits better. It's also closeable
without touchingplatform-bible-react:idon the<Label>plusrole="group" aria-labelledbyon a wrapper. - Search/zero-results disagree with what renders:
SettingsTab's search and zero-results state count
properties with no visibility filter, and the early return atproject-or-other-settings-list.component.tsx:51
checks unfiltered entries — so in Power mode "automatic sync" matches, the zero-results message is
suppressed, and you get a titled card with an empty body. en.json:448edits an existing key's value in place ("on startup" → "at startup") as the setting
becomes visible; looks meaning-preserving, but worth confirming given you retired
%firstRun_button_skipSync%properly viametadata.json+fallbackKey. es/fr/zh keep the old
phrasing.skipped-consent-not-confirmedreads oddly besideskipped/failed/selection-failed/timed-out;
skipped-consent-unconfirmedparses on first read. The distinct outcome itself is well justified.- Two
createSettingsStubhelpers now exist with different semantics (shared one keyed by short
names withREAD_THROWS; the extension's keyed by full setting name storing anError). Workspace
boundary prevents sharing, but aligning the option names would keep one idiom. - Consent step UX:
SyncConsentStepdoesn't callsetManagesOwnFooter(true), soWizardStepForm
emits its own row with "Sync" and the shell renders a second row beneath with "Don't sync yet" as
variant="ghost"— lowest emphasis in the set. For a decision where declining is legitimate,
outlinebeside the primary in one row reads as a peer choice. The relabel itself is good. - In-flight sync is visual-only (spinner, decline silently removed, nothing announced) —
aria-busy
or a polite live region would help on a long S/R; the shell already does this for its step
indicator. And the error state (destructive Alert + restored decline) has no Storybook story.
Tickets worth filing
Several things are acknowledged in the ADR/rule but untracked, so they'll be re-derived rather than
fixed: the cross-repo S/R auto-sync engine and syncOpenProjects (no caller in core — still a live
alternative explanation for PT-4369 that this PR can't close), getSharedProjects reaching the
registry behind the overlay before Step 4, the UiLanguageSelector a11y gap, and the ADR's own two
"not yet ticketed" latent issues.
Things I checked and liked
- The risky-looking deletions are safe. The localStorage cache key,
resolveInternalself-heal and
skippedStepall existed solely to make the wizard'ssyncOnStartup = falsewrite durable, and
the other self-heal — thefirstRunCompletecache-vs-setting reconciliation at
first-run-store.ts:213-218— is untouched. That's the one that matters now, since all four gates
read that flag; deleting it would have stranded users. hiddenInterfaceModesmirrorsMenuItemBasefield-for-field, schema included, and derived-type
propagation is correct (Localized<>leaves it alone;dist/index.d.tscarries both schema
copies). The renderer-vs-host filtering divergence from menus is the right call.- ADR slug is in correct
LC_ALL=Cbyte-order position, and the entry correctly records the
pre-existing startup gate. - Shutdown ordering is right — the gate after
cancelSyncis safe becausecancelSynccan't start
or complete a sync, and a consented in-flight sync stays cancellable. - Zero suppressions, no blocked imports, no secrets,
booleanValidatoron both settings.
One note on the PR description: claim #1 ("startup … now gated") overstates the diff — a startup gate
already existed at merge-base (startup-tasks.ts:355-363, same === true read). This PR extracts it
into the shared util and raises its log level; the genuinely new gates are shutdown, window close and
project switch — and only syncOnProjectSwitch can produce the reported symptom, which makes it the
actual fix. The ADR gets this right; just the description that overstates.
Review was AI-assisted (Claude Opus 5, /review-pr); findings verified against the branch and
curated by me.
9c5a7cd to
b4fac06
Compare
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Reply to Jolie — PR #2700 review of 2026-09-09
Thank you for this — the grep of every sync starter and the check that the power-mode regression
test actually pins what it claims saved me from re-deriving both. Everything below is on the branch
now, in two commits: b4fac06 ("hold automatic sync for the session after 'Don't sync yet'") and
01629b6 ("e2e-test the sync consent gate through the first-run wizard").
Short version: I took the session-scoped decline, removed the hiddenInterfaceModes limb rather
than fixing it, folded the two gate copies into one, and added the e2e sequence. I did not add
the source-scan enforcement test and did not split setting.component.tsx out — reasons below.
1. "Don't sync yet" opened all four gates — fixed, and the reading reversed
You were right that this is the point that matters, and that the decision was living in the wrong
place. I took the session-scoped reading.
There is now one consent gate, getAutomaticSyncConsent() in
src/main/first-run-consent.util.ts:61, answering granted / unconfirmed / deferred. "Don't
sync yet" calls platform.deferAutomaticSyncForSession (first-run-store.ts:403) before
persisting completion, and the wizard stays open if that call fails. The deferral lives in
main-process memory (first-run-consent.util.ts:34) rather than a setting, so it cannot outlive the
session and there is no startup-time clear to race; the next launch syncs as usual.
The ADR entry is rewritten around this and records that it reverses the epic owner's 2026-08-16
ruling, with your DoD point as the reason. Its status still reads "awaits the epic owner's
confirmation on PT-4369" — I've asked, and I agree it should be an explicit sign-off with the
ticket wording amended rather than something a reader discovers in an ADR paragraph. That is the one
item here still open on someone else.
completeFirstRun's TSDoc no longer claims the decline defers one session — it now says it persists
no sync preference and points at declineFirstRunSync
(src/renderer/services/first-run-store.ts:376).
2. hiddenInterfaceModes discarding isModeKnown — removed rather than fixed
Taking your #5 recommendation made this one disappear: hiddenInterfaceModes is gone from
SettingBase entirely, so project-or-other-settings-list.component.tsx reads no interface mode at
all and is back to filtering on isHidden alone (line 56). platform.syncOnStartup stays hidden and
the wizard no longer writes it.
3. Two copies of the gate stating different rules — folded into one
There is now one implementation. syncOnProjectSwitch asks the main process through the new
platform.getAutomaticSyncConsent command
(extensions/src/platform-scripture-editor/src/platform-scripture-editor.utils.ts:1152), so the
extension host keeps no copy of the rule; isFirstRunComplete() is gone.
For the mode check I took the "state the precondition" half rather than folding it in — folding it
in would make the function unusable from the one place that legitimately must not consult it. It is
now a Precondition paragraph in the TSDoc (first-run-consent.util.ts:56) and the same sentence
in the rule file, both saying an unreadable mode must not become a way past the gate: treat it as
Simple, or skip the sync.
4. ADR contradicting the code it describes — fixed
Architecture-Decisions.md:1788 now reads "Every consent skip logs at info, which packaged builds
keep, and names its reason."
5. Your two open questions
hiddenInterfaceModes: removed, as above — it was permanent public API serving one internal
setting, and your framing of that is what decided it. Profiles already carrying afalsefrom the
old decline button have no way back; that is PT-4607, shipping separately as you suggested.setting.component.tsx: I kept it here rather than splitting it. The debounced handler is now
created once and calls through a ref, which is what makes the control memo actually cache and
collapses edits across re-renders into one write — and it grew rather than shrank in this round:
aria-describedbyfor descriptions,role="alert"+aria-invalidfor errors, a labelled group
for the interface-language selector. So it is a larger blast radius than when you raised it, not a
smaller one. I judged the coupling to the settings-row work in this PR worth more than the cleaner
review boundary; that is a defensible call in the other direction too, so say so if you'd still
rather it moved — it separates as cleanly now as it did then.
6. The visible toggle governing 1 of 4 triggers — moot, and the TSDoc fixed
With the toggle hidden again there is no label promising more than it delivers. The cross-boundary
TSDoc at src/declarations/papi-shared-types.ts:427 now says it is hidden, that only the Simple-mode
startup sync reads it, that Power mode never does, that the wizard does not write it, and that a
stale false can be left over from the old decline button. papi.d.ts is regenerated.
The fail-closed asymmetry you noticed is deliberate and still there: an unreadable
syncOnStartup proceeds with the sync (startup-tasks.ts:371). Consent and preference fail closed
in opposite directions on purpose — syncing unasked cannot be undone, but a read failure should not
silently suppress a sync the user never turned off. That is stated at the site; tell me if you read
it differently.
The triggers core cannot gate at all are tracked as PT-4605 (syncs the Send/Receive extension
starts itself) and PT-4606 (the shared-projects registry path).
Testing
The e2e sequence is in (01629b6). A "First-run sync consent gate" suite drives the wizard to
the Sync consent step with "Unrestricted" selected and asserts that no Send/Receive starts before
consent, with two positive controls so the negative assertion stays falsifiable:
- Before the asserted window opens, the test sends
syncProjectsitself and waits for its toast —
proving the whole observation path (handler → notification router → toast behind the overlay)
works in that app instance. - Inside the window it switches the editor to a project, which is what the picker behind the overlay
does once internet use is permitted, and requires the gate's skip line to appear — proving an
automatic trigger really ran there and was turned away.
Both branches are covered: "Don't sync yet" leaves the gate answering deferred; "Sync" does not
record that deferral. The suite restores permittedInternetUse afterwards, since that setting is
shared with a co-installed Paratext 9 rather than scoped to the test profile.
Two things fell out of making it run: FirstRunPage now finds the wizard by its first-run-dialog
test id (the onboarding tour that opens as the wizard closes is also a role="dialog"), and
teardownElectronApp ends the whole process tree with taskkill /T on Windows — there are no
process groups there, so signalling the pid left children holding port 8876 and the next launch
talked to the stale app.
The source-scan enforcement test I did not build. I agree with the precedent and that it is the
right end state. What stopped me is that SendReceiveWriteLockCoverageTests works because its write
sites are a small set of literal call patterns in one language; the sync starters here are split
across TS in main, TS in a bundled extension, and a command-name string, and the honest version of
the scan has to recognise "this call site is user-initiated" — which is exactly the judgement the
rule exists to make. A scan I don't trust reads as coverage and is worse than the rule. I'd rather
scope it as its own piece of work than bolt it on here — happy to open the ticket, and happy to be
told it shouldn't wait.
Smaller things
- Backward-facing
PT-4369tags: removed from all five sites. The only remaining occurrences in
source are the ADR entry and forward-facingTODOs naming open tickets (PT-4605/4606/4607). - Six restatements of the rationale: collapsed to one.
getAutomaticSyncConsent's TSDoc is the
canonical statement; the other sites carry{@link getAutomaticSyncConsent}and keep only their
genuinely local notes — e.g. why the gate sits aftercancelSyncso the consent step's own sync
stays cancellable.
Open on your side, if you're willing: whether the setting.component.tsx call in #5 is acceptable,
and whether the enforcement scan should block this PR. Open on the epic owner's: confirming the
session-scoped decline so the ADR status and the ticket wording can settle.
@katherinejensen00 made 1 comment.
Reviewable status: 0 of 45 files reviewed, all discussions resolved.
jolierabideau
left a comment
jolierabideau
left a comment
There was a problem hiding this comment.
Re-review — PT-4369 gating
Re-reviewed at 01629b6. Thanks for the thorough reply. I checked each claim against the code and they hold: the session-scoped decline, the single getAutomaticSyncConsent() gate, removing hiddenInterfaceModes, the ADR log-level fix, the syncOnStartup TSDoc, and the new e2e suite with its positive controls. You also picked up several smaller items the reply didn't mention (skipped-consent-unconfirmed, one aligned stub helper, one footer row with an outline decline, the live region, the SyncFailed story). Nice work.
Needed before I approve
1. Enforcement scan: a test or a follow-up ticket. Keeping setting.component.tsx in this PR is fine by me. For the source-scan enforcement test, I'm happy to defer it, but it needs a ticket. I couldn't find one referenced anywhere, and the rule file's "Enforcement" section currently relies on reviewers remembering. Please file it and point to it from .claude/rules/first-run-sync-consent.md, e.g. as a TODO(PT-XXXX), so the gap has a searchable home.
2. Epic-owner sign-off on the session-scoped decline. The ADR status still reads "awaits the epic owner's confirmation on PT-4369" (Architecture-Decisions.md:1707). Once that's confirmed, please update the status and the ticket's DoD wording.
3. The PR description is out of date. It still describes the first version of this branch:
hiddenInterfaceModesas "the only public API addition", with its schema/dist/regeneration.syncOnStartupbecoming "a real, reversible setting".isFirstRunComplete(), the extension's "own copy of the gate", andskipped-consent-not-confirmed.- "42 files · 2 commits", and the decline described as wizard-scoped.
The two new public commands, platform.getAutomaticSyncConsent and platform.deferAutomaticSyncForSession, aren't listed at all. The footer also still has the [session](<session URL>) placeholder. Reviewers who come in through the description will read the wrong design.
Small fixes
assets/localization/metadata.json:63: the%firstRun_button_skipSync%deprecation message says the choice "defers only the wizard's own sync". It now defers every automatic sync for the session.first-run.spec.ts:151: the comment says "Don't sync yet" invokescompleteFirstRun(). It now goes throughdeclineFirstRunSync(), which records the deferral first.first-run-shell.component.tsx:234-240: the shell's fallback "Don't sync yet" button can't render.SyncConsentStepis the onlysetCanSkip(true)caller, and it also setsmanagesOwnFooter.onSkipis also always bound todeclineWizardSync, so a future step that callssetCanSkip(true)for an unrelated reason would silently defer sync for the session. Either delete the fallback button, or rename the plumbing (setCanDeclineSync/onDeclineSync) so the side effect is visible at the call site. The docs infirst-run-step-props.model.ts:6/:31-35still describe a generic "Skip button" either way.deferAutomaticSyncForSession: there's no way to lift the deferral short of a restart. Choosing "Sync" in a re-raised wizard doesn't clear it, and any PAPI client can call the command. I traced the re-raise path and it only starts on a later launch, so I don't think this is reachable today. A line in the TSDoc saying the one-way behaviour is intentional would stop someone "fixing" it later.startup-tasks.test.ts: startup is the only one of the four triggers with nodeferredcase. The interesting one is a decline recorded during the readiness wait, which the post-wait re-check handles but nothing pins.- PT-4605 and PT-4607 appear only in the ADR. A
TODO(PT-4605)at the gap in core, and aTODO(PT-4607)next to the stale-falsecaveat inpapi-shared-types.ts, would make them findable from the code. - Two known issues have no ticket: the ADR's "the gate is only as strong as the durable completion write" (~
:1796), and the "pre-existing upgrade race" from the description. Worth filing so they aren't re-derived later.
Nits (take or leave)
resetAutomaticSyncDeferral→resetAutomaticSyncDeferralForTesting, to matchresetWindowActivationForTestingand the other test-only helpers insrc/main.- The rule file and
getAutomaticSyncConsent's TSDoc cite the "first-run-sync-consentADR". The slug isadr-first-run-sync-consent, and other rule files cite the full slug. - The
SyncFailedstory's mock error says "Send/Receive server". The user-facing name is now "Sync". - The e2e "Sync" branch only asserts that no deferral was recorded.
grantedisn't covered end to end because demo mode never persists completion. That's fine given the unit coverage; just noting it.
Review was AI-assisted (Claude Opus 5.5, /review-paratext); findings verified against the branch and curated by me.
521bed6 to
eaacc80
Compare
Reply to the 2026-09-23 re-reviewThanks, Jolie. The branch is rebased onto current Needed before approval
Small fixes, all done
Nits, all taken
Worth a look after the rebaseMain changed two files this branch touches:
|
|
Follow-up tickets are filed and linked to PT-4369:
These are in commit |
The Simple-mode first-run wizard is an overlay, not a replacement: the dock layout, the default project picker and the shutdown tasks all keep running behind it. So an automatic Send/Receive could start before the user reached the wizard's sync-consent step and was asked. Gate every automatic sync trigger on `platform.firstRunComplete`: - startup, shutdown and window close share `isFirstRunComplete` in the new `src/main/first-run-consent.util.ts` - `syncOnProjectSwitch` keeps its own copy — a bundled extension cannot import from `src/main` An unreadable flag means do not sync: syncing unasked cannot be undone, while a missed automatic sync is picked up later. The gate is Simple-mode only by design — the flag is never written in Power mode, so gating a Power path on it would permanently disable that path with no UI to recover. A regression test names that trap. Also untangle the wizard's decline from the standing preference. The button said "Skip automatic sync" and persisted `platform.syncOnStartup = false`, which needed a localStorage hint and a self-heal block to stay reliable and left the user permanently opted out with no way back. Per PT-4178's approved labelling the choice is wizard-scoped, so the button is now "Don't sync yet", `completeFirstRun` persists no preference (the hint, the self-heal and `skippedStep` are gone), and `platform.syncOnStartup` becomes a visible, reversible setting. While making that setting user-facing, fix the label association it relies on: `Setting` pointed `htmlFor` at an id no control carried, so every settings toggle was unnamed to a screen reader and label clicks did nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
Adds `hiddenInterfaceModes` to `SettingBase`, mirroring the existing `MenuItemBase.hiddenInterfaceModes`, and applies it to `platform.syncOnStartup`. Only the Simple-mode startup path reads that setting, so a Power-mode user could previously flip a switch that did nothing — and the caveat was visible only on label hover. Other review fixes: - `syncOnProjectSwitch` now reads `platform.interfaceMode` itself rather than documenting a Simple-mode-only contract in prose. A future Power-mode caller would otherwise have been silently and permanently unsynced, since `platform.firstRunComplete` is never written outside Simple mode. An unreadable mode is treated as Simple, so it cannot become a way past the gate. - Extract the duplicated settings stub to `src/main/settings-stub.test-util.ts`. Each suite declares its own answered-key set, so an unstubbed key still throws — and stubbing `syncOnStartup` in the shutdown suite, which must never read it, is now a compile error. - Pin the `useId`-over-`settingKey` choice with a two-instance render test, and derive the label association from a single place so it cannot drift from the rendered control. - Raise the consent-skip log from `debug` to `info` on the startup and project-switch paths; packaged builds drop `debug`, so those skips were invisible in exactly the logs support needs. - Align the "skip" prose with the "Don't sync yet" users actually see, make the Storybook cross-reference a navigable deep link, align the settings label preposition with its neighbour, and mark the remaining `interfaceLanguage` label-association gap. - Record in ADR-0013 that the setting's startup-only scope is deliberate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-URL: <session URL>
`finalizeProjectSwitch` dispatches `syncOnProjectSwitch`, which now reads `platform.firstRunComplete` and logs through `papi.logger.info`. The finalize tests' mock papi resolved every settings key to 'simple' and exposed only `logger.warn`, so the consent gate read `'simple' !== true`, suppressed the sync, and then threw on the missing logger method — one failed assertion plus six unhandled rejections in CI. Extract the key-aware settings stub the syncOnProjectSwitch tests already use into `createSettingsStub` and share it with the finalize mock, and add `logger.info`. The two Power-mode tests now override just `platform.interfaceMode` instead of blanket-resolving every key. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_0121K5RSuvyZjx9RS23nyXJe
Declining at the wizard's sync-consent step persisted `platform.firstRunComplete = true`, the one flag every gate read, so a project switch, window close or quit could sync seconds after the user chose "Don't sync yet". One consent gate now answers for every automatic sync: `getAutomaticSyncConsent()` returns `granted`, `unconfirmed` or `deferred`. - Startup, shutdown and window close call it directly. - `syncOnProjectSwitch` asks main through the new `platform.getAutomaticSyncConsent` command, so the extension host no longer keeps its own copy of the rule (the two copies disagreed about an unreadable interface mode). - "Don't sync yet" calls `platform.deferAutomaticSyncForSession` before persisting completion, and the wizard stays open if that fails. The deferral lives in main-process memory, so it ends with the session and there is no startup-time clear to race; the next launch syncs as usual. This reverses the earlier "only the wizard's own sync" reading; confirmation is requested on PT-4369. Drop `hiddenInterfaceModes` from `SettingBase`. It was permanent public API added for one internal setting, and the visible toggle governed only the startup trigger. `platform.syncOnStartup` stays hidden and the wizard no longer writes it; a way back for profiles that already have `false` is PT-4607. Other review follow-ups: - The sync-consent step renders its own footer (Back, "Don't sync yet" as an outline button, Sync), takes the shell's new `isBusy` prop, announces an in-flight sync through a polite live region, and gains `WithBack` and `SyncFailed` stories. - Settings rows: the debounced change handler is created once and calls the latest handler through a ref, so the control memo caches and edits across re-renders collapse into one write. Descriptions are exposed through `aria-describedby`, errors render in `role="alert"` with `aria-invalid`, and the interface-language selector is named by a labelled group. - Rename the `skipped-consent-not-confirmed` shutdown outcome to `skipped-consent-unconfirmed` and add `skipped-consent-deferred`; every consent skip logs at info with its reason. - Align the extension's gate stub with main's `createSettingsStub` option names, remove backward-facing ticket tags, state the gate's rationale once, and rewrite the ADR entry and rule file for the session deferral. PT-4605 and PT-4606 track the triggers core cannot gate. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_01Gnemvj9f9UMHrY93QY2WMx
Adds a "First-run sync consent gate" suite. It drives the wizard to the Sync consent step with "Unrestricted" internet use selected, and asserts that no Send/Receive starts before consent: the project switch behind the overlay must log its gate skip, and no sync toast may appear. Two positive controls keep that negative assertion falsifiable: a sync the test sends itself must produce a toast first, and the skip line must appear inside the asserted window. - "Don't sync yet" leaves the gate answering `deferred` for the session. - "Sync" does not record that deferral. The internet-use setting is shared with a co-installed Paratext 9, so the suite restores it afterwards. Supporting changes: - `FirstRunPage` finds the wizard by its `first-run-dialog` test id, because the onboarding tour that opens as the wizard closes is also a `role="dialog"`. It gains `syncButton`, `unrestrictedOption`, and `selectUnrestricted`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The window-close deferral test still used the pre-rebase `windowWebViews` helper, which main renamed to `asWindowWebViews`. - The error-announcement test relied on the default localized-strings mock, which an earlier describe from main replaces for the rest of the file. It now sets its own strings and restores them afterwards. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Rename the wizard's decline plumbing from `setCanSkip`/`onSkip` to `setCanDeclineSync`/`onDeclineSync`, so a step that opts in can see that it defers automatic sync for the session. The prop docs no longer describe a generic Skip button. - State in the TSDoc of `deferAutomaticSyncForSession` and its command that the deferral is one-way until restart on purpose. - Pin the startup trigger's `deferred` answer, including a decline made during the readiness wait, which only the post-wait re-check catches. - Point at the open follow-ups from the code: `TODO(PT-4605)` at the consent gate for the extension's ungated `syncOpenProjects`, and `TODO(PT-4607)` beside the stale `syncOnStartup = false` caveat. - Rename `resetAutomaticSyncDeferral` to `resetAutomaticSyncDeferralForTesting`, like the other test-only helpers in `src/main`. - Cite the ADR by its full `adr-first-run-sync-consent` slug. - Correct stale wording: the `%firstRun_button_skipSync%` deprecation message, the e2e comment on "Don't sync yet" (it goes through `declineFirstRunSync()`), and the `SyncFailed` story's error text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `TODO(PT-4774)` in the rule file's Enforcement section: a source-scan test to replace reviewer vigilance. - The ADR cites PT-4775 for the durable completion-write gap, and records the unconfirmed upgrade race with the `firstRunComplete` backfill as PT-4776. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
8274e41 to
5727979
Compare
This branch has not been deployed
2 participants
PT-4369: Gate automatic sync on first-run consent
Branch:
pt-4369-gate-sync-on-consent→main· 34 files · 8 commits · rebased ontomain2026-09-23Summary
The Simple-mode first-run wizard is an overlay. The dock layout, the project picker, and the
shutdown tasks keep running behind it. So an automatic Send/Receive could start before the user
reached the wizard's Sync consent step. Choosing "Unrestricted" at the Internet step was enough:
the project picker opened an editor behind the overlay, and the project switch synced it.
What this PR changes:
getAutomaticSyncConsent()insrc/main/first-run-consent.util.tsreturnsgranted,unconfirmed(the wizard is notanswered, or its flag could not be read), or
deferred. Startup, shutdown, and window close callit directly.
syncOnProjectSwitchin the extension host asks main through the newplatform.getAutomaticSyncConsentcommand, so there is no second copy of the rule. It failsclosed.
platform.deferAutomaticSyncForSessioncommand before it persists completion, and the wizardstays open if that call fails. The deferral lives in main-process memory, so it ends with the
session and the next launch syncs as usual. It is one-way by design: nothing lifts it before a
restart.
persisted
platform.syncOnStartup = falsepermanently, with no UI to undo it. Now nothing ispersisted. The setting stays hidden. A way back for profiles that already have
falseisPT-4607.
Where to look
src/main/first-run-consent.util.tsextensions/.../platform-scripture-editor.utils.tssyncOnProjectSwitch: the trigger behind the bug. It asks main throughplatform.getAutomaticSyncConsent, and it treats an unreadable interface mode as Simple so the mode cannot become a way past the gate.src/main/shutdown-tasks.tscancelSync, so a sync the user consented to can still be cancelled. Adds theskipped-consent-unconfirmed/skipped-consent-deferredoutcomes.src/main/startup-tasks.tssrc/main/main.tssrc/renderer/services/first-run-store.tsdeclineFirstRunSync()records the deferral, then completes the wizard. The oldsyncOnStartupwrite, its localStorage hint, and the self-heal that supported it are removed.src/renderer/components/first-run/…setCanDeclineSync/onDeclineSyncso the session-wide side effect is visible at the call site.src/renderer/.../setting.component.tsxaria-describedby, and errors userole="alert"/aria-invalid. The debounced writers are created once and call the latest handler through a ref..claude/rules/first-run-sync-consent.md+adr-first-run-sync-consentThe trap to know about. Only the Simple-mode wizard writes
platform.firstRunComplete, so inPower mode it stays
falseforever. Gating a Power-mode path on it would disable that pathpermanently, with no UI to recover. A named regression test in
startup-tasks.test.tsguardsagainst this.
Public API changes
platform.getAutomaticSyncConsent: () => Promise<'granted' | 'unconfirmed' | 'deferred'>(@experimental)platform.deferAutomaticSyncForSession: () => Promise<void>(@experimental)platform.syncOnStartup: TSDoc only. The type is unchanged and the setting is still hidden.lib/platform-bible-utils: no changes. ThehiddenInterfaceModesaddition from an earlierrevision was removed.
Rebase notes (2026-09-23)
Main changed code this branch touches, so the rebase combined them:
window in its skip line. Main's new window-close tests stub
firstRunComplete: true, becausethey would otherwise be closed by the gate.
ZoomStepperon main. It joins the{ control, labelFor }memo. Itis a button group named by
groupLabel, so the row label does not point at it.useMemo. This branch keeps its ref-helddebouncers, now applied to both main's 500 ms writer and its 150 ms stepper writer. Both
approaches pass main's "re-render between keystrokes" test. Only the ref version also writes
through the latest props when they change while an edit is pending.
killProcessTreealready covers the Windows e2e teardown fix, so this branch's copy wasdropped.
Testing
src/main/: 1177 tests pass. Renderer and extension-host: all pass except tworc-dock-tab-cache-patchtests. Those fail because this worktree's installed rc-dock is notre-patched, and this branch does not touch rc-dock.
platform-scripture-editorextension: 1680tests pass.
npm run typecheckis clean.npm run lintis clean on branch files."Unrestricted" selected and asserts that no Send/Receive starts before consent. Two positive
controls keep that negative assertion falsifiable. The "Sync" branch asserts only that no
deferral was recorded: demo mode never persists completion, so
grantedis covered by unit testsrather than end to end.
Known gaps and follow-ups
syncOpenProjectscan start syncs core cannot gate (TODO(PT-4605)at the gate).getSharedProjectsregistry lookup runs before consent. It is not a Send/Receive.syncOnStartup = falseby the old decline have no way back (TODO(PT-4607)inpapi-shared-types.ts).TODO(PT-4774)in the rule file's Enforcement section).firstRunCompletebackfill, not yet reproduced (cited in the ADR).AI-assisted — session
This change is