fix(desktop): guard broadcasts by renderer frame

[Dante-dan]

Summary

• gate shared Desktop broadcasts on the lifetime of the current main WebFrameMain
• send through the captured frame so the liveness check and delivery address the same Renderer generation
• resume normal broadcasts when Electron installs a live replacement frame, leaving the existing recovery and resynchronization paths unchanged

WebContents.isDestroyed() is insufficient during renderer-process loss and navigation because the WebContents can outlive its current main frame. Capturing the frame before checking isDestroyed() avoids asking WebContents.send() to resolve a disposed generation internally.

The frame guard is best-effort: Electron can still dispose a frame between the liveness check and send(), and internally logged send failures are not intercepted by JavaScript try/catch.

Scope: this change protects the shared, high-frequency application broadcast boundary. WorkHub presentation’s separate event-driven sends remain outside this fix; extending the helper there would change a separate delivery path. The catch stays silent because expected replacement gaps must not reintroduce repeated renderer diagnostics.

Fixes #5135

Verification

• npm run build
• node --test --test-force-exit apps/desktop/dist/main/__tests__/workhub-presentation.test.js apps/desktop/dist/main/__tests__/main-startup-lifetime.test.js (36 passed)
• npm run lint
• npm run format:check
• npm run typecheck
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npx biome check apps/desktop/src/main/main-window.ts apps/desktop/src/main/__tests__/workhub-presentation.test.ts
• git diff --check

CI follow-up

The previous Linux E2E job reported a 4683px transcript range-boundary displacement. This branch now includes current main’s visible-Turn residual anchoring correction from #5374. The unchanged transcript-scroll scenario is included in the current-main run with 37 E2E tests passing.

After merging main, build, lint, format, typecheck, both knip checks, and the existing lifecycle plus scroll-authority suites pass (61 tests). The local target Electron E2E failed at process launch with SIGABRT before running the scenario; this is not a passing local E2E result.

Review follow-up

The existing broadcast regression now also covers a destroyed-but-registered WebContents and a healthy second recipient during destroyed-frame and accessor-throw gaps. The harness frame implements isDestroyed() and send(). After this test-only change, build, lint, format, typecheck, both knip checks, and the focused WorkHub/startup suites pass (38 tests).

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Codex traced the Renderer recovery path and implemented the frame-generation broadcast guard.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[hqhq1025]

Technical GO on exact head 99135613c7c9ee00dcbd56ab3865bda4231fc707; I found no P0-P2 issues. One P3 test-coverage gap is noted inline.

The implementation fixes the actual ownership boundary: each broadcast captures the current WebFrameMain, checks that exact renderer generation, and sends through the same object. A real Electron 43.4.1/Xvfb stress probe completed 14,100 broadcasts across forcefullyCrashRenderer() and reload without the prior Error sending from webFrameMain, while broadcasts resumed after a live replacement frame appeared.

Review conclusions:

1. The solution is optimal for the reported problem and stays at the shared broadcast boundary.
2. Production code to delete: none identified.
3. Tests to delete: none; the existing broadcast test should be strengthened as described inline.
4. No deeper refactor is required.
5. The revision is technically merge-ready; the P3 is non-blocking, and the final decision remains with a maintainer.
6. Residual gaps: the exact crash/reload behavior was exercised on Linux/Xvfb, not native Windows or macOS.

This changes renderer-lifecycle IPC delivery behavior, so independent human review is required.

Verification passed build:test, full typecheck/lint/format, ASF headers, changed-file Biome, git diff --check, focused Desktop lifecycle tests 36/36, the real Electron crash/reload probe, all hosted checks, and a clean merge with current main bf6e9422960401da6bba01d03a8ea9979722bb5a. The full Desktop run was 2470 passed / 8 cancelled; those cancellations are in unchanged current-main MCP OAuth deadline tests and are not attributable to this two-file change.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

[P3] Exercise the unavailable-frame branch instead of only adapting the mock

This test now supplies mainFrame.send, but both WebContents and the frame stay permanently live. It therefore does not verify the defect being fixed: a non-destroyed WebContents whose mainFrame is destroyed or throws during renderer replacement, followed by a live replacement frame. For example, removing the frame.isDestroyed() check while keeping mainFrame.send() would leave this test green. Please drive the same registered renderer through destroyed-frame, accessor-throw, and replacement-live states, asserting no delivery/no throw during the gap and delivery after recovery.

[Dante-dan]

Automated Codex follow-up: I added the requested regression coverage in 4982416. The existing auxiliary-renderer test now drives the same registered renderer through a destroyed main frame, a mainFrame accessor that throws during replacement, and a live replacement. It verifies that both unavailable phases neither throw nor deliver, then confirms delivery resumes after replacement.

Validation:

• npm run build
• node --test --test-force-exit apps/desktop/dist/main/__tests__/workhub-presentation.test.js (29 passed)
• npm run lint, npm run format:check, npm run typecheck
• npx knip --workspace apps/desktop, npx knip --workspace packages/ui

I also temporarily bypassed the frame-destruction guard: the new no-delivery assertion failed as intended; restoring the guard returned the suite to 29/29.

[hqhq1025]

Technical GO on exact head 49824162834d0dc8b4d0af2bd0b4d25c4c1bf83c; I found no remaining P0-P3 issues.

The new test closes the previous coverage gap. It now verifies all three relevant renderer-generation states at the shared broadcast boundary: a destroyed WebFrameMain is skipped, a transient webContents.mainFrame access failure is contained, and delivery resumes exactly once after a live replacement frame appears. The production implementation is byte-identical to the previously reviewed head, so the earlier Electron 43.4.1/Xvfb stress result still applies: 14,100 broadcasts across forcefullyCrashRenderer() and reload completed without Error sending from webFrameMain, and delivery resumed after recovery. The current exact head also passed the real WorkHub crash/recovery Electron journey.

Review conclusions:

1. The solution is optimal for the reported lifecycle race and remains localized at the shared broadcast boundary.
2. Production code to delete: none identified.
3. Tests to delete or replace: none; the added regression directly exercises the formerly missing states.
4. No deeper refactor is required.
5. The revision is technically merge-ready; GitHub remains REVIEW_REQUIRED, so the final decision belongs to an independent maintainer.
6. Residual gaps: the crash/recovery path was exercised on Linux/Xvfb, not native Windows or macOS.

This changes renderer-lifecycle IPC delivery behavior, so independent human review is required.

Verification passed build:test, full typecheck/lint/format, ASF headers, changed-file Biome, renderer architecture 112/112, E2E budget 38, focused WorkHub presentation 29/29, the real Electron WorkHub crash/recovery E2E 1/1, hosted test/package, git diff --check, and a clean merge with current main f32cf2b482a17f235b92e2775ed2672c2775e219. The full Desktop run was 2470 passed / 8 cancelled; the same eight cancellations reproduce in the unchanged MCP OAuth deadline suite when run alone and are not attributable to this two-file change.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

Reviewed exact head 4f0ee7e5bb25526eaf770b08432447adbe43131f.

I found no P0-P3 issue in this two-file renderer-frame guard change. apps/desktop/src/main/main-window.ts:144-160 captures and validates each recipient's current WebFrameMain, contains the transient mainFrame access failure during renderer replacement, skips destroyed frames, and resumes delivery through the replacement generation. The merge resolution in apps/desktop/src/main/__tests__/workhub-presentation.test.ts:432-471 preserves both that three-state regression and current main's renderer-parent ownership assertions.

Exact-head validation passed: clean install with Node 24.18.1, build:test, full workspace typecheck, Desktop 2535/2535, focused WorkHub presentation 29/29, renderer architecture 112/112, E2E budget, production renderer build, lint, format, ASF headers, changed-file Biome, git diff --check, and the real WorkHub crash/recovery Electron E2E under Xvfb. A conflict-free synthetic merge onto current main cb4747171e742bb0fb8187d1f0206c1591aac982 also passed clean build, full typecheck, the focused 29/29 suite, architecture, and E2E budget checks. Hosted package succeeded.

Hosted test is red only in the unchanged transcript-scroll-cost E2E; that file and its production scroll authority are outside this PR, and the same case passed locally 2/2, so I did not attribute the failure to this change. The required gate still needs a successful rerun or separate resolution before merge.

I did not independently exercise native macOS crash recovery.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

Reviewed exact head 91cb8da6409cd72772425dfed2c6c87949840010.

I found no P0-P3 issue. This head merges current main c87651e23d2069fdaed687c785a8f965e211c15e; relative to that main, the effective PR diff remains the two-file renderer-frame guard. apps/desktop/src/main/main-window.ts:143-163 captures and validates each recipient’s current WebFrameMain, contains the transient mainFrame access failure during renderer replacement, skips destroyed frames, and resumes delivery through the replacement generation. The merged test fixture in apps/desktop/src/main/__tests__/workhub-presentation.test.ts:451-503 retains the destroyed-frame, getter-throw, and replacement-frame regression while also adopting current main’s shared native View container and renderer-parent assertions.

Exact-head validation passed with Node 24.18.1: clean install, build:test, full workspace typecheck, Desktop 2538/2538, focused WorkHub presentation 31/31, renderer architecture 112/112, E2E budget 37, production renderer build, lint, format, ASF headers, git diff --check, and the real WorkHub renderer crash/recovery Electron journey under Xvfb. Hosted test and package are both successful, and GitHub reports the PR MERGEABLE.

I did not independently exercise native macOS crash recovery.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[Astro-Han]

Review

Reviewed the two-file diff against #5135. I agree with the diagnosis and the fix lands at the right seam. No blocking issues; the points below are nits.

What I verified

• Root cause and guard placement. apps/desktop/src/main/main-window.ts:144-151 is the single shared broadcast boundary for every mainWindowController.send(...) caller (runtime-host-boot, pet-pack-import, desktop-shell-presentation, browser-view state/live). #5135's repro shows WebContents.isDestroyed() === false while the main frame is already disposed, so liveness now has to be answered by the frame — this is the correct authority.
• Liveness check and delivery address the same generation. liveMainFrame captures the WebFrameMain once and safeSendToRenderer sends through that same object, so the check cannot pass for one generation and deliver to another. contents.isDestroyed() is still checked first (main-window.ts:154), which is required since Electron throws on property access for a destroyed WebContents.
• Delivery scope is unchanged in practice. #5135's stack shows WebContents.send already delegating into WebFrameMain.send, so narrowing to contents.mainFrame matches existing behavior. I also confirmed nodeIntegrationInSubFrames is not enabled anywhere in apps/desktop, and the app's only subframe is the sandboxed srcdoc artifact iframe (apps/desktop/src/renderer/features/workbar/tools/artifacts/artifact-preview.tsx:215-218, sandbox="allow-scripts" → no preload, no window.maka). Main-frame-only delivery also matches the repo's inbound convention (event.senderFrame !== event.sender.mainFrame at app-ipc-main.ts:70, browser-ipc-main.ts:144, workhub-control.ts:327, workhub-presentation.ts:504).
• IPC/security surface. Outbound-only change: no new channels, no new inbound handlers, no change to how ...args are serialized. Nothing to validate here.
• Regression test goes through the production authority. workhub-presentation.test.ts:451-496 esbuild-bundles the real main-window.ts and runs it in a fresh context rather than reimplementing the loop, and it does fail without the change: the fake WebContents no longer defines send, so the old contents.send(...) throws and the first assert.doesNotThrow at line 488 fails. All three states (destroyed frame, throwing mainFrame getter, resumed delivery on the replacement frame) are covered, and the "exactly once" assertion at line 496 is preserved.
• Hosted test and package are green on head 91cb8da; no new dependencies; bare catch { matches the prevailing style in apps/desktop/src/main.

Nits

1. contents.isDestroyed() early return is now uncovered. The only fake registered in the test never reports destroyed, and once renderer.emit('destroyed') fires the release listener removes it from the map — so the broadcast loop never observes a destroyed WebContents. That branch (main-window.ts:154) carried over from the old code unchanged; one line (mark the fake destroyed while still registered, assert no throw and no send) would pin it.

2. No multi-recipient isolation assertion. Only one renderer is registered, so "one recipient with a dead/throwing frame does not suppress delivery to the others" is not asserted even though the loop now has a skip-and-continue shape. A second registered fake with a live frame would cover it cheaply.

3. Sibling broadcast path keeps the old predicate. apps/desktop/src/main/workhub-presentation.ts:100-103 still guards with !wc.isDestroyed() alone, plus direct sends at :120, :247, :441. Those are event-driven rather than per-session-event, so the stderr-flood risk is much lower, but the exposure window is the same one described in #5135. If "shared broadcast boundary" is the intent, exporting liveMainFrame (or a small sendToLiveMainFrame) and reusing it here would remove the duplicate predicate. Worth an explicit in/out-of-scope decision in the PR rather than a follow-up issue.

4. Harness fake mainFrame = {} is a latent trap. workhub-presentation.test.ts:66 gives the fake Contents a mainFrame with no isDestroyed/send. Harmless today because the harness controller sends via wc.send, but any future test that routes a fake through safeSendToRenderer would land in liveMainFrame's catch and silently receive nothing — a confusing "no message" failure instead of a clear TypeError. Making the fake mirror production (isDestroyed: () => false, send: (c, ...a) => this.sent.push([c, ...a])) removes the trap.

5. Optional diagnostic in the catch. main-window.ts:158-162 also swallows genuinely unexpected failures (e.g. frame.isDestroyed() throwing). The file already reports renderer lifecycle failures via console.error('[renderer] …') (:237, :243, :629); a first-occurrence-only log would keep the flood fix from #5135 while preserving debuggability. Fully optional — silence is defensible and conventional.

6. Checklist. "Tests cover the change and fail without it" is unchecked but is in fact satisfied (see above). Worth ticking.

Residual gap (not a blocker)

The guard is best-effort: a frame can be disposed between frame.isDestroyed() returning false and frame.send() executing. Per #5135, Electron logs those internally rather than throwing, so no JS-side try/catch can close that window — the guard narrows it to a very small race. Worth stating in the PR body so the fix isn't read as eliminating the class entirely.

[Dante-dan]

Automated Codex follow-up: I addressed the coverage nits in the existing regression: destroyed-but-still-registered WebContents is skipped, and a second live recipient continues receiving during both destroyed-frame and throwing-accessor gaps. The harness mainFrame now implements isDestroyed/send.

Build, lint, format, typecheck, both knip checks, and the focused WorkHub/startup suites pass (38 tests). I also checked the regression checklist item and documented the best-effort race.

I kept the sibling WorkHub event-driven sends outside this high-frequency broadcast fix; sharing the helper there would expand a separate delivery path. I left the catch silent to avoid restoring repeated diagnostics during expected renderer replacement.