fix(app): isolate pane query reads behind suspense islands so the route boundary never detaches the composer (#346) - #391
Conversation
|
Warning Review limit reached
Next review available in: 30 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 (1)
📝 WalkthroughWalkthroughThe PR adds accessible suspense fallbacks for panel, quick-pane, chat, and composer content. It replaces the FAB working attribute with a busy indicator. It adds delayed-session focus coverage and waits for page-query RPC completion in the test helper. ChangesConciv loading states
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Prevents pending Solid Query reads from suspending route DOM and disrupting composer focus.
Changes:
- Adds a suspense-safe
settledDatahelper. - Guards session, model, metadata, marker, and catalog reads.
- Adds a browser regression test for delayed session loading.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
apps/conciv/src/data/settled-data.ts |
Adds the guarded query-data reader. |
apps/conciv/src/routes/__root.tsx |
Guards launcher session state. |
apps/conciv/src/routes/quick.tsx |
Guards quick-pane session usage. |
apps/conciv/src/routes/panel.$sessionId.tsx |
Guards panel session lookup. |
apps/conciv/src/pane/chat-pane.tsx |
Guards pane query results. |
apps/conciv/src/composer/session-selector.tsx |
Guards session rows. |
apps/conciv/src/composer/model-selector.tsx |
Guards model and session metadata. |
apps/conciv/src/composer/actions.tsx |
Guards harness metadata. |
packages/embed/test/panel-focus.it.test.ts |
Adds the delayed-response focus regression test. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const host = await serveHost(() => | ||
| hostPage({apiBase: suite.kit().base, widget: '{"quickTerminal":false,"transport":"fetch"}'}), | ||
| ) | ||
| const page = await suite.browser().newPage() | ||
| const releaseSessionList = await holdFirstSessionList(page) | ||
| await page.goto(host.base, {waitUntil: 'domcontentloaded'}) | ||
| try { | ||
| await expectLocator(page.getByRole('button', {name: 'Open conciv chat'})).toBeVisible({timeout: 15_000}) | ||
| } finally { | ||
| releaseSessionList() | ||
| } | ||
| await openPanel(page) | ||
| await expectLocator(sessionPill(page)).toBeVisible({timeout: 30_000}) | ||
| await expectLocator(composer(page)).toBeFocused({timeout: 10_000}) | ||
| await page.keyboard.type('typed after the session list resolved') | ||
| await expectLocator(composer(page)).toHaveText('typed after the session list resolved') | ||
| await page.close() | ||
| await host.close() |
92a4cfc to
eb12fad
Compare
omridevk
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/extension-testkit/src/get-extension-test-api.ts`:
- Around line 63-67: Update the dispose callback to guarantee the cleanup
sequence continues when closeBrowser() or close() fails: run close() in a
finally block after the browser-close attempt, and stop() in a nested/final
finally so it always executes. Preserve the first cleanup error and rethrow it
only after stop() completes, while retaining the existing timeout call and
cleanup order.
In `@packages/harness-testkit/src/call-tool.ts`:
- Around line 123-128: Update the approval-pump handling around drainApprovals
so failures are not unconditionally swallowed. Suppress only the expected stream
error triggered by abort.abort(), while propagating other failures—including
permissionDecision deadline rejections—through the existing deadline('testkit
approval pump drain', ...) flow. Add test coverage for a rejected permission
decision.
In `@packages/harness-testkit/src/deadline.ts`:
- Around line 3-16: Update deadline to accept a work function receiving an
AbortSignal instead of an already-started PromiseLike, create an
AbortController, and pass its signal to the work function. Abort the controller
when the timer expires, while preserving resolution and rejection cleanup; also
add late-result cleanup so a result produced after timeout is stopped or
otherwise disposed when cancellation is unsupported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa5255fb-9554-494c-905b-994fc8607a12
📒 Files selected for processing (17)
apps/conciv/src/pane/chat-pane.tsxapps/conciv/src/routes/panel.$sessionId.tsxapps/conciv/src/routes/quick.tsxapps/conciv/src/shell/fab-robot.tsxapps/conciv/src/shell/fab.tsxapps/conciv/src/shell/pending.tsxapps/conciv/src/styles.csspackages/extension-testkit/src/boot-server.tspackages/extension-testkit/src/get-extension-test-api.tspackages/harness-testkit/package.jsonpackages/harness-testkit/src/call-tool.tspackages/harness-testkit/src/deadline.tspackages/harness-testkit/src/session.tspackages/harness-testkit/src/testkit.tspackages/harness-testkit/src/until.tspackages/harness-testkit/test/deadline.test.tspackages/harness-testkit/test/until.test.ts
| const pump = drainApprovals(rpc, stream, onApproved).catch(() => {}) | ||
| try { | ||
| return await run() | ||
| } finally { | ||
| abort.abort() | ||
| await pump.catch(() => {}) | ||
| await deadline('testkit approval pump drain', TESTKIT_DEADLINE_MS, pump) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not discard approval-pump failures.
Line 123 converts every drainApprovals failure into fulfillment. If rpc.chat.permissionDecision() exceeds its deadline, line 128 observes a successful pump. The test then fails later with an unrelated timeout, or it can return a successful run() result.
Preserve non-abort errors from the approval pump. Suppress only the expected stream error caused by abort.abort(). Add coverage for a rejected permission decision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-testkit/src/call-tool.ts` around lines 123 - 128, Update the
approval-pump handling around drainApprovals so failures are not unconditionally
swallowed. Suppress only the expected stream error triggered by abort.abort(),
while propagating other failures—including permissionDecision deadline
rejections—through the existing deadline('testkit approval pump drain', ...)
flow. Add test coverage for a rejected permission decision.
| export function deadline<Result>(label: string, budgetMs: number, work: PromiseLike<Result>): Promise<Result> { | ||
| return new Promise<Result>((resolve, reject) => { | ||
| const timer = setTimeout(() => reject(new Error(`${label} exceeded ${budgetMs}ms`)), budgetMs) | ||
| Promise.resolve(work).then( | ||
| (value) => { | ||
| clearTimeout(timer) | ||
| resolve(value) | ||
| }, | ||
| (error: unknown) => { | ||
| clearTimeout(timer) | ||
| reject(error) | ||
| }, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make timed-out work cancellable.
work starts before deadline receives it. Line 5 rejects only the wrapper promise. The underlying operation continues after timeout.
A timed-out start() can later create an Engine that no caller stops. This can leave a server and temporary state directory active after the test fails.
Change deadline to accept a work function with an AbortSignal. Abort it on timeout. Add late-result cleanup for operations that cannot accept cancellation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-testkit/src/deadline.ts` around lines 3 - 16, Update
deadline to accept a work function receiving an AbortSignal instead of an
already-started PromiseLike, create an AbortController, and pass its signal to
the work function. Abort the controller when the timer expires, while preserving
resolution and rejection cleanup; also add late-result cleanup so a result
produced after timeout is stopped or otherwise disposed when cancellation is
unsupported.
eb12fad to
b4b2936
Compare
|
Second commit added after the shard-7 RCA: Latent follow-up worth a decision (not changed here): 🤖 Generated with Claude Code |
da27bba to
903d634
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…te boundary never detaches the composer (#346) solid-router wraps every route Match in a Suspense with an undefined fallback. Solid registers a suspension when a solid-query `.data` read runs inside a non-user computation (a memo or a JSX render effect) while the query is pending with no cached data, and the nearest enclosing Suspense catches it. With no inner boundary that was the route Match, so the whole subtree detached: blank shell, composer unmounted, focus lost. Per the user's direction this uses the platform rather than guarding the reads: every query-reading island now sits in its own Suspense with a real loading state (session pill, context usage, view tabs, composer actions, thread) and the reads stay plain. The root FAB's working state became a real ring element inside its own boundary, so the button, its ref and the mascot rig never suspend. The composer input shares a boundary with no query read at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… page tools (#346) waitForWidget treated "the FAB is visible" as "the widget is connected". That held only by accident: the FAB's class read sessions.data, so the whole button suspended until that query round trip finished, which was always after the page plane's page.queries subscription reached the server. #346 moved the working() read into its own Suspense island, so the FAB now paints immediately and the accidental synchronisation is gone. The first adapter.client.detect() then hit an empty page bus, got NO_PAGE_CLIENT, and the adapter's catch turned it into null. Gate the helper on the real readiness signal instead: the rpc observer's completed() for the page.queries subscription. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wright Test Main moved the embed integration suite off vitest onto @playwright/test under tests/e2e (#415) since this branch opened. Port the pending test from the old packages/embed/test/panel-focus.it.test.ts onto the new suite: native expect()/test.describe idiom, hostPage/serveHost from tests/helpers, the page.route hold of the first /rpc/sessions/list carried over unchanged.
903d634 to
bde77b5
Compare
…ver disposes the composer (#434) (#440) 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>
Closes #346. Full rework after review feedback — the earlier read-guard helper is gone; this is the platform approach.
Mechanism
solid-router wraps every route Match in
Solid.Suspensewith an undefined fallback. A solid-query.dataread from a tracked scope (memo or JSX render effect — component-body reads run untracked and never suspend) while the query is pending-with-no-cached-data registers with the NEAREST Suspense boundary. With no inner boundaries that's the route Match: the whole subtree detaches, silently. The worst case was the FAB'sclassreadingworking()(→sessions.list) — it suspended the ROOT Match, unmounting the entire widget until the session list resolved. The composer-focus flake was the same detach hitting the panel route between paint and TipTap's rAF-deferredfocus().Fix — Suspense islands with real loading states
Reads stay plain and idiomatic. Each query-reading region gets its own
<Suspense>with a designed fallback (skeleton chips,role=\"status\"+ sr-only text,motion-reducerespected): session pill, usage chip, view tabs, composer actions, thread conversation, quick-pane variants, and the FAB working-ring (fallback: the calm FAB — button, ref, aria and mascot rig sit outside the boundary and can never unmount). Nearest-boundary-wins keeps the route Match from ever tripping. Invariant verified by tree-trace: the composer input shares a boundary with no query read.Bug fixed in flight:
toolCtx.harnessIdwas an eager body-level snapshot taken whilemetawas always pending — frozen at''forever. Now a getter, read inside the Thread island.Regression test
Holds the first
/rpc/sessions/list(widget pinned to the retained fetch transport): pre-fix, even the FAB is absent from the DOM (verbatim red run in the work log); post-fix the shell paints with skeletons, the composer focuses and accepts typing while the query is still pending, and text survives the fallback→content swap.Verified
typecheck/lint/format green; @conciv/app 98/98; @conciv/embed 121/121; panel-focus suite ×3 consecutive green; fallow 0 introduced; FAB working-ring before/after frame-capture identical.
Flagged, not fixed here
rebind.it.test.ts:192frame-count flake (one occurrence, passes isolated + rerun; outside this diff's ownership).imageInput's pre-existingisPendinggate in chat-pane needspaneAttachmentsreshaped to take an accessor before the last hand-rolled guard can go.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes