fix(desktop): wait for TipTap view before DOM binding

[wesbillman]

Summary

• wait for TipTap mount/create before binding selection-tray DOM listeners
• detach, cancel queued positioning, and mark the view unavailable on unmount
• preserve fix(desktop): hide selection formatting tray on composer right-click #6683's right-click suppression behavior across editor remounts
• add a StrictMode lifecycle regression test covering mount-before-create, unmount, global updates while unmounted, remount, and cleanup

Why

#6683 synchronously accessed editor.view.dom from the tray effect. TipTap can expose an editor before EditorContent mounts its view, which throws and reaches the route-level error boundary. Green E2E did not cover that cold-mount lifecycle.

Validation

• pre-push hooks: desktop-check, desktop-typecheck, desktop-test (5,433 passed), file-size-check
• focused lifecycle regression test passed
• focused composer-selection-formatting.spec.ts right-click test passed
• E2E desktop build passed

Fixes the release-blocking regression introduced by #6683.

[wpfleger96]

Thufir, an automated reviewer, approving via Will’s GitHub account.

Reviewed exact head 430c85a1bcf7d676ad6f13bdc6834d6b3dba329e. No actionable defects found.

I traced the implementation against TipTap 3.22.5’s actual lifecycle: mount creates the view before emitting, create follows asynchronously, and unmount clears initialization and the view before notifying listeners. The change avoids every pre-view read, rebinds the right-click listeners to the current DOM after remount, marks the view unavailable before queued/global updates can execute, and removes editor, DOM, window, and RAF resources during both TipTap and React cleanup. The shape also holds under React StrictMode’s effect replay.

The regression test observes the production seam rather than passing incidentally: with only the production hunks reverted and the new test retained, the full just desktop-test suite went red with exactly this lifecycle test failing (5,432 passed / 1 failed). The PR implementation’s desktop unit-test and build steps passed in CI.

Gurney independently exercised the exact head in a production E2E web build: 36/36 throttled cold boots, 12/12 selection-tray unmount/remount cycles with updates dispatched while the old view was gone, and 12/12 right-click suppression/recovery cycles passed without a page error or root error boundary. That live lane used Chromium rather than native WebKit; the lifecycle ordering itself comes from TipTap/React, and the macOS desktop build is green.

[jedwards27]

:bot: Jude’s code review agent reviewed exact head 430c85a1bcf7d676ad6f13bdc6834d6b3dba329e against base f6e6617a9dcc2308d5039f8afaab974b49fb9577.

Blocking defect

attachEditorDomListeners is registered for both mount and create and unconditionally reads editor.view.dom (desktop/src/features/messages/ui/SelectionFormattingTray.tsx:192-203). TipTap 3.22.5 emits mount synchronously, queues create on a zero-delay timer, and only suppresses that callback when the editor is destroyed. A valid mount → unmount → delayed create sequence therefore does this:

1. mount attaches the listeners.
2. unmount nulls TipTap's editorView and this effect detaches.
3. The queued create event still fires.
4. This handler reads editor.view.dom from the unmounted view proxy and throws view is not mounted.

The exact-head implementation was reproduced failing with that lifecycle ordering using the PR's own harness; the checked-in regression does not exercise it.

Author action: gate the create callback to the currently mounted view/generation (or otherwise make it a no-op after the corresponding unmount), and add a causal mount → unmount → delayed create regression. Preserve current remount listener ownership, RAF cancellation, and right-click suppression behavior.

Verification owner: author for patch/regression; reviewer for exact-head mutation rerun.

Validation and reconciliation

• Full Desktop unit package: 5,433/5,433 pass.
• just desktop-check, just desktop-typecheck, and git diff --check: pass.
• Browser E2E build and the compiled-artifact right-click journey: 1/1 pass. The native context menu remains unprevented; the tray hides and returns after a new selection.
• Causal mutation restoring the old eager view read makes the new checked-in regression fail as intended.
• Listener cleanup, stale-DOM detachment, queued RAF cancellation, remount rebinding, toolbar semantics, and accessibility are otherwise coherent.

The product/UI lane found no independent visible or accessibility defect, but the delayed-create lifecycle reproduction is a concrete reachable ordering in pinned TipTap source and therefore overrides a clear verdict.

CI and residual risk

The required Desktop aggregate is currently red because Smoke shard 4 failed in unrelated project/thread/workflow coverage; Desktop Core, smoke shards 1–3, both integration shards, and the macOS build passed. I found no causal link from that failure to this two-file change, so it is a gate confidence gap rather than a second author defect. Native packaged Tauri/WKWebView cold-mount/remount observation was not run; browser artifact evidence reduces but does not eliminate platform-specific lifecycle/input risk.

[jedwards27]

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES
Reviewed: f6e6617a9dcc2308d5039f8afaab974b49fb9577..1047a3c0416c583150c787c055c43c6ea54d51f1 (exact head 1047a3c0416c583150c787c055c43c6ea54d51f1)
Risk: high — editor mount/unmount lifecycle can crash the route boundary.

Blocking finding

A stale TipTap isInitialized value is still trusted as proof that the view is mounted. The new createMayAttach fence correctly prevents the delayed create handler itself from reading an absent view. However, pinned TipTap 3.22.5 queues create, allows unmount to null editorView and clear initialization, then the stale callback emits create and unconditionally sets isInitialized=true. A later ordinary effect replay takes if (editor.isInitialized) attachEditorDomListeners() at desktop/src/features/messages/ui/SelectionFormattingTray.tsx:210-215, and attachEditorDomListeners reads editor.view.dom although no view exists.

A deterministic extension of the PR harness reproduced mount → unmount → stale create settles → rerender with disabled changed throwing view is not mounted. The checked-in lifecycle test stops before that effect replay.

Author action: make initial effect attachment independently prove the current view is mounted; do not use isInitialized alone. Add the full ordering regression above while retaining the direct stale-create fence, old-DOM listener/RAF cleanup, remount rebinding, and right-click behavior.

Verification owner: author for patch/regression; reviewer for exact-new-head causal mutation and affected full package rerun.

Integrated evidence

The systems lane verified that the direct delayed-create crash is fixed and mutation-proved the new fence; it also passed the full Desktop unit package (5,433/5,433), focused lifecycle regression, desktop-check, and desktop-typecheck. The product/adversarial lane extended the same lifecycle through effect replay and reproduced the remaining crash. DOM listener ownership, RAF cancellation, tray hiding, right-click behavior, and toolbar semantics were otherwise coherent.

Exact-head CI has Desktop Core, macOS build, smoke 1–3, and both integration shards green; smoke shard 4 was still running at final sampling. No packaged Tauri/WKWebView observation was completed; that is reviewer/release-owned residual risk, not an additional author defect.

Any head movement expires this review.

[jedwards27]

:bot: Jude’s code review agent — REQUEST CHANGES on exact head 1047a3c0416c583150c787c055c43c6ea54d51f1 (base f6e6617a9dcc2308d5039f8afaab974b49fb9577).

The direct delayed-create crash is fixed, but a reachable continuation of the same lifecycle ordering still crashes.

Blocking defect: stale isInitialized is treated as proof of a mounted view

TipTap 3.22.5 queues create during mount; unmount then clears editorView and sets isInitialized = false; the queued callback can subsequently emit create and unconditionally set isInitialized = true even though no view is mounted (@tiptap/core/src/Editor.ts:174-219).

The new createMayAttach fence correctly prevents that stale create handler from reading editor.view. However, a later effect replay still does this at desktop/src/features/messages/ui/SelectionFormattingTray.tsx:213-215:

if (editor.isInitialized) {
  attachEditorDomListeners(); // reads editor.view.dom
}

scheduleUpdate changes when inputs such as disabled or trayWidth change, so the effect can replay with the same editor object after mount → unmount → stale create. At that point isInitialized is true but editorView is absent, and attachEditorDomListeners() throws view is not mounted, producing the route-boundary crash this PR is intended to prevent.

I reproduced this deterministically by extending the checked-in harness with mount → unmount → stale create → rerender(disabled=true). At the exact head, the full Desktop package ran 5,433 tests: 5,432 pass, 1 fail, and the added assertion failed through SelectionFormattingTray.tsx:196 with view is not mounted. The checked-in regression currently stops before the effect replay (SelectionFormattingTray.test.mjs:114-139), so it does not cover this continuation.

Author action: make initial effect attachment independently establish that the editor view is currently mounted; do not rely on isInitialized alone. Add a deterministic regression covering mount → unmount → stale create settlement → effect replay, while preserving the direct stale-create fence, listener/RAF cleanup, remount rebinding, and right-click behavior.

Verification owner: author for the patch and regression; reviewer for exact-head causal rerun and lifecycle reconciliation.

Other evidence and confidence gaps

• Unmodified exact-head Desktop unit package: 5,433/5,433 pass.
• just desktop-check and just desktop-typecheck: pass (reported diagnostics are pre-existing and outside changed files).
• Causal removal of the new createMayAttach fence makes the focused lifecycle test fail, so the new regression does protect the prior direct defect.
• CI at final review sampling: Desktop Core, macOS build, smoke shards 1–3, and both integration shards green; smoke shard 4 still in progress. CI does not exercise the stale-initialization effect replay above.
• Packaged native Tauri/WKWebView cold-mount/remount observation remains a reviewer confidence gap, not additional author action.
• Outside the blocker, listener ownership, RAF cancellation, stale-tray hiding, right-click suppression/recovery design, and toolbar accessibility semantics remain coherent.

Any head movement invalidates this verdict.

[wesbillman]

Closing this PR to restore scope. #6618 is the approved fix for the deterministic broken-main cache failure; this separate Desktop lifecycle issue is not required for that repair and should not delay it. The latest review also identified a real remaining stale-initialization lifecycle defect, so this PR is not safe to merge as-is. We can reopen or replace it as a separately scoped follow-up if needed.