fix(app): keep pane suspensions inside their islands so panel open never disposes the composer (#434) - #440
Conversation
…ver disposes the composer (#434) ChatPane builds its ToolViewCtx eagerly, in imperative body code that runs before any of its JSX (and before any of the #391 Suspense islands) exist. makeToolViewCtx() immediately invokes the harnessId callback passed to it (harnessId: deps.harnessId()), which read meta.data unguarded. Per @tanstack/solid-query's useBaseQuery, .data reads while a query is pending with no cached value call the underlying resource accessor (queryResource()), which is solid-query's Suspense-registering read. That read executed under ChatPane's own owner, which traces up through the Show in ChatPaneRoute to the Match-level Suspense that @tanstack/solid-router wraps every route match in (node_modules/@tanstack/solid-router dist/esm/Match.js:355) -- not any of the #391 islands, which only own their own JSX children and can't retroactively cover code that ran before they were created. So a slow /rpc/meta/models response suspended the whole route match, detaching and remounting ChatPane -- exactly the #346/#391 mechanism, just through an eager pre-JSX read #391 didn't touch. The fix follows the guard pattern already established in this same file (imageInput already checks meta.isPending before touching meta.data): guard harnessId the same way, so the read never fires while meta is pending and never registers a suspense boundary. No new pattern, no behavior change -- harnessId already fell back to '' whenever meta.data was undefined. Investigated but not changed: the markers/list-delay failure the prior round also isolated. dividersAt/dividersInRange only read markers.data inside JSX already wrapped by the Thread Suspense island (chat-pane.tsx line 317, from #391). Solid's JSX children are compiler-emitted getters, so these reads are lazily evaluated exactly when Suspense (and further-nested <For>/<Show> primitives) evaluate their children -- confirmed against the babel-preset-solid compiled output for this exact code shape, solid-router's Match/Outlet source, and the Thread.Messages implementation in @conciv/ui-kit-chat. No second eager, unguarded read of markers.data was found anywhere reachable from ChatPane. Empirically, an instrumented onCleanup on ChatPane showed a single mount/dispose pair (at teardown) across 10 unstarved runs of all three focus-stability tests, both before and after this fix -- consistent with the established finding that the failure needs CPU starvation to manifest, which this investigation does not reproduce. Extends panel-focus-stability.browser.test.tsx with a composer DOM identity check (captures the editor node once visible, asserts isSameNode once settled) as a standing guard against detach/remount. This does not fail on unstarved main with the bug present (confirmed by reverting the fix and rerunning), so it is a guard, not a proven red/green regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
1 participant
Root cause
panel-focus-stability.browser.test.tsxflaked under CPU starvation: the composer's input handle registered→focused→released 3-4× per panel open, with the final.focus()landing mid-teardown and the assertion timing out with focus on<body>.Three investigation rounds (two candidate mechanisms disproven by direct logging along the way — the
<Show keyed>object-literal and anattachmentsmemo leak) converged on:makeToolViewCtxinvokes itsharnessIdcallback eagerly in ChatPane's imperative body, before any JSX — and before any of the #391 Suspense islands exist. That callback readmeta.dataunguarded; per@tanstack/solid-query'suseBaseQuery, a.dataread while pending with no cached value hits the suspense-registering resource accessor. Executing under ChatPane's own owner, the suspension traces up to the Match-level Suspense@tanstack/solid-routerwraps every route match in (dist/esm/Match.js:355) — the #346/#391 detach mechanism, through an eager pre-JSX read #391's islands could not cover. A slow/rpc/meta/modelstherefore suspended the whole route match, detaching and remounting ChatPane per flap; under starvation the window is wide enough for the focus assertion to catch it.Fix
Guard the read the same way this file already guards
imageInput:meta.isPending ? '' : (meta.data?.harness.id ?? ''). No new pattern, no behavior change (harnessIdalready fell back to''while undefined). One line.Tests
Extended all three focus-stability tests with a composer DOM-identity guard (capture editor node at first-visible, assert
isSameNodeafter settle). Honest labeling: this does not fail on unstarved main (verified by reverting the fix and rerunning) — the failure needs CPU starvation — so it's a standing guard against detach/remount regressions, not a red/green proof. The starved-rig repro that characterized the failure (firefox 5/10 at --cpus 1) was retired mid-investigation by direction.The markers/list-delay test's failures from the original starved runs found no second eager read (dividers reads are island-contained via compiler-emitted lazy getters — verified against compiled output); most plausibly spillover from the meta-driven remount storm in the same browser session.
Gates
typecheck ✓, lint ✓, full @conciv/app suite
--forceserial: 28 files / 105 tests ✓, fallow audit: pass.Closes #434
🤖 Generated with Claude Code