PT-4162: Power-mode startup/shutdown session-sync triggers by rolfheij-sil · Pull Request #2560 · paranext/paranext-core

rolfheij-sil · 2026-07-15T15:37:21Z

What

Adds the core half of PT-4162 — scheduled startup/shutdown Send/Receive for Power mode. startup-tasks.ts and shutdown-tasks.ts gain a Power-mode branch that triggers the S/R extension's paratextBibleSendReceive.runScheduledSessionSync('startup' | 'shutdown') command instead of returning early. Simple mode is byte-for-byte unchanged.

startup-tasks.ts: Power mode calls runScheduledSessionSync('startup') via a retrying request (mirrors Simple's syncProjects retry rationale — the extension may not have activated yet this early in startup).
shutdown-tasks.ts: Power mode calls runScheduledSessionSync('shutdown') via a no-retry request. The existing bounded-wait scaffold (AsyncVariable + the 10-minute SHUTDOWN_SYNC_TIME_OUT_MS) is extracted into a shared runBoundedShutdownSync helper reused by both Simple's sendReceiveProjects and Power's new call — per the design doc, this is the one wait mechanism for shutdown, not a second one.

Core stays the thin trigger; the S/R extension (built separately) owns reading PT-4160's onStartupShutdown schedule subset, running the sync, raising/clearing the PT-4159 editing-block for startup, stamping lastRunAt, and opening the results view on startup conflicts. Shutdown never blocks editing and never surfaces conflicts (app is closing — PT9 parity).

Why

Design doc: 2026-07-14-pt-4162-startup-shutdown-design.md (D1–D8, Q4 answered), plus the Q4 UX decision in 2026-07-15-ux-decisions-pt-4158.md (non-blocking UI for both session syncs; editing-only block is extension-side). Builds on the Simple-mode lifecycle triggers from #2434.

Graceful degradation

runScheduledSessionSync isn't registered in plain paranext-core — it's implemented later by the paratext-bible-send-receive extension. Both core call sites use the network layer's untyped request path (the same pattern already established for cancelSync, which also isn't in CommandHandlers), catch on failure, logger.warn, and continue. A missing/rejecting command is a logged no-op — never a crash, never a blocked startup or shutdown. (Investigated adding a typed CommandHandlers declaration first; found the seam file that would need it, paratext-bible-send-receive.d.ts, isn't actually part of the compiled tsc -p tsconfig.json program — a pre-existing quirk unrelated to this change — so predeclaring the contract there would be dead code. Left the extension to own its own type contract, consistent with the design doc's D1 split.)

Test evidence

npx vitest run src/main/startup-tasks.test.ts src/main/shutdown-tasks.test.ts — 16/16 passing (extended both existing suites with Power-mode cases: fires the new command with the right boundary arg, swallows a missing/rejecting command without throwing, and a mode-gating test for values that are neither simple nor power).
npm run typecheck:core — clean (0 errors, matches baseline).
npx eslint <changed files> — clean.
npx cspell <changed files> — clean.
Full npm test — 971/971 passing across 87 files.
Full npm run lint — clean (only 2 pre-existing, unrelated warnings in an unrelated file).

Deferred (extension-side, not this PR)

Reading the PT-4160 schedule store, running the actual sync, the PT-4159 edit-block bracket, lastRunAt stamping, and startup conflict/results-view routing — all owned by the paratext-bible-send-receive extension's runScheduledSessionSync implementation (built separately).

Update (PT-4213 fold): Folded in review items 8 & 9 — runBoundedShutdownSync now returns a succeeded/failed/timed-out outcome so shutdown logging is truthful (no more misleading "complete" log on a settled-but-failed sync), and added a test covering the previously-untested timeout branch (src/main/shutdown-tasks.test.ts, 10/10 passing).

Update (live E2E boot-race fix): Live testing at app boot (2026-07-16) caught the startup session sync silently never firing — the extension host routinely takes longer to activate paratextBibleSendReceive than the shared networkService.request retry policy's ~9s ceiling, so all 10 attempts got MethodNotFound and gave up moments before the extension activated. performPowerModeStartupSync now uses its own local retry loop (requestSessionSyncWithBootRetry, built on networkService.requestNoRetry) with a 120s boot-appropriate budget: same ~1s cadence for the first 10 attempts, backing off to 2s after that; only MethodNotFound is retried, any other error aborts immediately rather than being retried blindly. The shared retry policy is untouched for every other caller. startup-tasks.test.ts gained cases for retrying past the old ceiling and succeeding late, aborting immediately on a non-MethodNotFound error, and giving up (without blocking startup) once the budget is exhausted — 10/10 passing; full suite still 975/975.

Rebase note (2026-07-16): rebased onto main after #2555 merged. One conflict, in src/main/shutdown-tasks.ts: #2555 hoisted SHUTDOWN_SYNC_TIME_OUT_MS to src/shared/data/platform.data.ts, so this branch's local export was dropped in favor of importing the constant from there (the ShutdownSyncOutcome truthful-logging work is unchanged); shutdown-tasks.test.ts now imports the constant from platform.data too. Startup/shutdown suites 20/20, targeted eslint clean. New tip: 8fac6042de2.

🤖 Generated with Claude Code

Fix round — 2026-07-17 (response to lyonsil's 29-finding review)

All findings addressed at tip 9b09025 (5 commits) except the two recorded deferrals below. Also two PR-description corrections (this section supersedes the claims above): "Simple mode byte-for-byte unchanged" was inaccurate — Simple's logging changed (complete only on success, warn on failure/timeout); and the earlier claim that the send-receive .d.ts isn't in the tsc program was wrong (it is, via typeRoots) — the real reason for the raw request path is retry semantics, now stated in code.

43b8550 — blockers C60-8/9/11/12/25: mode-gate fails closed (unreadable interfaceMode ⇒ explicit no-sync + warn, both paths; no sync-everything fallback); request timeouts classified retryable within the 120 s budget.
12c2354 — C60-1/26 (outcome contract: consumes 'synced' | 'failed' | 'skipped' from runScheduledSessionSync — ext side landed at paratext-bible-internal-extensions@c45e6e2; legacy void tolerated as 'synced'), C60-2 (comment + see deferral), C60-3 (freshness gate anchored to window-interactive time), C60-10 (quit-aware abort), C60-13 (monotonic deadline), C60-14 (documented asymmetry), C60-15 (error-class-specific warning), C60-16 (single summary warn; see deferral), C60-19/27 (rationale inlined; no dangling doc refs), C60-4 (shared typed constants), C60-7, C60-24, B4 (main.ts:233 comment corrected).
d06f4f0 — C60-6, C60-29 (field/function order; existing commits NOT rewritten to add trailers — that would disturb review anchors; all new commits carry them).
4767a35 — C60-20/21/22/23 + B5 test hardening (mutation-verified: budget test fails under attempt>=2 throw; cadence test fails under phase inversion; swallow test fails without the outer catch); fixture format derived from the producer.
9b09025 — second close-click RESTORED as the documented force-close escape hatch (our own pre-push review caught that suppressing it removed the only exit from a hung shutdown sync).
C60-17: already resolved at head by the post-PT-4159: auto-sync blocking overlay (edit block for scheduled S/R) #2555 rebase (review was against the pre-rebase revision).

Deferrals (recorded on PT-4214, Stage U scope): C60-2's full shutdown feedback/cancel UX; C60-16's registration-signal replacement for the 120 s poll.

Rebase note: the runScheduledSessionSync declaration was added to the send-receive .d.ts at its current location; when #2570 re-homes that file, this hunk moves with it.

This change is

lyonsil

Review: PT-4162 Power-mode session-sync triggers

Reviewed the full diff at max effort in an isolated worktree. The code is careful and unusually well documented, and I verified the PR's test claims independently: npx vitest run src/main/startup-tasks.test.ts src/main/shutdown-tasks.test.ts is 20/20, and npx eslint on the four changed files is clean.

29 inline comments below. Four are blockers, flagged [blocker]; the rest are correctness, test-coverage, and cleanup notes. Several are backed by mutation testing or by compiling a deliberate mutation rather than by inspection alone — those say so and show the output.

The blockers, in short:

shutdown-tasks.ts:170 — succeeded can never be false on the Power path, so 'Sync on shutdown complete' logs unconditionally. Commit 3's truthful-logging fix doesn't achieve its goal for Power mode.
startup-tasks.ts:55 — a failed platform.interfaceMode read falls through into Simple, which fires syncProjects with no IDs = a full S/R of every shared project. Triggered by the same slow-cold-boot conditions the 120s retry loop exists to fix — and the retry loop never runs.
startup-tasks.ts:202 — the per-request timeout isn't classified as retryable, so the 120s budget collapses to one attempt against a slow-but-registered handler.
shutdown-tasks.ts:125 — the 10-minute bound is the real bound (the extension disables this command's request timeout), so a stalled network can hang a preventDefault()ed window for 10 minutes with no feedback and no cancel.

Two findings I couldn't anchor inline

main.ts:233 — the call-site comment is now false for the Power path. It reads: "The S/R command is registered by the .NET data provider, which may not yet be reachable; performStartupTasks uses retrying sendCommand semantics and swallows failures internally." After this PR, a Power boot calls runScheduledSessionSync, which is registered by the send-receive extension in the extension host (not the .NET data provider), and is invoked via requestNoRetry inside a hand-rolled 120s loop — explicitly not sendCommand's retry semantics. Both leads are dead ends for anyone debugging a 120s Power boot. (main.ts isn't in this PR's diff, so this needs a follow-up or an added hunk.)

shutdown-tasks.test.ts:167 — 'swallows unexpected errors and does not throw' is non-falsifiable. The mock uses an empty webview list, so find() returns undefined, if (!projectId) return; fires at line 101, and runBoundedShutdownSync is never constructed. The only throw exercised is cancelSync's, already caught by a dedicated bare catch two lines away — so performShutdownTasks's outer try/catch, which the test is named for, is never reached. I verified: deleting that entire outer try/catch leaves all 10 tests green. Giving the mock a writable editor (as the test at line 121 does) would make it exercise the path it claims. (Outside the diff hunks, so not anchorable.)

Scope note

Two things I checked and am deliberately not raising: the no-await-in-loop suppressions match the wording of their equivalents in rpc.model.ts:204/217 and are the canonical unavoidable case for that rule; and extracting runBoundedShutdownSync is squarely in scope, since Power genuinely needs the bounded wait Simple already had.

Some comments reference the behavior of the counterpart send-receive extension work. I've described that behavior rather than quoting it, since this repo is public — happy to go into specifics on the extension PR instead.

(AI-assisted, with my guidance)

rolfheij-sil · 2026-07-17T09:40:12Z

🤖 Claude: Fixed (12c2354). The main.ts call-site comment no longer describes the old fire-and-forget / sendCommand-retry / .NET-data-provider reality. It now states that a Power boot calls performStartupTasks, which fires runScheduledSessionSync — registered by the send-receive extension in the extension host, not the .NET data provider — via requestNoRetry inside the budgeted boot-race loop (explicitly not sendCommand's retry semantics). Both dead-end leads for anyone debugging a 120s Power boot are gone. (main.ts wasn't in the original diff; the hunk is now included in the branch.)

rolfheij-sil · 2026-07-17T09:40:38Z

🤖 Claude: Fixed and now genuinely falsifiable (4767a35). The test now gives the mock a writable editor (as the line-121 test does), so the flow runs past if (!projectId) return into performSimpleModeShutdownSync, and it injects the throw from an unguarded spot — the "Syncing project on shutdown…" logger.info, which isn't wrapped in any inner try/catch — so the error can only be swallowed by performShutdownTasks's outer catch. Mutation check: deleting that outer try/catch makes performShutdownTasks reject, so await expect(performShutdownTasks()).resolves.toBeUndefined() fails (the test also asserts logger.error fired). So it now exercises the path it's named for.

rolfheij-sil · 2026-07-17T09:41:07Z

🤖 Claude: Fix round pushed (tip 9b09025). Of the 29 inline findings plus the 2 unanchored body items: the bulk (27) are fixed outright; C60-17 was already stale-by-rebase (the local SHUTDOWN_SYNC_TIME_OUT_MS const was replaced by the shared-module import when this branch rebased onto main after #2555 — your review was against the pre-rebase revision); and C60-2 and C60-16 have their code-level asks done with the larger remaining scope tracked on PT-4214 (the shutdown-window feedback/cancel UX; the event-signal replacement for the boot-race polling, which needs new RPC registration-event infra that doesn't exist yet).

All four blockers are closed:

C60-1 — core now consumes the 'synced' | 'failed' | 'skipped' result and logs truthfully; the producer landed in the send-receive extension (paratext-bible-internal-extensions@c45e6e2); legacy void tolerated during skew.
C60-2 — the extension's disabled-timeout dependency is now named in code, and the second-close force-close escape hatch was restored at 9b09025 after our own pre-push review caught an intermediate revision had removed it (silent-hang UX deferred to PT-4214).
C60-8 — the mode gate now fails closed (return + warn) instead of falling into Simple's sync-everything.
C60-9 — request timeouts are now retried within the budget.

PR-description corrections applied: the "Simple mode byte-for-byte unchanged" claim (C60-18) and the ".d.ts isn't in the tsc program" claim (C60-4) are both corrected. The mutation-backed test findings (C60-20, C60-21, C60-22, C60-23, B5) are re-pinned so the mutations you demonstrated now fail. Per-thread replies below. Two dispositions I'd value your read on: C60-14 (shutdown deliberately has no boot-race retry) and the C60-22 shared-producer vs. structured-error-code choice.

… 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>

Adds the core half of scheduled startup/shutdown Send/Receive for Power mode. startup-tasks.ts and shutdown-tasks.ts gain a Power-mode branch that triggers the S/R extension's `paratextBibleSendReceive. runScheduledSessionSync('startup' | 'shutdown')` command instead of returning early; Simple mode is unchanged. Shutdown's bounded-wait scaffold (AsyncVariable + 10-minute timeout) is extracted into a shared helper reused by both Simple's sendReceiveProjects and Power's new call, rather than adding a second wait mechanism. The command isn't registered in plain paranext-core (it's implemented later by the S/R extension), so both call sites use the same retry/no-retry untyped network-request pattern already established for cancelSync — a rejected/absent command is logged and swallowed, never a crash or a blocked startup/shutdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Address PT-4213 review items 8 and 9 for the bounded shutdown sync: - runBoundedShutdownSync now returns a succeeded/failed/timed-out outcome instead of a bare boolean, so callers only log "Sync on shutdown complete" when the sync actually succeeded. A settled-but-failed sync gets its own distinct log line instead of the previous misleading "complete" message, and a timed-out wait now logs a warning too. - Add a shutdown-tasks.test.ts case exercising the previously-untested completed === false / timed-out branch via vitest fake timers and a never-resolving sync request, plus assertions on the new truthful logging for the success and failure paths.

…retry budget Found in live E2E boot testing (2026-07-16): performPowerModeStartupSync used the shared networkService.request retry policy (MAX_REQUEST_ATTEMPTS x REQUEST_ATTEMPT_WAIT_TIME_MS in rpc.model.ts, ~9s total). The extension host was observed taking longer than that to activate the send-receive extension, so all 10 attempts got MethodNotFound and the startup session sync silently never ran, even though the extension activated moments later. Replace that call with a local retry loop (requestSessionSyncWithBootRetry) built on networkService.requestNoRetry: same ~1s cadence for the first 10 attempts (so the common fast case is unchanged), backing off to a gentler 2s cadence for a total budget of 120s. Only MethodNotFound failures are retried; any other error (a real handler failure) aborts immediately rather than being blindly retried. The shared networkService.request retry policy is untouched for every other caller. Also documents that the cleaner long-term shape - the S/R extension self-triggering its own startup sync at the end of its own activation, so core doesn't have to race extension-host startup at all - is deliberately out of scope here. Extends startup-tasks.test.ts with cases for: retrying past the shared ~9s ceiling and succeeding once the handler appears late, aborting immediately on a non-MethodNotFound error, and giving up (without blocking startup) once the retry budget is exhausted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…quest timeouts An unreadable `platform.interfaceMode` no longer falls through to Simple mode's sync-everything on either the startup or shutdown path. Both paths now skip the automatic sync and warn, symmetrically, so a failed mode read can't S/R every shared project (startup) or an arbitrary open editor (shutdown) and override a Power user's schedule (PT-4162 review C60-8, C60-11, C60-12). The variable is typed from the setting's closed `'simple' | 'power'` union, so the non-simple/non-power branch is now unreachable and is removed along with the tests that pinned that impossible state; a future third mode becomes a compile error rather than a silent no-sync (C60-25). The startup boot-race retry loop now also retries a client-side request timeout, not just MethodNotFound, so a slow-but-registered handler no longer collapses the 120 s budget to a single attempt (C60-9). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

…riggers quit/freshness-aware `runScheduledSessionSync` now returns `Promise<'synced' | 'failed' | 'skipped'>` instead of `Promise<void>`, declared in the vendored send-receive `CommandHandlers` .d.ts. Core reads that value on both boundaries and logs it truthfully — `'synced'` -> info "complete", `'failed'` -> warn, `'skipped'` -> debug — so `succeeded` is finally real and the log never claims a sync that didn't happen (C60-1). A legacy void resolution is tolerated as `'synced'`. `ShutdownSyncOutcome` is now a real, behaviour-driving 5-state union rather than decorative (C60-26); the bounded-wait returns a settlement the caller maps, and the settled-guard / mislabel-as-timeout issues in that wait are fixed while it was being rewritten (C60-28). Startup trigger hardening: - Freshness gate: a startup sync that only registers after the window has been interactive past a threshold is dropped instead of fired, so it can't raise the PT-4159 edit-block on an editor the user is already typing in. Anchored to window-interactive time from main.ts (C60-3). - Quit-aware: an AbortSignal wired to window-close / will-quit stops the boot-race loop so it can't fire startup after shutdown, and network.service.initialize() now refuses to reconnect once shutdown has begun, so a late request can't resurrect the torn-down connection (C60-10). - Monotonic deadline via performance.now() instead of wall-clock Date.now() (C60-13); the "never registered" vs "handler failed" warnings are branched on the actual error class (C60-15). Shutdown: documents why it deliberately has no boot-race retry (near-impossible at shutdown) and names the cross-repo dependency that SHUTDOWN_SYNC_TIME_OUT_MS is the only bound because the extension disables this command's request timeout (C60-14, C60-2 comment); drops the ungated "Syncing scheduled projects" info log that claimed a sync before knowing one would run (C60-7). Command name/boundary come from a shared scheduled-session-sync contract module so the two raw call sites can't drift (C60-4). The `debug` logger mock is added to the shutdown suite (C60-24). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

…elds before functions Narrative/style cleanup called out in the PT-4162 review: - Reorder startup-tasks.ts so all constants, the StartupTasksSignals interface, and the outcome type come before the functions, matching the Code-Style-Guide and the sibling shutdown-tasks.ts; this also removes a forward reference the behaviour changes introduced (C60-29). - main.ts: rewrite the false fire-and-forget comment. The Power path's trigger is registered by the send-receive extension in the extension host (not the .NET data provider) and is driven via requestNoRetry inside a bounded 120 s loop, deliberately not sendCommand's retry semantics (B4). - shutdown-tasks.test.ts: fix the never-settles test comment that cited a nonexistent `completed` variable and a review-item number, and described the wrong branch — it exercises the timeout branch, not a `succeeded === false` branch (C60-6). The doc-accuracy fixes for the reworked functions (retry-semantics rationale C60-4, "unregistered rejects, real bound is the disabled timeout" C60-5, monotonic-clock note C60-13, error-class-branched warning C60-15, inlined design-doc rationale C60-19, and the removed dangling module-doc cross-reference C60-27) landed with the behaviour rewrite in the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

…from the producer format Harden the boot-race retry tests so they actually pin the design instead of passing against mutations (all verified by temporary mutation, then reverted): - The cadence test now succeeds on a non-symmetric attempt (15) and asserts the exact elapsed fake time (18s), so deleting the two-phase backoff or inverting its phases fails the suite (C60-20). - The budget test asserts the exact attempt count (66) over STARTUP_SYNC_RETRY_BUDGET_MS instead of `> 1`, so an `if (attempt >= 2) throw` mutation — or any budget that ignores the constant — fails (C60-23). - The MethodNotFound and request-timeout fixtures now derive their message format from the shared producer (`getJsonRpcRequestErrorMessagePrefix` / `JSON_RPC_REQUEST_TIMED_OUT_MESSAGE_PREFIX` in rpc.model, used by network.service's doRequest and the startup-tasks matchers), so the format lives in one place and a reformat can't leave a stale hand-copied fixture matching (C60-22). The misleading test that used a non-MethodNotFound error under the MethodNotFound-retry name is removed; the retryable path is covered by the cadence and budget tests using the real fixture (C60-21). - shutdown 'swallows unexpected errors' now uses a writable editor plus an error from an unguarded spot so it genuinely reaches performShutdownTasks's outer try/catch — falsifiable: deleting that try/catch fails it (B5). Regenerates papi.d.ts for the two new rpc.model exports so the "Report file changes" gate passes. (C60-24, the shutdown debug-logger mock, landed with the behaviour change in the earlier commit.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

… hatch A second X-click during a running shutdown sync deliberately falls through to Electron's default close again: with the extension disabling the request timeout, the bounded wait can hold the window up to 10 minutes with no feedback, and the fall-through is the only user escape until the ticketed feedback/cancel UX lands. Also trims the abortSignal doc to the events actually wired (will-quit + window close). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

CI's Report-file-changes gate regenerates papi.d.ts and diffs; the committed copy carried pre-normalization TSDoc wrapping for the new rpc.model export. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

…D1/D5 pointers Adds the sanitized PT-4162 startup/shutdown design and the 2026-07-15 UX-decisions record under docs/specs/donna-auto-syncs/ (the tracked docs/specs convention; docs/superpowers is gitignored). These are the design records behind the session-sync lifecycle triggers in src/main/startup-tasks.ts and shutdown-tasks.ts. Restores short "Full design record: ..." pointers at the three JSDoc sites where the C60-19 fix inlined rationale in place of dangling PT-4162 design D1/D5 citations, so the inlined rationale keeps a traceable link to the full record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

@experimental

…ync @experimental Mark the drift-prevention JSON-RPC error/timeout message-prefix exports (getJsonRpcRequestErrorMessagePrefix, JSON_RPC_REQUEST_TIMED_OUT_MESSAGE_PREFIX) and the paratextBibleSendReceive.runScheduledSessionSync seam command as @experimental, and regenerate papi.d.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe

… 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>

lyonsil

Lots of good updates. I am pushing back on one fix. I may have more suggested changes after further review, but for one there is 1 blocker.

@lyonsil reviewed 10 files and all commit messages, made 1 comment, and resolved 28 discussions.
Reviewable status: 10 of 13 files reviewed, 1 unresolved discussion (waiting on rolfheij-sil).

@experimental

…pin gaps - Remove docs/specs/donna-auto-syncs design records and every code-comment reference to them and to PT-XXXX tickets / PR numbers in this PR's files (review decision: design docs stay out of the repo; the already-inlined rationale keeps the comments self-contained) - Add the fresh-window counterpart to the startup freshness-gate test so an inverted gate or zeroed window can't silently drop every real boot's sync - Add network.service.test.ts pinning the hasShutDown latch: initialize() and requests reject after shutdown() instead of resurrecting the connection - Export MAX_REQUEST_ATTEMPTS / REQUEST_ATTEMPT_WAIT_TIME_MS from rpc.model (@experimental) and derive the startup boot-race initial cadence and its test constants from them instead of re-declared literals - Complete getJsonRpcRequestErrorMessagePrefix TSDoc (@param/@returns); regenerate papi.d.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The mechanical PT-XXXX deletions left a dangling "Now that ..." temporal reference, orphaned commas before "via", and bare "the editing-block" references whose definition previously lived in the PT-4159 citation. Reword those sites so the comments stand alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

lyonsil

- I know I'm approving some of my own edits on top of Rolf's PR.

@lyonsil reviewed 12 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rolfheij-sil).

rolfheij-sil force-pushed the pt-4162-session-sync branch from 99cc694 to 8fac604 Compare July 16, 2026 17:10

lyonsil reviewed Jul 16, 2026

View reviewed changes

lyonsil mentioned this pull request Jul 16, 2026

Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571

Open

rolfheij-sil requested review from irahopkinson and tjcouch-sil as code owners July 16, 2026 23:00

rolfheij-sil mentioned this pull request Jul 17, 2026

Re-home paratext-bible-send-receive.d.ts seam, remove dead duplicate (PT-4213) #2570

Merged

rolfheij-sil and others added 11 commits July 17, 2026 17:30

rolfheij-sil force-pushed the pt-4162-session-sync branch from acaffaf to 61cebc3 Compare July 17, 2026 15:31

lyonsil reviewed Jul 17, 2026

View reviewed changes

Comment thread src/main/startup-tasks.test.ts

Comment thread src/shared/services/network.service.ts

Comment thread src/main/startup-tasks.ts Outdated

Comment thread src/shared/data/rpc.model.ts

lyonsil approved these changes Jul 17, 2026

View reviewed changes

lyonsil merged commit c02ff7b into main Jul 17, 2026
6 of 7 checks passed

lyonsil deleted the pt-4162-session-sync branch July 17, 2026 22:11

Uh oh!

Conversation

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

Uh oh!

What

Why

Graceful degradation

Test evidence

Deferred (extension-side, not this PR)

Fix round — 2026-07-17 (response to lyonsil's 29-finding review)

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Review: PT-4162 Power-mode session-sync triggers

Two findings I couldn't anchor inline

Scope note

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rolfheij-sil commented Jul 17, 2026

Uh oh!

rolfheij-sil commented Jul 17, 2026

Uh oh!

rolfheij-sil commented Jul 17, 2026

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyonsil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rolfheij-sil commented Jul 15, 2026 •

edited

Loading