Synced from GitHub
Thread transfer impact
⚠️ The latest CI run did not produce a thread transfer result for ff41fc5.
This comment will update automatically after the next completed run.
refactor(web): ChatView's persistent terminal drawers live in their own component#22681
ChatView subscribed to every thread's refs, every draft thread, and every thread's terminal-open flag, only so it could decide which terminal drawers stay mounted. Because of that, a change on any other thread re-rendered the whole chat view.
chat/ChatTerminalDrawers.tsx now holds those subscriptions, the mounted-drawer state, and the drawer presence. ChatView passes in only the active thread's values. The drawer's onAddTerminalContext now gets the stable callback; before, it got a new function on every render.
| previous layer | this layer | |
|---|---|---|
| ChatView.tsx lines | 8,585 | 8,531 |
Renders were counted with the rc-init.js fiber counter on a dev server seeded from real data (median of 3 runs; base 6a6c84f vs the stack top before its rebase onto current main):
| Interaction | main: commits / renders (ChatView) | stack top: commits / renders (ChatView) |
|---|---|---|
| Type 20 chars in the composer | 22 / 678 (1) | 22 / 678 (1) |
| Switch threads | 14 / 779 (5) | 14 / 777 (5) |
| Open + close the terminal drawer | 15 / 494 (3) | 15 / 497 (3) |
| Open + close the right panel | 21 / 723 (4) | 21 / 726 (4) |
| Drag the right panel's resize handle | 13 / 139 (1) | 1 / 1 (0) |
The +3 renders are the new ChatTerminalDrawers and ChatRightPanel wrappers. The drag row is the right panel layer: the drag's final width commit used to re-render ChatView and the whole chat column, and now re-renders only ChatRightPanel.
Background-thread measurement. Tab A keeps thread A open while a second browser (B) on the same dev server acts on another thread. Renders were counted in A, comparing the stack base (current main, ca049dc) with this stack. Only ChatView's own renders are listed. For the new-thread rows, each run waited until B's message had actually been typed and sent.
| What B does | base: ChatView renders per run | stack: ChatView renders per run |
|---|---|---|
| Streams a real Claude reply in an existing thread (30 s) | 1, 0, 1 | 0, 1, 3 |
| Sends the first message of a new thread (~20 s) | 0, 1, 0 | 1, 0, 3 |
| Nothing at all (62 s, stack only) | — | 1, 3, 1 |
Every one of those ChatView renders is the clock. Once a minute a context update re-renders ChatView along with six RelativeTimeLabels, for ~80 renders in one commit, and a second clock-driven state update (state#61) sometimes follows it. The idle row shows the same renders with nothing happening. In every run, a background thread's arrival or streaming caused no ChatView render on either side. ChatTerminalDrawers never rendered.
An earlier version of this table showed the base at 0 renders and the stack at 85 for the new-thread case. Both numbers were wrong. B's command palette had swallowed the typed prompt, so no thread was created in either run. The 85 renders were the minute tick landing inside the stack's window.
Conclusion: there is no regression and no measured win. The subscriptions this layer moves don't fire for any background event I could reproduce. The layer still keeps every-thread state out of ChatView and removes ~55 lines, so judge it as a readability change only.
Tests: vp test run src/reactCompilerCoverage.test.ts for ChatView and every new file (0 bailouts), tsc-rs --noEmit for apps/web, and oxlint on the changed files.
🤖 Generated with Claude Code
Checks
- SuccessfulCodeRabbitGitHub
Review completed
- Skipped
- Skipped
- Skipped
- SuccessfulRemove preview downloadGitHub
- SuccessfulCheckGitHub
- SkippedRelease SmokeGitHub
- Skipped
- SkippedRustGitHub
- SuccessfulBuild and TypecheckGitHub
- SuccessfulTestGitHub
- SuccessfulTest WebGitHub
- Skipped
- SuccessfulLabel PR sizeGitHub
- SuccessfulLabel PR 22680GitHub
- SuccessfulPrepare PR size configGitHub
- SuccessfulCollect PR targetsGitHub
- SuccessfulNative fingerprint diffGitHub
- SuccessfulTest Server 2GitHub
- SuccessfulTest Server 6GitHub
- SuccessfulTest Server 3GitHub
- SuccessfulTest Server 4GitHub
- SuccessfulLintGitHub
- SuccessfulTest Server 8GitHub
- SuccessfulTest Server 1GitHub
- SuccessfulTest Server 7GitHub
- SuccessfulTest Server 5GitHub
- SuccessfulChangesGitHub
- Skipped
- SuccessfulLabel PR sizeGitHub
- Successful
- SuccessfulLabel PR 22680GitHub
- Successful
- SuccessfulCollect PR targetsGitHub
- SuccessfulPrepare PR size configGitHub
- Successful
- SuccessfulNative fingerprint diffGitHub
- SuccessfulCheckGitHub
- SkippedRustGitHub
- SkippedRelease SmokeGitHub
- Skipped
- SuccessfulBuild and TypecheckGitHub
- SuccessfulTestGitHub
- SuccessfulTest WebGitHub
- SuccessfulNative fingerprint diffGitHub
- SuccessfulTest Server 2GitHub
- SuccessfulTest Server 6GitHub
- SuccessfulTest Server 1GitHub
- SuccessfulTest Server 4GitHub
- SuccessfulTest Server 3GitHub
- SuccessfulLintGitHub
- SuccessfulTest Server 8GitHub
- SuccessfulTest Server 7GitHub
- SuccessfulTest Server 5GitHub
- SuccessfulChangesGitHub
- SuccessfulNative fingerprint diffGitHub
- CancelledTestGitHub
- Cancelled
- CancelledTest WebGitHub
- CancelledRustGitHub
- CancelledBuild and TypecheckGitHub
- CancelledRelease SmokeGitHub
- FailedCheckGitHub
- SkippedEAS PreviewGitHub
- SkippedDeploy web previewGitHub
- CancelledTest Server 7GitHub
- CancelledTest Server 3GitHub
- CancelledTest Server 8GitHub
- CancelledTest Server 5GitHub
- CancelledTest Server 2GitHub
- CancelledChangesGitHub
- CancelledTest Server 1GitHub
- SkippedDeploy web previewGitHub
- CancelledTest Server 4GitHub
- CancelledTest Server 6GitHub
- CancelledLintGitHub
- SkippedEAS PreviewGitHub
- CancelledNative fingerprint diffGitHub
- Skipped
- SuccessfulLabel PR sizeGitHub
- SuccessfulLabel PR 22680GitHub
- Skipped
- CancelledLabel PR sizeGitHub
- CancelledLabel PR 22680GitHub
- SuccessfulCollect PR targetsGitHub
- SuccessfulPrepare PR size configGitHub
- SuccessfulPrepare PR size configGitHub
- SuccessfulCollect PR targetsGitHub
Added 1 commit
- refactor(web): ChatView's persistent terminal drawers live in their own componentff41fc5
Synced from GitHub
Synced from GitHub
Approvability
Verdict: Approved at ff41fc5
Macroscope's review found this PR approvable — This PR extracts existing persistent terminal-drawer subscription and rendering logic into a dedicated component without introducing a new capability or changing stored/API contracts. Its main runtime effect is isolating re-renders and stabilizing callback identity, with the existing drawer behavior preserved.
You can add or adjust custom eligibility rules. Learn more.
Synced from GitHub
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Team
- Run ID:
3f7c4f8c-c837-4314-a7ee-d2cf51e44a66
📥 Commits
Reviewing files that changed from the base of the PR and between f48841f9ef73601c5af03586ca7c334684b92fb5 and ff41fc5c94b5eecb6d9b6d17dbaebe27b3dc8906.
📒 Files selected for processing: 2
apps/web/src/components/ChatView.tsxapps/web/src/components/chat/ChatTerminalDrawers.tsx
Limit details: You’ve used all 10 included reviews currently available.
📝 Walkthrough
ChatView now delegates terminal drawer tracking and rendering to ChatTerminalDrawers. The component filters open terminal threads to existing server or draft threads, reconciles mounted drawers within the hidden-thread limit, and passes active-thread-specific state to the active drawer.
Changes: Terminal drawer mounting
Layer / File(s) Summary Track and render mounted drawers
apps/web/src/components/chat/ChatTerminalDrawers.tsxThe new component collects open terminal threads, filters out threads that no longer exist, reconciles mounted thread keys, and renders persistent drawers. Integrate drawer component
apps/web/src/components/ChatView.tsxChatView removes its cross-thread drawer mounting and presence logic. It renders ChatTerminalDrawers with active-thread state and shared terminal props. Priority: ⬇️ Low
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Refactor
Sequence Diagram(s)
mermaid1 sequenceDiagram 2 participant ChatView 3 participant ChatTerminalDrawers 4 participant TerminalUIStore 5 participant ThreadAndDraftState 6 participant PersistentThreadTerminalDrawer 7 ChatView->>ChatTerminalDrawers: Render with active-thread and shared terminal props 8 ChatTerminalDrawers->>TerminalUIStore: Read open terminal threads 9 ChatTerminalDrawers->>ThreadAndDraftState: Read server threads and drafts 10 ChatTerminalDrawers->>PersistentThreadTerminalDrawer: Render reconciled mounted drawers Suggested reviewers:
t3dotgg
Merge Risk: ⚪ Minimal · up to ff41f
The terminal drawer extraction preserves the inspected mounting and context-insertion behavior. No issue requiring a fix before merge was identified.
Pre-merge checks: Passed 4
Check name Status Explanation Title check Passed The title clearly and concisely describes the main refactor: moving persistent terminal drawers from ChatView into a separate component. Description check Passed The description clearly explains the problem, implementation, performance measurements, limitations, and verification results. It does not use the template headings and does not provide explicit scope… Linked Issues check Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request. Full details: Description check
Explanation
The description clearly explains the problem, implementation, performance measurements, limitations, and verification results. It does not use the template headings and does not provide explicit scope or maintainer-approval evidence, but it is otherwise detailed and on topic.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
📝 Generate docstrings
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Usage-based review receipt
- Mode: Continue automatically
- Reviewed files: 2
- Charged: $0.50
- View usage details
Note
This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.
Comment @coderabbitai help to get the list of available commands.