Repository navigation
Conversation
github-actions
Bot
added
vouch:trusted
Contributor
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR substantially restructures shared production panel infrastructure, including lazy registration, host-context state wiring, and terminal integration across ChatView and the right-panel launcher. The intended behavior is largely preserved and well tested, but the cross-cutting architectural change warrants human review. You can add or adjust custom eligibility rules. Learn more. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This refactor moves the Preview, Diff, and Terminal panels into a shared registered-panel host. Launcher labels, shortcuts, and availability are unchanged. No concrete regression has been identified, and the change appears ready to merge after normal checks.
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 31.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 22 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description thoroughly explains the problem, implementation, verification, affected surfaces, and unverified cases. However, it does not satisfy the required scope and approval section because it … | Add a link to the triaged issue or discussion with explicit maintainer approval of the direction and scope, including the approval comment. If no approval exists, obtain it before merging or explain a valid approval exemption for this chang… |
✅ Passed checks (3 passed)
-
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the primary change: opening the right-panel terminal through the panel host.
-
Full details: Description check
-
-
Explanation
The description thoroughly explains the problem, implementation, verification, affected surfaces, and unverified cases. However, it does not satisfy the required scope and approval section because it explicitly states that no maintainer has approved the panel-host direction.
Resolution
Add a link to the triaged issue or discussion with explicit maintainer approval of the direction and scope, including the approval comment. If no approval exists, obtain it before merging or explain a valid approval exemption for this change.
-
✨ Finishing Touches
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
-
-
-
-
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
saphid
force-pushed
the
stack/03-terminal-panel
branch
3 times, most recently
from
October 6, 2026 16:03
bdb5405 to
fe0a090
Compare
Contributor
Author
|
Review requested
Logged so this PR shows when a maintainer was asked to review it. |
saphid
force-pushed
the
stack/03-terminal-panel
branch
5 times, most recently
from
October 10, 2026 02:13
8701762 to
cdb4f58
Compare
saphid
force-pushed
the
stack/03-terminal-panel
branch
from
October 10, 2026 04:05
cdb4f58 to
a77d211
Compare
Preview becomes the second panel on the side-panel registry that Diff started. Each definition now also carries the panel's title, icon, launcher letter, client support and unavailable copy, so the tabs, the empty launcher and the add menu read one ordered list instead of three hand-kept ones. Labels, letters, order and copy are unchanged. Panel props are inferred from each lazily loaded body, and the caller is a closed union, so another panel's props, unknown ids and widened ids do not compile. ChatView lends the rendered panel a small host (thread, right panel visibility, composer draft target, workspace mutation id and the annotation send) instead of drilling the same props into each body; the annotation send keeps the per-render closure it had before, and PreviewView still drops a pick that settles after a thread switch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ChatView built a new PanelHost on every render, so every usePanelHost consumer re-rendered even when no host field changed. Memoize it on its fields and send annotations through onSendRef so the sender stays stable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Plain server threads reuse one ChatView, so the memoized panel host's sender could resolve to the next thread's composer when a pick settled after a switch. The latest sender now carries its thread key, and each host forwards only to a sender for its own thread. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ChatView replaced the panel host's annotation sender while rendering. If React threw that render away, an in-flight preview pick could still call its onSend, for example one that edits a queued message instead of sending a turn. Update the sender in a layout effect so only committed renders lend it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
PersistentThreadTerminalPanel, PersistentThreadTerminalDrawer, their two reconciliation helpers and the terminal launch-context types now live in apps/web/src/panels/terminal. The moved code is unchanged apart from the added export keywords; ChatView imports them and its call sites, props, memo boundaries and callbacks are untouched. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The right-panel terminal is now a registered side panel. Its body reads the thread and visibility from the panel host and keybindings from the server keybindings atom, then hands them to the unchanged memoized terminal, so ChatView renders that leave its inputs alone still skip it. ChatView passes only the terminal surface, launch context, focus request, callbacks and shortcut labels. Launcher copy, letter, order and availability are unchanged; the bottom drawer stays mounted by ChatView. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tree A terminal launch context with a null worktree path means the terminal was launched on the local checkout. The right-panel terminal treated that null as missing and fell back to the thread's worktree, so a thread that gained a worktree after the launch gave the drawer a worktree path and runtime env that did not match its cwd. Use the launch context whenever one exists, as the persistent drawer already does. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…hread's worktree A terminal summary's null worktree path means the server opened the terminal on the project checkout. Without a launch context the right-panel terminal treated that null as missing and fell back to the thread's worktree, so a thread that gained a worktree later gave the drawer a checkout cwd with a worktree path and runtime env. Fall back to the thread's worktree only when there is neither a launch context nor a summary. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
saphid
force-pushed
the
stack/03-terminal-panel
branch
from
October 10, 2026 07:06
a77d211 to
e34c8a4
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #16038 (and #15010). Review only the top 4 commits: e34c8a4.
Problem
The right-panel terminal is the last built-in right-panel surface that
ChatViewstill mounts by hand. Its component, the bottom terminal drawer and their helpers (about 600 lines) live inside the 11k-lineChatView.tsx, and the + menu and empty launcher keep a hand-written Terminal row with its own letter, icon and copy, separate from the registered Browser and Diff panels. This PR opens the terminal as a registered right panel on the panel host, with no visible change.Why this qualifies
This continues the panel host proposed in Ideas discussion #14938 (following #1377); moving the terminal onto it is a scope extension of that proposal. No maintainer has agreed to that direction yet. It is stacked on the Preview panel PR (which is stacked on #15010) and depends on both; review those first. If #14938 is declined, close this too. Previous PR in this stack: refactor(web): register the Preview side panel (#16038).
Fix
Two commits; review them separately.
refactor(web): move the thread terminal panel and drawer out of ChatView, +630/−597).PersistentThreadTerminalPanel,PersistentThreadTerminalDrawer, their two reconciliation helpers and the terminal launch-context types move toapps/web/src/panels/terminal/. Nothing inside the moved code changes except the addedexportkeywords. Mechanically checked: withexportstripped, each new file equals the parent'sChatViewlines 818–824 and 896–1480, and the remainingChatViewequals the parent minus those lines apart from its import list.panels/bundledPanels.tsxregistersterminal(title "Terminal",TerminalSquare, letter T, the same unavailable copy) with a lazyload.panels/terminal/TerminalSidePanel.tsxis the registered body. It reads the thread and visibility from the host and keybindings from the same server keybindings atomChatViewused, then renders the unchanged memoized terminal. The host value is rebuilt on everyChatViewrender, so the host is read outside the memo. That wayChatViewrenders that leave the terminal's inputs unchanged still skip it, as before.ChatViewmounts<RegisteredSidePanel id="terminal" …/>with only terminal inputs (surface, launch context, focus request, callbacks, shortcut labels). Adding terminal context to the composer still goes through the composer handle, so cursor insertion, focus and the pending-question refusal are unchanged.RightPanelTabs: the hand-written Terminal row becomesregistered("terminal")in the same slot (B T F D P L M), and the terminal tab icon reads the definition. TheonAddTerminal/terminalAvailableprops are replaced by thepanels.terminallauncher (available when a project is open). The pull-requests page lists Terminal as unavailable, as before.Size: 12 files, +1140/−644; excluding the pure move, +516/−53 (production +47/−42, the rest tests).
Evidence
Before = the Preview panel PR head (
8b92a7f06b), after = this head (a24bc61e6c). Same fixture on both: a one-commit git repo in a temporary directory, two draft threads (A and B), fresh isolated state, 1440×1000. Web =vp run devin headless Chromium; desktop = the built Electron app fromvp run build:desktop(Electron 44.4.2) with its own isolated profile. Shells use a neutral$prompt.Observed. Terminal identity and scrollback match on both revisions and both clients. The marker is
SH03-MARKER-<rev>-1followed byseq 1 300:Terminal inputonce the terminal is ready. Typing reaches the shell without a click, on both revisions.Ton the empty launcher opens a terminal. The + menu lists TerminalTin the same slot (B T F D P L M).before/after). The light close-confirm pair also differs by dialog-backdrop rasterization; it shows the same dialog but does not prove exact pixel parity.Flow videos (open → marker → collapse/reopen → A→B→A → split/close; real time, trimmed only at the ends):
Stills: before / after, web and Electron, dark and light
Remote (
vp run dev --share, this head, owned server and an unpaired browser profile). Opened the terminal from the right panel and typedecho PID=$$; echo SH03-REMOTE. Resized the window 1440×1000 → 900×900 → 1440×1000, then collapsed and reopened the panel.tput colsread 67 → 47 → 67, and 67 after reopen. The PID was the same before and after reopen (6125), andSH03-REMOTEstayed in scrollback. MP4Captured by GPT-6 Astra; checked by Claude Opus 5.5.
Checks at this head (
a24bc61e6c), re-run 2026-10-05 (CI=true, all exit 0):vp test run apps/web/src/panels apps/web/src/components/RightPanelTabs.test.tsx apps/web/src/components/RightPanelTabs.browserProfile.test.tsx apps/web/src/components/RightPanelTabs.terminal.test.tsx apps/web/src/components/ThreadTerminalDrawer.test.ts apps/web/src/components/ChatView.logic.test.ts apps/web/src/terminalUiStateStore.test.ts apps/web/src/state/terminalSessions.test.ts apps/web/src/lib/terminalContext.test.ts apps/web/src/lib/terminalCloseShortcut.test.ts apps/web/src/lib/terminalCloseConfirm.test.ts apps/web/src/lib/terminalFocus.test.ts apps/web/src/hooks/useTerminalFocus.test.ts apps/web/src/terminal-links.test.ts: 18 files, 222 tests pass. Recorded during development, with this PR's source reverted to its parent, 2 tests fail and 2 suites cannot load (their module does not exist there):ChatViewdoes;ChatViewand the server open call are not run);TerminalSidePanel.attach.test.tsxmounts the real registered terminal down to its viewport, with only the terminal transport and the WASM surface stubbed: it attaches the host's environment, thread and terminal, sends typed input to that same terminal, and follows a thread switch to another environment. It does not run a PTY, so it does not show the same process or scrollback surviving a reopen;TerminalSidePanel.test.tsx: does not re-render the terminal when the host is rebuilt with the same inputs, and follows visibility (this also fails if the terminal loses its memo).vp run --filter @t3tools/web typecheckpasses; compile-only fixtures (terminal missing its callbacks, host-ownedvisiblepassed as a prop, terminal props on Preview) produce 9 type errors against the parent.vp lint --report-unused-disable-directivesandvp fmt --checkon the touched files: pass; lint warnings equal the parent's for the same files.vp run knip:checkpasses.vp run --filter @t3tools/web buildpassed when this PR was built; the terminal body is its own 4.4 kB lazy chunk, referenced only by the launcher, and shares (does not duplicate) the terminal surface chunk. The web build,vp run build:desktopandnode scripts/release-smoke.tspassed at the top of this stack (the example plugins PR), which contains this change.Surfaces
apps/web.--shareremote-browser pass above attached, typed, resized and reopened the terminal over the wire.Not verified
RightPanelTabs.terminal.test.tsxcovers the disabled row.--sharepass was run). Mobile is not affected (no right panel).vp run build:desktoppassed on both revisions for the captures; Release Smoke was not run (no desktop, server or packaging change).Claude Opus 5.5 (build), GPT-6.1 Sol (review) and GPT-6 Astra (captures) via T3 Code
🤖 Generated with Claude Code