Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) by rolfheij-sil · Pull Request #2571 · paranext/paranext-core

rolfheij-sil · 2026-07-16T17:31:42Z

Follow-up PR agreed in #2555's approval ("we'll have a small, follow up PR for any remaining concerns").

1. Self-catching sync-edit-blocked notification helper (re-review N1)

#2555 gave the scripture editor a self-catching notifySyncEditBlocked(), but the finding-9 call sites added in the same round still did bare papi.notifications.send(...) — a rejection from the notification service would surface as an unhandled promise rejection. This adds the platform-scripture twin of that helper next to isSyncEditBlockedError in sync-edit-blocked.util.ts and routes all five call sites through it, so the shared "editing paused" message/severity cannot drift per call site:

checks-side-panel.web-view.tsx (deny/allow catch paths)
inventory.web-view.tsx (approved/unapproved items catch paths)
manage-books.web-view.tsx (mutation-result toast routing — the pre-existing same-shape exposure; its non-blocked generic toast now handles rejections with .catch for the same reason)

Rider: inventory's two await setValidItems?.(...) / await setInvalidItems?.(...) awaits now adopt the runtime promise via Promise.resolve(...) — useProjectSetting's declared setter type erases the promise the setter actually returns, so the bare await had no effect on the type and looked like a dead catch.

Unit tests cover the helper's happy path and that a rejected send is swallowed and logged.

2. Rename `SHUTDOWN_SYNC_TIME_OUT_MS` → `AUTO_SYNC_MAX_DURATION_MS` (re-review N2)

Thread: #2555 (comment)

Since the finding-21 hoist, the constant bounds both the shutdown-sync wait and the renderer's edit-block safety leash — the "shutdown" name no longer fit half its uses and the doc comment had to reconcile that. Renamed for what it bounds (the maximum duration of a single automatic Send/Receive), doc comments simplified at both consumers, papi.d.ts regenerated and committed.

3. Blocking-store hardening (PT-4214 Stage T; original findings 7/8)

Superseded in the fix round below: lyonsil's 2026-07-16 review proved the oldest-first clear-to-leash pairing described here wrong in both directions (his findings 1–3), and the seeding seam inert. The stores now use identity release tokens and the seeding is cut at this tip (it returns for real in #2574, served from C#). This section is kept for review history.

Finding 8 — the store's safety timer hard-reset blockCount = 0 when it fired, so under overlapping blocks one stale expiry wiped newer in-flight blockers. Each raise now arms its own one-shot leash that releases only its own block, exactly once; a clear cancels the oldest outstanding leash so a cleared block's leash can never fire later. Same flaw, same fix in the near-twin workspace-updating-store.ts (30 s leash). The two fixes are deliberately symmetrical without a shared abstraction — that extraction is deferred to PT-4214 Stage U (reviewer-agreed).

Finding 7 (mitigation) — implements the upgrade path documented in auto-sync-blocking-service.ts ("a command to read the current blocking state … that this service consults on init"): on init the service queries paratextBibleSendReceive.getAutoSyncBlocking (untyped requestNoRetry, the same pattern as cancelSync in shutdown-tasks) and seeds the store, so a renderer reload during an in-flight scheduled sync no longer comes up unblocked while the backend is still syncing. Best-effort by design: a live event always wins over the snapshot, a failed request keeps the assume-unblocked default (the expected path on plain Platform.Bible), and the per-blocker leash bounds a seeded block whose clearing event never arrives. Remaining for PT-4214: registering getAutoSyncBlocking on the Send/Receive extension side and declaring it in the copied paratext-bible-send-receive.d.ts command contract — the core-side seam here is inert until then.

Tests: overlapping blockers where a stale leash expires while a newer block is active (count stays correct, UI stays blocked until the newer one clears), clear-pairs-with-oldest-leash (both stores), and all seeding paths (seeds on true, skips false/non-boolean/failure, event wins over snapshot, no seeding after cleanup).

Verification

npm run typecheck — clean across all workspaces
npm run lint — clean
npm test — 3,599 passed, 1 skipped (all workspaces; includes the new/updated tests for the notification helper, blocking store, workspace-updating store, blocking service, shutdown/startup tasks)
npm run build:types — regen committed with the rename; re-run after the final commit with zero drift

Fix round — lyonsil's 2026-07-16 full-diff review (19 findings, review 4718215827)

Per-finding replies go on the inline threads. Findings 18/20 (message sentinel → coded error; a single interception point for gate rejections) are deferred to PT-4214 — recorded in that ticket's description with the comment ids, including the design note that FAILED_PRECONDITION alone cannot discriminate the gate. Finding 4 (#2560 collision over the constant rename) stays on the documented whichever-merges-second-rebases plan. Finding 17 (deferral cost) acknowledged — the record is accurate.

532739cc891 — findings 1/2/3 (blocking) + 14/15 + 5/6/13: leash pairing reworked to identity — each switch/block gets a release token, released exactly once by its own clear or its own leash; a late clear after a fired leash is a no-op and can never release a newer live blocker. The project-switch events now carry a switchId (typed via NetworkEvents) so the workspace pipeline pairs exactly end-to-end; the anonymous auto-sync boolean keeps oldest-first-with-tombstones and documents honestly what anonymous pairing cannot fix (resolved by PT-4214 Stage U: backend-authoritative per-project S/R block signal (E2E defects 2+3) #2574's snapshots). Both review traces are pinned by integration tests that fail on the pre-fix code. The dual counters and timer-array scans dissolve (14/15). The inert getAutoSyncBlocking seeding is cut (registered nowhere at this tip; its doomed consult cost ~9 s of main-process retry churn per renderer launch — requestNoRetry's renderer-side behavior is now documented on the function), the Known limitation reload note is restored pointing at PT-4214, and initAutoSyncBlockingService is synchronous again so the index.tsx comment is accurate (13).
a759690ac2e — findings 9/19: useProjectSetting declares its real types — setSetting returns the update-instructions promise, resetSetting returns Promise<boolean> (the wrapper no longer swallows it). Inventory's two no-op await Promise.resolve(...) workarounds deleted; the generic Setting component awaits the setter so write-gate rejections reach its existing catch. Typecheck clean across all workspaces — zero consumer fallout. papi.d.ts regenerated.
56b9934560a — findings 7/8/12/16: checks-side-panel no longer rethrows non-sentinel errors into a fire-and-forget void (logs + returns false); bare-string tests pin isSyncEditBlockedError's quote-wrapped manage-books path and why the regex must stay unanchored; SYNC_EDIT_BLOCKED_MESSAGE_KEY unexported and the test asserts the literal key; AUTO_SYNC_MAX_DURATION_MS doc names both consumers again with the never-diverge rule and pins "automatic" = app-driven.

Fix-round verification: npm test all workspaces green (includes the new trace/integration tests), npm run typecheck clean across all workspaces, eslint clean on changed files, npm run build:types drift committed. #2574 rebased onto this tip (its snapshot rewrite supersedes the auto-sync store/service changes here — expected churn, reconciled in its 08d0d3d31ac) and #2575 rebased on top; stack verified linear; #2574's C# suite green (1530 passed).

🤖 Generated with Claude Code

This change is

lyonsil

Full-diff review at max effort: 10 independent finder angles, adversarial verification of every candidate, and a gap sweep. 19 findings inline, ordered roughly by severity.

Two things I'd call blocking:

The per-blocker leash rework introduces a regression (workspace-updating-store.ts:53, same shape at auto-sync-blocking-store.ts:89). It fixes finding 8's bug but replaces it with the inverse: a leash that fires while other blockers are live decrements the count without recording that the real clear is still owed, so the late clear releases a live blocker and cancels its leash. I verified this executably against both this branch and origin/main with fake timers — origin/main handles the same trace correctly, so it's new. The fix that resolves it also resolves the two findings stacked under it.
src/main/shutdown-tasks.ts:77 collides with #2560, which deletes these lines for a shared runBoundedShutdownSync helper and adds three new references to the old constant name. Whichever merges second breaks; worth coordinating before either lands.

Also worth deciding before merge: the seeding seam (auto-sync-blocking-service.ts) is inert — getAutoSyncBlocking is registered nowhere, including in paranext-bible-internal-extensions#186, the consolidated PR that builds the auto-sync feature and emits the event this service consumes. Today it buys nothing and costs a ~9-second doomed retry loop on every renderer startup, because requestNoRetry's skipRetry flag is silently dropped on the renderer side.

Things I checked and am explicitly not flagging, so they don't cost anyone review time: the try/catch → .catch() swap in manage-books is a strict improvement (papi.notifications is a createSyncProxyForAsyncObject whose trap returns an async arrow, so the old catch was dead code and the rejection was leaking); N duplicate toasts are unreachable (EnterWrite throws before any MutationResult is built); and the grace-timer, notify and test-teardown paths all came back clean under fuzzing. The %...% localize key passed to notifications.send is correct — the host localizes it.

The blocking-store hardening is the right instinct and the doc comments are unusually good; the findings cluster in the leash pairing, the seeding seam, and the extension call sites.

(AI-assisted, with my guidance)

lyonsil · 2026-07-16T22:43:44Z

+    // second time when its leash fires. Switches are anonymous, so oldest-first is a pairing
+    // heuristic — and the conservative one: the still-open switches keep the newest (longest
+    // remaining) leashes.
+    const oldestLeash = safetyTimers.shift();


1. A late clear after a leash fires releases a live blocker. Verified regression vs origin/main.

Trace (30 s leash): A starts at t=0; B starts at t=29s; at t=30s A's leash fires → switchCount 2→1 (correct so far); at t=30.5s A's real onDidSwitchProject arrives → safetyTimers.shift() cancels B's leash and drops the count to 0. The overlay unmounts while B is still switching, and B's real finish is then swallowed by Math.max(0, -1).

I ran this against the real store with fake timers: this PR yields getWorkspaceUpdating() === false; origin/main correctly yields true, because its single re-armed timer never fires while a newer switch is alive. So this is a regression, not a pre-existing quirk.

Reachable here: 30 s is easy to overrun on a large project, and concurrent switches are the documented reason the ref count exists. Same defect at auto-sync-blocking-store.ts:89, though the window there is narrower.

Root cause: a fired leash decrements the count but doesn't record that the blocker's real clear is still owed, so the late clear consumes an anonymous live slot. Suggest marking a blocker released when its own leash fires, so the owed clear becomes a no-op.

(AI-assisted, with my guidance)

🤖 Claude: Confirmed, and your suggested shape ("marking a blocker released when its own leash
fires, so the owed clear becomes a no-op") is what landed, taken one step further to identity
end-to-end. Both stores now hand out one release token per switch/block; a token is released
exactly once — by its own clear or its own leash — and a late clear after the leash fired is a
no-op by identity, so it can never consume another switch's slot. For the workspace store the
pairing is exact all the way to the emitter: the project-switch events now carry a switchId
(typed via NetworkEvents in platform-scripture-editor's d.ts), and the service releases exactly
the switch each did-finish names. Your 30 s trace is pinned verbatim in
workspace-updating-integration.test.ts (real store + service, simulated events, fake timers) —
it fails on the previous code exactly as you describe (false at t=30.5 s) and passes now, with
the equivalent 10-minute shape for the blocking store in its store suite. The Math.max(0, -1)
swallowing is gone with the counter itself (finding 14). Fixed in 532739c.

lyonsil · 2026-07-16T22:43:44Z

+    // second time when its leash fires. Blockers are anonymous, so oldest-first is a pairing
+    // heuristic — and the conservative one: the still-open blocks keep the newest (longest
+    // remaining) leashes.
+    const oldestLeash = safetyTimers.shift();


2. Oldest-first cancellation defeats the leash it is meant to provide.

An abandoned blocker's leash is always the one nearest to firing, so every clear cancels it and the abandoned block inherits an ever-newer leash. Verified against the real store: with a blocker abandoned at t=0 and ordinary syncs every 5 minutes, getAutoSyncBlocking() is still true after 2 hours. The same shape holds for workspace-updating-store.ts (still true after ~10 min of switches).

This one is pre-existing — origin/main behaves identically — so it isn't introduced here. Flagging it because this PR's docs now assert it is fixed ("each block is released exactly once — by its clear or by its own leash"), and the new seeding leans on it ("the store's per-blocker safety leash bounds a seeded block whose clearing event never arrives"). Neither claim holds.

Worth noting the comment calls oldest-first "the conservative one" — it's the opposite: it always cancels the leash closest to firing. And paratext-bible-internal-extensions#186 widens the trigger: every SR_BUSY-rejected overlapping attempt raises and clears, and each of those clears strips the oldest leash — which belongs to the long-running sync actually in flight.

(AI-assisted, with my guidance)

🤖 Claude: Confirmed, including the "conservative one" comment being exactly backwards — that
text is gone. Fix in 532739c, in two layers. Store layer: release tokens by identity, so a
clear can only release its own block and an abandoned block is always released by its own leash;
your abandoned-blocker-with-traffic trace is pinned at the store level (2 h of 5-minute syncs,
asserts unblocked) and end-to-end for the workspace store, where the new switchId on the wire
makes finish-to-switch pairing exact (integration test: abandoned switch + 10 min of traffic,
fails on the old code). Wire layer for auto-sync: the blocking event is still an anonymous
boolean at this tip, and no clear-to-raise pairing rule can beat that — an abandoned raise's
missing clear makes any heuristic shift the unmatched block onto some newer raise. The service
doc now says so outright instead of claiming the leash covers it, and the oldest-first queue
keeps leash-released raises as tombstones so a late clear can at least never release a newer live
blocker (finding 1's direction). The real fix is identity on the wire, which #2574 does by
replacing the event with the backend gate's { isBlocking, projectIds } snapshots — no
ref-counting, no pairing, renderer leash deleted. The SR_BUSY raise/clear widening you note in
#186 dies with the same event (its emit chain removal is tracked in PT-4214).

lyonsil · 2026-07-16T22:43:44Z

+/**
+ * One outstanding safety leash per in-flight blocker, oldest first. Every raise arms its own
+ * one-shot leash and every clear cancels the oldest outstanding one, so the array's length always
+ * equals `blockCount` and each block is released exactly once — by its clear or by its own leash.


3. This documented invariant is false, and the new tests canonize the defect.

"each block is released exactly once — by its clear or by its own leash" is contradicted by both failure modes above: a block can be released by another block's leash, and its own leash can be cancelled by another block's clear.

The two new tests (auto-sync-blocking-store.test.ts:177, workspace-updating-store.test.ts:148) only exercise the direction where oldest-first pairing happens to be correct — the clearing blocker is the oldest. Neither covers the inverse (older blocker abandoned, newer one clears), which is the case that fails. As written they green-light the behavior as designed, so a future fix has to argue with a passing suite first.

(AI-assisted, with my guidance)

🤖 Claude: The invariant text was aspiration, not description — with anonymous oldest-first
pairing it was unachievable, not just unimplemented. It is now true by construction (release
tokens are idempotent and leashes are per-token), and the doc states it only where it holds. Both
inverse directions you flagged as untested are covered: late-clear-after-own-leash-fired
(finding 1's trace) and older-abandoned-newer-clears (finding 2's trace) run against the real
stores in both suites plus the service integration tests, and both fail on the pre-fix code. The
tests that green-lit oldest-first as designed were rewritten with the API.

lyonsil · 2026-07-16T22:43:44Z

  // Copy to a const so TypeScript knows the type is string inside the async IIFE below.
  const syncProjectId = projectId;
-  const syncComplete = new AsyncVariable<void>('shutdown sync', SHUTDOWN_SYNC_TIME_OUT_MS);
+  const syncComplete = new AsyncVariable<void>('shutdown sync', AUTO_SYNC_MAX_DURATION_MS);


4. Merge collision with #2560 over this rename.

#2560 rewrites exactly these lines in the opposite direction: it deletes the AsyncVariable line entirely in favor of a shared runBoundedShutdownSync(...) helper, and adds three new references to SHUTDOWN_SYNC_TIME_OUT_MS — inside the helper, as a new import in shutdown-tasks.test.ts, and in a startup-tasks.ts doc comment.

Whichever merges second breaks: #2560-second references a symbol this PR deleted; this PR second leaves the rename incomplete. Git will conflict on the import and this region either way, so it won't merge silently — but it needs coordinating with #2560's author rather than being discovered at merge time.

Worth reconsidering the rename on the merits, too: #2560 makes this constant bound a shared helper used by both Simple's sendReceiveProjects on shutdown and Power's runScheduledSessionSync('shutdown'). Simple's shutdown sync is not a scheduled/unattended sync under this PR's own definition, so AUTO_SYNC_MAX_DURATION_MS gets less accurate as #2560 lands.

(AI-assisted, with my guidance)

🤖 Claude: The collision is known and has a standing plan: whichever of #2560/#2571 merges second
rebases and reconciles (both are ours, so no cross-author surprise at merge time; the conflict on
this region + the import is expected). On the merits of the name: agreed the old doc oversold
"automatic" as scheduled-only, but the constant's doc (56b9934) now pins "automatic" to
app-driven syncs — ones the app starts itself (scheduled, session, shutdown) as opposed to the
user driving the Send/Receive dialog — which covers Simple's shutdown sync, so
AUTO_SYNC_MAX_DURATION_MS stays accurate as #2560's shared runBoundedShutdownSync lands. The
doc also names both consumers again with the never-diverge rule (finding 12).

lyonsil · 2026-07-16T22:43:44Z

+
+  (async () => {
+    try {
+      const isBlocking = await requestNoRetry<[], unknown>(


5. requestNoRetry does not skip retries from the renderer — this fires a ~9-second doomed retry loop on every startup.

RpcClient.request(requestType, requestParams) takes only two parameters, so the third skipRetry argument passed at network.service.ts:221 is silently dropped and never crosses the wire. Main's RpcServer.processMessage then re-enters this.request(method, params) with skipRetry defaulting to false, routing to requestWithRetry — MAX_REQUEST_ATTEMPTS = 10 spaced by REQUEST_ATTEMPT_WAIT_TIME_MS = 1000, so roughly 9 seconds of retry churn per renderer launch for a command that is registered nowhere.

The cited precedent doesn't transfer: the only other requestNoRetry callers are in src/main/shutdown-tasks.ts, where jsonRpc is RpcWebSocketListener and its request(requestType, requestParams, skipRetry = false) genuinely honors the flag. This is the first renderer-side caller.

So the doc comment above ("the request rejects and init keeps the assume-unblocked default") is right about the outcome but wrong about the cost. Not a stall — the IIFE is fire-and-forget — but it is pure waste today.

(AI-assisted, with my guidance)

🤖 Claude: Verified end to end: RpcClient.request takes two parameters and drops the flag, and
main's processMessage re-dispatches renderer-originated requests through requestWithRetry
(10 × 1 s), so "requestNoRetry" from the renderer is a misnomer — your cost math is right, and
the shutdown-tasks precedent indeed doesn't transfer (that caller runs in main, where the flag is
honored locally). Rather than invent a wire extension to carry the flag mid-review, the seeding
is cut at this tip (your first suggested option; see finding 6 — the command it consulted is
registered nowhere here), and requestNoRetry's TSDoc now carries a WARNING spelling out exactly
this renderer behavior so the next caller isn't misled (532739c, also in papi.d.ts). Worth
noting for #2574, which reintroduces the consult against a command served by core's own C#: there
the retry loop is arguably a feature — it bridges the dotnet-registration race at renderer
startup — which is another reason not to force the flag across the wire.

lyonsil · 2026-07-16T22:43:44Z

+export async function notifySyncEditBlocked(): Promise<void> {
+  try {
+    await papi.notifications.send({
+      message: SYNC_EDIT_BLOCKED_MESSAGE_KEY,


16. SYNC_EDIT_BLOCKED_MESSAGE_KEY's export is now vestigial, and the test asserts it against itself.

This PR removed its last external consumer — checks-side-panel, inventory and manage-books all call notifySyncEditBlocked() now — so the only references left are this line and the test. Leaving it exported keeps it reading as public surface, inviting a new caller to re-introduce the exact papi.notifications.send({ message, severity }) duplication the helper exists to prevent.

Suggest dropping the export. Related: sync-edit-blocked.util.test.ts:44 asserts toHaveBeenCalledWith({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, ... }) — comparing the constant to itself, so it cannot fail if the key is wrong. Asserting the literal '%webView_platformScripture_error_syncEditBlocked%' is strictly stronger and lets the constant go private.

(AI-assisted, with my guidance)

🤖 Claude: Both parts done in 56b9934: the constant is private (its doc now says why — every
caller goes through notifySyncEditBlocked so the severity/message pair cannot drift), and the
notification test asserts the literal '%webView_platformScripture_error_syncEditBlocked%', so
it fails if the key ever drifts from the contributed localizedStrings entry instead of comparing
the constant to itself. No other consumers existed (verified by grep before unexporting).

lyonsil · 2026-07-16T22:43:44Z

+ * One outstanding safety leash per in-flight switch, oldest first. Every start arms its own
+ * one-shot leash and every finish cancels the oldest outstanding one, so the array's length always
+ * equals `switchCount` and each switch is released exactly once — by its finish or by its own
+ * leash. (Same pattern as auto-sync-blocking-store; extracting a shared abstraction is deliberately


17. This deferral is what caused the leash defect to ship twice rather than once.

The PT-4214 Stage U deferral is reviewer-agreed and I'm not relitigating it — but it's worth recording the concrete cost, since it lands in this diff rather than a later one: the leash mechanism is duplicated near-verbatim across both stores (the array, the arming block, the shift-oldest pairing, the reset body — differing only by the timeout constant and a noun), and the subtle bug flagged above was copy-pasted along with it. It now has to be found and fixed in two places, and the two suites duplicate the same two scenarios.

Confirmed there's nothing existing to reuse — Mutex, MutexMap, AsyncVariable, PromiseChainingMap and EventRollingTimeCounter are all the wrong shape — so Stage U is genuine extraction work. One note for it: PlatformEventEmitter already covers the Set<() => void> + notifyListeners + subscribe/unsubscribe half that both stores hand-roll.

(AI-assisted, with my guidance)

🤖 Claude: The record is accurate: the deferral shipped the same defect twice, it had to be
found and fixed in two places, and this is the second time in this epic that deferred-cleanup
territory is where the bugs landed (the E2E driver-race was the first). The redesign keeps the
two stores deliberately near-verbatim parallel (token set + per-token leash + release-by-
identity) so the Stage U extraction stays mechanical; your PlatformEventEmitter pointer for
the hand-rolled listener half is noted on PT-4214's latch-store extraction item, where your
confirmation that nothing existing fits (Mutex/AsyncVariable/etc.) is also useful ammunition for
it being genuine extraction work rather than reuse.

lyonsil · 2026-07-16T22:43:44Z

@@ -32,3 +33,22 @@ export const SYNC_EDIT_BLOCKED_MESSAGE_KEY = '%webView_platformScripture_error_s
 export function isSyncEditBlockedError(error: unknown): boolean {


18. Altitude: this sentinel should be a coded error, and the pattern is proliferating rather than converging.

Detecting a backend rejection by regex-matching its message text is fragile in ways nothing here can catch: the contract is a bare string duplicated across a C# const and two independent TS regexes with no compile-time link, so a reword, any layer that wraps or truncates or localizes the message, or a project whose own error text contains the literal silently breaks or falsely trips detection.

The platform already has the machine-readable channel: PlatformErrorCodes.WithCode(code, message) stashes the code in Exception.Data["platformErrorCode"], network.service.ts:257 extracts it and rethrows newPlatformError(response, platformErrorCode), giving the renderer a checkable error.code — used in ~269 places, including ManageBooksService.cs:461 in this same feature area, which calls into this very gate. FAILED_PRECONDITION's own doc describes this case exactly ("rejected because the system is not in a state required for the operation's execution").

The producer is SendReceiveWriteLock.cs:215, which throws a bare InvalidOperationException with the sentinel glued onto the message. One line there (PlatformErrorCodes.WithCode(FailedPrecondition, ...)) lets every TS site check error.code and deletes the sentinel and both regexes.

This is getting more expensive, not less: paranext-core#186 adds isSyncAlreadyInProgressError (marker-substring matching) and paratext-10-studio#163 puts (SR_BUSY) on the wire with a comment noting a bare token "only matched in English." That's four sentinel sites across three repos and two languages. #163 doesn't touch SendReceiveWriteLock.cs, so the fix is unblocked.

(AI-assisted, with my guidance)

🤖 Claude: Agreed on the altitude: a bare-string contract duplicated across a C# const and
multiple independent regexes in three repos is fragile in exactly the ways you list, and it's
proliferating (isSyncAlreadyInProgressError, (SR_BUSY)). Not done in this round because it
is a wire-contract change spanning public core C#, the private studio patch, and two extensions'
matchers mid-review-window — the coordination cost is the definition of disruptive here. It is
now a concrete item in PT-4214's Stage U contract work (same seam as the
paratext-bible-send-receive.d.ts declarations), citing this comment. One pushback on the
mechanism as proposed: PlatformErrorCodes.WithCode(FailedPrecondition, …) alone can't carry
the discrimination — FAILED_PRECONDITION is shared by other precondition failures (SR_BUSY
among them), so error.code checks can't replace the regexes without also matching message
text, which keeps the sentinel alive. Deleting the sentinel needs a distinguishing
subcode/payload convention first; that design question is recorded on the ticket. The wire
sentinel stays unchanged until that lands.

lyonsil · 2026-07-16T22:43:45Z

      try {
-        await setValidItems?.(items.join(' '));
+        // `useProjectSetting`'s declared setter type erases the promise the setter actually
+        // returns at runtime; adopt it via Promise.resolve so this await genuinely waits and the


19. The root cause is one wrong line in the hook — fixing it there fixes ~15 consumers instead of two.

This comment diagnoses it correctly, then works around it locally. use-project-setting.hook.ts asserts the true type internally (lines 92-101: => Promise<DataProviderUpdateInstructions<...>>), passes the value through untouched at line 119, then declares it => void at line 68. The declared type is simply wrong.

That's not cosmetic: because TS believes the setter returns void, @typescript-eslint/no-floating-promises cannot fire on any unawaited call, so the compiler actively hides the bug from every consumer. It's already live at setting.component.tsx:227 — if (setSetting) setSetting(newValue); sits inside a try/catch that sets an error message, so the author clearly expected the catch to report write failures; the unawaited promise means it never sees the rejection. The sibling useSetting path one type above (line 86) declares the promise correctly, so only useProjectSetting erases it.

Fix line 68 and these two wrappers become deletable, the linter starts finding the remaining sites automatically, and Settings gets fixed for free. (resetSetting at line 112 is worse — it drops the promise inside the hook, so consumers can't even rescue it at the call site.)

(AI-assisted, with my guidance)

🤖 Claude: Fixed at the source in a759690: the declared tuple now matches runtime —
setSetting returns the DataProviderUpdateInstructions promise (the exact type the hook
already asserted internally) and resetSetting returns Promise<boolean> with the wrapper no
longer swallowing it inside the hook. Both inventory wrappers are deleted (finding 9), and the
setting.component site you cite now awaits the setter inside its existing try/catch, so a
write-gate rejection surfaces as the error message its author expected instead of an unhandled
rejection; its local ProjectSettingsControls type mirror of the wrong => void is fixed too.
Fallout: full typecheck across all workspaces is clean — the widening from void broke zero
consumers (checked the ~20 files using the hook). One correction to the mechanism you describe:
@typescript-eslint/no-floating-promises isn't enabled in any of this repo's eslint configs, so
the linter won't start flagging the remaining unawaited sites automatically — the type fix makes
them fixable (and visible to anyone who turns the rule on), which is necessary but not
sufficient. papi.d.ts regenerated with the corrected surface.

lyonsil · 2026-07-16T22:43:45Z

        // the shared "editing paused" warning; re-throw anything else so real errors still surface.
        if (isSyncEditBlockedError(error)) {
-          papi.notifications.send({ message: SYNC_EDIT_BLOCKED_MESSAGE_KEY, severity: 'warning' });
+          notifySyncEditBlocked();


20. Altitude: teaching N call sites about one backend error doesn't scale — and the omissions are invisible.

The write gate is not a niche path: ParatextProjectDataProvider.EnterSyncWriteScope() brackets Scripture, settings, extension-data and comment mutations — i.e. every project write in the app — plus ~10 more EnterWrite sites across ManageBooksService, InventoryDataProvider and CheckRunner. Against that, roughly 10 hand-written TS call sites across two extensions know the sentinel exists.

Every other write path — the Settings tab, share-layout.dialog, platform-bible-toolbar, resource-text-panel, use-buffered-layout-setting, find.web-view, and anything added later — gets a raw technical error or a silent unhandled rejection. Nothing fails when a new site forgets; the omission surfaces only when a user hits it during a real sync.

Extracting the shared helper is a genuine improvement over five divergent copies, so this isn't an objection to the PR's change — but it makes the pattern cheaper to spread, which is the wrong direction long-term. A single interception point (a coded error from the gate per the sync-edit-blocked.util.ts comment, checked once in the PAPI request layer or a PDP write wrapper) would cover every write path including the ones nobody has thought of yet.

(AI-assisted, with my guidance)

🤖 Claude: Agreed that per-call-site education doesn't scale and that the failure mode of the
current shape is invisible omissions — your inventory of uncovered write surfaces matches ours.
This is deferred to PT-4214 alongside finding 18 (cited by comment id there) rather than started
now, because the single interception point is only buildable once the gate rejection is machine-
readable: today a centralized layer would be regex-matching message text for every request,
which globalizes the sentinel fragility instead of removing it. Sequence on the ticket: coded
error with a discriminating subcode first (finding 18), then one checkpoint in the papi request
layer or a PDP write wrapper mapping it to the shared "editing paused" notification, then delete
the per-site matchers. The helper extraction this PR did is treated there as the interim
containment it is, not the end state.

… oldest-first pairing Review findings 1-3 on #2571: pairing anonymous clears with the oldest outstanding leash both releases a live blocker (a late clear after a leash fired cancels a newer switch's leash) and starves an abandoned blocker's own leash (every clear cancels the leash nearest to firing, so blocking persists indefinitely under ongoing traffic). Both stores now hand out one release token per block/switch. A token is released exactly once - by its own clear or its own leash - and a late clear after the leash fired is a no-op by identity. The dual counter and the timer-array scan dissolve (findings 14/15): the live-token set is the single source of truth. The project-switch events now carry a switchId (declared in NetworkEvents, emitted by platform-scripture-editor), so the workspace-updating service pairs every finish with exactly the switch that started it - both review traces are fixed end-to-end and pinned by integration tests that fail on the old code. The auto-sync blocking event remains an anonymous boolean, so that service keeps oldest-first pairing with tombstones for leash-released raises (fixes the late-clear trace); the abandoned-raise trace is unfixable on an anonymous wire and is resolved by the backend-authoritative snapshot model in #2574, as documented in the service. Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the command is registered nowhere at this tip; the doomed consult cost ~9 s of main-process retry churn per renderer launch because requestNoRetry's flag does not cross the wire - now documented on requestNoRetry itself). The "Known limitation" reload note is restored, pointing at PT-4214, whose C# SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting the seed also makes initAutoSyncBlockingService synchronous again, so the index.tsx startup comment is accurate at this tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review findings 9/19 on #2571: the hook asserted the true promise-returning setter type internally, passed the value through untouched, then declared it `=> void` at the signature - hiding every dropped write rejection from the compiler. `resetSetting` was worse: its wrapper discarded the promise inside the hook, so consumers could not observe a failed reset at all. The declared tuple now matches runtime: setSetting returns the DataProviderUpdateInstructions promise and resetSetting returns Promise<boolean>. Inventory's two `await Promise.resolve(...)` workarounds (no-ops - await adopts thenables regardless of the declared type) are deleted along with their incorrect type-erasure comments, and the generic Setting component now awaits the setter so a write-gate rejection reaches its existing catch and surfaces as the error message its author intended. Typecheck across all workspaces is clean - the widening from `void` broke no consumer. papi.d.ts regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… pin, key privacy, constant doc Review findings 7/8/12/16 on #2571: - checks-side-panel: the allow/deny catch rethrew non-sentinel errors into a fire-and-forget caller (check-card does not await), recreating the exact unhandled-rejection hazard the catch documents. Non-sentinel failures are now logged and return false, matching this webview's other mutation paths. - sync-edit-blocked.util tests: bare-string cases added - manage-books passes AlertEntry.text, which getErrorMessage quote-wraps via JSON.stringify, so the deliberately unanchored regex is now pinned against the anchoring hardening that would silently kill that path. - SYNC_EDIT_BLOCKED_MESSAGE_KEY unexported (last external consumer left in this PR) and the notification test asserts the literal localization key instead of comparing the constant to itself. - AUTO_SYNC_MAX_DURATION_MS doc names both consumers again (main's shutdown wait, renderer's per-blocker leash) with the never-diverge rule, and pins "automatic" to app-driven syncs so the name stays accurate as #2560's shared shutdown helper lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Rebase reconciliation on top of #2571's identity-pairing fix round: drop the anonymous-boolean integration test whose event seam (onAutoSyncBlockingChanged + raise/clear pairing) this branch deletes - the snapshot store makes the pairing problem it pinned unrepresentable - and update the index.tsx startup comment, which #2571 made accurate again by cutting the inert seeding: this branch reintroduces the (now C#-served) init consult, so the services are no longer both synchronous (review finding 13 on #2571). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…h a self-catching helper Add notifySyncEditBlocked() to sync-edit-blocked.util.ts - the platform-scripture twin of the scripture editor's helper of the same name: it awaits papi.notifications.send inside try/catch and logs a failure, so fire-and-forget callers can never leak an unhandled promise rejection, and the shared "editing paused during Send/Receive" message and warning severity cannot drift per call site. Route all five bare send call sites through it: - checks-side-panel.web-view.tsx (deny/allow catch paths) - inventory.web-view.tsx (approved/unapproved items catch paths) - manage-books.web-view.tsx (mutation-result toast routing; this one was pre-existing - its send sat inside a try/catch without await, so the catch only ever saw synchronous throws. The non-blocked generic toast there now handles rejections with .catch for the same reason.) Also adopt the runtime promise `useProjectSetting`'s setters return in inventory's two try blocks (the hook's declared setter type erases it, so a bare await has no effect on the type and the catch looked dead). Unit tests cover the helper's happy path and that a rejected send is swallowed and logged. Addresses lyonsil's re-review note N1 on #2555. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…URATION_MS Since #2555 hoisted it to platform.data.ts, the constant bounds both the main process's shutdown-sync wait and the renderer's auto-sync edit-block safety leash - the "shutdown" name no longer fit half its uses, and its doc comment had to spend two lines reconciling that. Name it for what it bounds (the maximum duration of a single automatic Send/Receive) and simplify the doc comments at both consumers. papi.d.ts regenerated (npm run build:types) - the constant is exported there. Addresses lyonsil's re-review note N2 on #2555. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…or auto-sync blocking Two hardenings from #2555's review (PT-4214 Stage T): Finding 8: the auto-sync-blocking store's safety timer hard-reset blockCount to 0 when it fired, so under overlapping blocks one stale expiry wiped newer in-flight blockers (and a block released by the reset could later be released again by its own clear, stealing another blocker's count). Each raise now arms its own one-shot leash that releases only its own block, exactly once; a clear cancels the oldest outstanding leash so a cleared block's leash can never fire later. Same flaw, same fix in the near-twin workspace-updating-store (30 s leash). The two fixes are deliberately symmetrical without a shared abstraction - that extraction is deferred to PT-4214 Stage U (reviewer-agreed). Finding 7 (mitigation): the blocking service's protocol was event-only, so a renderer reload during an in-flight scheduled sync came up unblocked while the backend was still syncing - the documented limitation in auto-sync-blocking-service.ts. Implement the documented upgrade path: on init, query the emitter's current blocking state (paratextBibleSendReceive.getAutoSyncBlocking, requested untyped like cancelSync in shutdown-tasks) and seed the store from it. Best-effort: a live event always wins over the snapshot, a failed request keeps the assume-unblocked default, and the per-blocker leash bounds a seeded block whose clearing event never arrives. Registering the command on the Send/Receive extension side remains for PT-4214. Tests: overlapping blockers where a stale leash expires while a newer block is active (count stays correct, UI stays blocked until the newer one clears), clear-pairs-with-oldest-leash, and the seeding paths (seeds on true, skips false/non-boolean/failure, event wins over snapshot, no seeding after cleanup). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… oldest-first pairing Review findings 1-3 on #2571: pairing anonymous clears with the oldest outstanding leash both releases a live blocker (a late clear after a leash fired cancels a newer switch's leash) and starves an abandoned blocker's own leash (every clear cancels the leash nearest to firing, so blocking persists indefinitely under ongoing traffic). Both stores now hand out one release token per block/switch. A token is released exactly once - by its own clear or its own leash - and a late clear after the leash fired is a no-op by identity. The dual counter and the timer-array scan dissolve (findings 14/15): the live-token set is the single source of truth. The project-switch events now carry a switchId (declared in NetworkEvents, emitted by platform-scripture-editor), so the workspace-updating service pairs every finish with exactly the switch that started it - both review traces are fixed end-to-end and pinned by integration tests that fail on the old code. The auto-sync blocking event remains an anonymous boolean, so that service keeps oldest-first pairing with tombstones for leash-released raises (fixes the late-clear trace); the abandoned-raise trace is unfixable on an anonymous wire and is resolved by the backend-authoritative snapshot model in #2574, as documented in the service. Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the command is registered nowhere at this tip; the doomed consult cost ~9 s of main-process retry churn per renderer launch because requestNoRetry's flag does not cross the wire - now documented on requestNoRetry itself). The "Known limitation" reload note is restored, pointing at PT-4214, whose C# SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting the seed also makes initAutoSyncBlockingService synchronous again, so the index.tsx startup comment is accurate at this tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review findings 9/19 on #2571: the hook asserted the true promise-returning setter type internally, passed the value through untouched, then declared it `=> void` at the signature - hiding every dropped write rejection from the compiler. `resetSetting` was worse: its wrapper discarded the promise inside the hook, so consumers could not observe a failed reset at all. The declared tuple now matches runtime: setSetting returns the DataProviderUpdateInstructions promise and resetSetting returns Promise<boolean>. Inventory's two `await Promise.resolve(...)` workarounds (no-ops - await adopts thenables regardless of the declared type) are deleted along with their incorrect type-erasure comments, and the generic Setting component now awaits the setter so a write-gate rejection reaches its existing catch and surfaces as the error message its author intended. Typecheck across all workspaces is clean - the widening from `void` broke no consumer. papi.d.ts regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… pin, key privacy, constant doc Review findings 7/8/12/16 on #2571: - checks-side-panel: the allow/deny catch rethrew non-sentinel errors into a fire-and-forget caller (check-card does not await), recreating the exact unhandled-rejection hazard the catch documents. Non-sentinel failures are now logged and return false, matching this webview's other mutation paths. - sync-edit-blocked.util tests: bare-string cases added - manage-books passes AlertEntry.text, which getErrorMessage quote-wraps via JSON.stringify, so the deliberately unanchored regex is now pinned against the anchoring hardening that would silently kill that path. - SYNC_EDIT_BLOCKED_MESSAGE_KEY unexported (last external consumer left in this PR) and the notification test asserts the literal localization key instead of comparing the constant to itself. - AUTO_SYNC_MAX_DURATION_MS doc names both consumers again (main's shutdown wait, renderer's per-blocker leash) with the never-diverge rule, and pins "automatic" to app-driven syncs so the name stays accurate as #2560's shared shutdown helper lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@experimental

Mark the shared auto-sync duration bound as @experimental and regenerate papi.d.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

Rebase reconciliation on top of #2571's identity-pairing fix round: drop the anonymous-boolean integration test whose event seam (onAutoSyncBlockingChanged + raise/clear pairing) this branch deletes - the snapshot store makes the pairing problem it pinned unrepresentable - and update the index.tsx startup comment, which #2571 made accurate again by cutting the inert seeding: this branch reintroduces the (now C#-served) init consult, so the services are no longer both synchronous (review finding 13 on #2571). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

rolfheij-sil requested review from irahopkinson, lyonsil and tjcouch-sil as code owners July 16, 2026 17:31

lyonsil reviewed Jul 16, 2026

View reviewed changes

This was referenced Jul 16, 2026

PT-4214 Stage U: backend-authoritative per-project S/R block signal (E2E defects 2+3) #2574

Open

PT-4214: blocked-state UI reflection — Comments panel + project settings (E2E defect 4) #2575

Open

rolfheij-sil and others added 8 commits July 17, 2026 17:35

chore(types): regenerate papi.d.ts after platform.data.ts doc restora…

033f6a8

…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs(shared): tag AUTO_SYNC_MAX_DURATION_MS @experimental

b99d516

Mark the shared auto-sync duration bound as @experimental and regenerate papi.d.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

rolfheij-sil force-pushed the pt-4159-edit-block-followups branch from 37d955d to b99d516 Compare July 17, 2026 15:35

		@@ -32,3 +33,22 @@ export const SYNC_EDIT_BLOCKED_MESSAGE_KEY = '%webView_platformScripture_error_s
		export function isSyncEditBlockedError(error: unknown): boolean {

Uh oh!

Conversation

rolfheij-sil commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Self-catching sync-edit-blocked notification helper (re-review N1)

2. Rename SHUTDOWN_SYNC_TIME_OUT_MS → AUTO_SYNC_MAX_DURATION_MS (re-review N2)

3. Blocking-store hardening (PT-4214 Stage T; original findings 7/8)

Verification

Fix round — lyonsil's 2026-07-16 full-diff review (19 findings, review 4718215827)

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rolfheij-sil commented Jul 16, 2026 •

edited

Loading

2. Rename `SHUTDOWN_SYNC_TIME_OUT_MS` → `AUTO_SYNC_MAX_DURATION_MS` (re-review N2)