Prioritize active terminal renderer writes#4792
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds scheduler active-targeting and coordinates pane lifecycle and global effects to set/clear active terminal output targets and update main-process PTY active state across visibility, activation, and close events. ChangesTerminal output targeting and scheduler coordination
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — adds renderer-side active terminal output prioritization so the active xterm's queued output drains before background terminal queues, complementing #4791's main-process PTY hint.
- Active terminal output priority in
takeNextDrainableEntry— scans theactiveOutputTargetsWeakSetfirst during queued-output drains, falling through to insertion-order background draining when no active terminal has drainable output. - Lifecycle management of active hints —
useTerminalPaneGlobalEffectssyncs active targets for all panes when the tab becomes active/visible, clears all when hidden/inactive, and cleans up on unmount.useTerminalPaneLifecycle'sreportActiveRendererPtyForPaneupdates priority on split focus changes and pane close, gated by tab visibility. closedPane.terminalin close callback —ClosedPaneInfonow carries the terminal reference so teardown can clear the exact xterm from the scheduler'sWeakSet.- Discard cleanup —
discardTerminalOutputalso removes the terminal fromactiveOutputTargets.
Cleanup is covered at every boundary: pane close, PTY close, output discard, tab hide/inactive, and effect unmount. The WeakSet provides automatic GC safety for replaced terminal objects, and the explicit delete calls handle the live-teardown paths.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts (1)
141-167: ⚡ Quick winConsolidate duplicated active-targeting logic.
The
syncActiveOutputTargetshelper (lines 141-152) duplicates the logic fromreportActiveRendererPtyForPaneinuse-terminal-pane-lifecycle.ts. Both iterate over panes/transports and set active state identically.♻️ Refactor to use the exported helper
+import { reportActiveRendererPtyForPane } from './use-terminal-pane-lifecycle' + export function useTerminalPaneGlobalEffects({ // ... }: UseTerminalPaneGlobalEffectsArgs): void { // ... useEffect(() => { const manager = managerRef.current - const syncActiveOutputTargets = (activePaneId: number | null): void => { - for (const pane of manager?.getPanes() ?? []) { - setActiveTerminalOutputTarget(pane.terminal, pane.id === activePaneId) - } - for (const [paneId, transport] of paneTransportsRef.current) { - const ptyId = transport.getPtyId() - if (!ptyId || ptyId.startsWith('remote:')) { - continue - } - window.api.pty.setActiveRendererPty?.(ptyId, paneId === activePaneId) - } - } if (!isActive || !isVisible || !manager) { - syncActiveOutputTargets(null) + reportActiveRendererPtyForPane(paneTransportsRef.current, manager, null, false) return } const activePane = manager.getActivePane() const activePaneId = activePane?.id ?? null - // Why: active output hints must clear every pane when a tab hides; split - // focus can change after this effect's active-pane snapshot. - syncActiveOutputTargets(activePaneId) + reportActiveRendererPtyForPane(paneTransportsRef.current, manager, activePaneId, true) return () => { - syncActiveOutputTargets(null) + reportActiveRendererPtyForPane(paneTransportsRef.current, manager, null, false) } }, [isActive, isVisible, managerRef, paneTransportsRef])🤖 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 `@src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts` around lines 141 - 167, Replace the duplicated syncActiveOutputTargets implementation with the shared helper reportActiveRendererPtyForPane from use-terminal-pane-lifecycle.ts: remove the local syncActiveOutputTargets function and instead import reportActiveRendererPtyForPane and call it with the same inputs (manager.getPanes()/manager, paneTransportsRef.current and the activePaneId or null) so setActiveTerminalOutputTarget and window.api.pty.setActiveRendererPty logic is centralized; ensure the effect still calls reportActiveRendererPtyForPane(activePaneId) when active and the cleanup/hidden path calls reportActiveRendererPtyForPane(null).
🤖 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
`@src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.ts`:
- Line 191: The test currently only asserts window.api.pty.setActiveRendererPty
was not called with ('remote:env@@pty-2', true) but can still be invoked with
other args; change the assertion to ensure no main IPC call for that remote PTY
by using
expect(window.api.pty.setActiveRendererPty).not.toHaveBeenCalledWith('remote:env@@pty-2',
expect.anything()), so any invocation carrying the 'remote:env@@pty-2' id is
rejected; update the assertion in use-terminal-pane-lifecycle.test.ts
accordingly referencing window.api.pty.setActiveRendererPty.
---
Nitpick comments:
In
`@src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.ts`:
- Around line 141-167: Replace the duplicated syncActiveOutputTargets
implementation with the shared helper reportActiveRendererPtyForPane from
use-terminal-pane-lifecycle.ts: remove the local syncActiveOutputTargets
function and instead import reportActiveRendererPtyForPane and call it with the
same inputs (manager.getPanes()/manager, paneTransportsRef.current and the
activePaneId or null) so setActiveTerminalOutputTarget and
window.api.pty.setActiveRendererPty logic is centralized; ensure the effect
still calls reportActiveRendererPtyForPane(activePaneId) when active and the
cleanup/hidden path calls reportActiveRendererPtyForPane(null).
🪄 Autofix (Beta)
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
Run ID: d8a35a67-e7e4-4559-a97e-0cd017cf06c1
📒 Files selected for processing (9)
src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-manager-types.tssrc/renderer/src/lib/pane-manager/pane-split-close.test.tssrc/renderer/src/lib/pane-manager/pane-split-close.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts
This was referenced Jun 7, 2026
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.
Summary
This PR adds renderer-side active terminal output priority. It complements #4791, which prioritizes the active local PTY before output reaches the renderer. This slice handles the next boundary: once output is already queued in the renderer, the terminal output scheduler now drains the active xterm target before older background terminal queues.
The intent is to protect typing and foreground TUI responsiveness when many agents/OpenCode-style TUIs are producing output at once, including remote/SSH terminals whose output does not use the local main-process PTY scheduler hint.
What changed
pane-terminal-output-scheduler.This does not stop reading terminals and does not increase output chunk sizes. Background terminals keep reading and queuing under the existing caps; this only changes which queued renderer target gets first chance to write when the scheduler is under load.
Why this direction
This is a small step toward the Ghostty-shaped model/view architecture we have been discussing: keep terminal state/output ingestion alive everywhere, but make foreground view work explicitly higher priority than background render work. It is intentionally narrower than a full model/view rewrite, while still moving in the same direction.
Benchmark
Command:
Final run on this branch:
For comparison, the previous merged main-side active PTY priority PR (#4791) reported:
The numbers are in the same low-ms band. The meaningful improvement in this PR is not a broad benchmark drop for local PTYs already helped by #4791; it is closing the renderer-side gap so active visible xterm writes win even when queued renderer work comes from transports that cannot use the local PTY scheduler hint, including remote/SSH paths.
Validation
pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.test.ts src/renderer/src/lib/pane-manager/pane-split-close.test.ts src/renderer/src/components/terminal-pane/use-terminal-pane-global-effects.test.ts src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.test.tspnpm typecheckpnpm exec oxlint ...touched files...pnpm exec oxfmt --check ...touched files...git diff --checkSKIP_BUILD=1 npx playwright test tests/e2e/artificial-opencode-terminal-load.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1 --reporter=jsonSummary by CodeRabbit
New Features
Bug Fixes