Repository navigation
feat(terminal): bound command block output - #224
Conversation
📝 WalkthroughWalkthroughThe change adds a persisted command-block maximum output-line setting. Terminal folding now enforces visible and cached-line limits, supports prefix folds, preserves markers during resizing and buffer updates, and updates folded block rendering. ChangesCommand-block limits and settings
Automatic folding and block rendering
Terminal integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalPane
participant AppStore
participant FoldStore
participant XtermTerminal
TerminalPane->>AppStore: load command-block split mode and max lines
TerminalPane->>FoldStore: create store with visible and cached limits
XtermTerminal->>FoldStore: feed lines or parsed writes
FoldStore->>XtermTerminal: remove rows and retain fold markers
FoldStore->>TerminalPane: update fold records and overlay ranges
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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a bounded command-block output mechanism in the terminal UI by adding a persisted “max output lines” setting and extending the fold store to automatically fold only the oldest output prefix while keeping the newest rows visible, all within the explicit xterm scrollback budget.
Changes:
- Add shared terminal limit constants + normalization helpers, and persist a new
command_block_max_linessetting with clamping. - Extend command-block folding to support automatic “prefix” folds with a global saved-line cache budget and reflow re-enforcement.
- Update terminal UI + settings UI/i18n to surface and apply the new limit, with accompanying unit + contract tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib/terminal/limits.ts | Defines scrollback + command-block max-line limits and normalization helpers. |
| src/lib/terminal/limits.test.ts | Adds unit tests for normalization and cache-budget invariants. |
| src/lib/terminal/folds.ts | Implements automatic prefix folding, bounded saved-line caching, and reflow re-enforcement hook. |
| src/lib/terminal/folds.test.ts | Expands fold-store tests for cache budgeting and automatic folding, including real xterm contracts. |
| src/lib/terminal/block-content.ts | Makes block text extraction include both cached prefix rows and newest visible rows for prefix folds. |
| src/lib/terminal/block-content.test.ts | Tests correct ordering for prefix-fold content resolution. |
| src/lib/stores/app.svelte.ts | Adds persisted store accessors for command_block_max_lines. |
| src/lib/stores/app.svelte.test.ts | Tests persistence + clamping behavior for the new setting. |
| src/lib/i18n/locales/zh.ts | Adds zh strings for the new setting UI. |
| src/lib/i18n/locales/en.ts | Adds en strings for the new setting UI. |
| src/lib/components/TerminalPane.svelte | Wires xterm scrollback, fold-store options, auto-refold on reflow, and unfold-on-disable behavior. |
| src/lib/components/CommandBlockSettings.svelte | Adds the numeric setting control with min/max bounds and save-on-blur behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $effect(() => { | ||
| app.commandBlockBar(); // subscribe | ||
| const visible = app.commandBlockBar(); | ||
| if (!visible) foldStore?.unfoldAll(); | ||
| fitTerminal(); |
There was a problem hiding this comment.
Fixed in 77d17dd. Re-enabling the command-block bar now calls enforceAutoFold after fitTerminal, so previously unfolded long blocks are bounded immediately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/lib/terminal/folds.ts (1)
350-355: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding
enforceAutoFoldwork when many blocks overflow.
enforceAutoFoldcallsfoldBlockOverflowfor every tracked block. Each successful fold runsremoveLines, which splices the buffer, callsenforceSavedLineBudget,term.refresh, andemit. When several blocks overflow andmaxCachedLinesis small,enforceSavedLineBudgetunfolds the oldest fold and re-inserts its rows, then the next iteration folds another block. This produces repeated splice and refresh cycles during one reflow.A cheap mitigation is to fold from the newest block backwards and stop once the cache budget is full, so older blocks are never folded and immediately unfolded.
🤖 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/lib/terminal/folds.ts` around lines 350 - 355, Update enforceAutoFold to iterate tracked blocks from newest to oldest and stop folding once the cache budget is full. Preserve the existing canAutoFold guard and foldBlockOverflow behavior, while preventing older blocks from being folded after the cache reaches maxCachedLines.src/lib/terminal/block-content.test.ts (1)
311-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an explicit
kind: "full"fold.This test covers the
"prefix"branch. The existing folded-block test uses a fixture withoutkind, so it exercises theundefinedpath at line 76 ofblock-content.ts, not the explicit"full"value thatFoldStoreproduces. Add one case withkind: "full"that asserts only the prompt and the cached rows are returned. That locks the discriminator against a future default change.🤖 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/lib/terminal/block-content.test.ts` around lines 311 - 327, Add a separate test near the existing prefix-fold test that configures foldStore.getFold to return kind: "full" with cached rows, then assert resolveBlockLines for fakeBlock returns only the prompt line and cached rows. Use the explicit full discriminator rather than omitting kind, and preserve the existing prefix test unchanged.src/lib/terminal/block-content.ts (1)
75-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the duplicated end-line resolution.
Lines 78-79 repeat the same
cursorAbsandendLinecomputation as lines 86-87. Extract a small local helper, so a future change to the end-line fallback applies to both the folded and unfolded paths.♻️ Proposed refactor
export function resolveBlockLines( term: Terminal, block: CommandBlock, foldStore?: FoldLookup, ): IBufferLine[] { if (block.start.isDisposed) return []; const buf = term.buffer.active; + const resolveEndLine = () => { + const cursorAbs = buf.baseY + buf.cursorY; + return block.end && !block.end.isDisposed ? block.end.line : cursorAbs; + }; const fold = foldStore?.getFold(block.id); if (fold) { const prompt = buf.getLine(block.start.line); const body = fold.savedLines as unknown as IBufferLine[]; const out = prompt ? [prompt, ...body] : [...body]; if (fold.kind !== "prefix") return out; - const cursorAbs = buf.baseY + buf.cursorY; - const endLine = block.end && !block.end.isDisposed ? block.end.line : cursorAbs; + const endLine = resolveEndLine(); for (let y = block.start.line + 1; y <= endLine; y++) { const line = buf.getLine(y); if (line) out.push(line); } return out; }🤖 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/lib/terminal/block-content.ts` around lines 75 - 84, Extract the shared cursor-absolute and end-line resolution from the block content logic into a local helper, then reuse it in both the folded and unfolded paths, including the existing disposed-end fallback behavior. Update the relevant code around the fold handling without changing the resulting output.src/lib/components/TerminalPane.svelte (1)
1639-1643: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
maxVisibleLinesis fixed for the pane lifetime.
commandBlockMaxLinesis read once inonMountand passed tocreateFoldStore.FoldStorenormalizes it into a constant. An open terminal therefore keeps the old limit after the user changes the setting, whileshouldAutoFolddoes react live. The tracker above documents the same lifetime rule for the split mode, so the behavior is consistent.State this in the settings UI, or extend
FoldStoreOptionsto readmaxVisibleLinesthrough a getter, so the new limit applies on the next fold check.🤖 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/lib/components/TerminalPane.svelte` around lines 1639 - 1643, Update the terminal settings UI to state that command block max visible lines changes apply only to newly opened terminals, or modify the FoldStoreOptions/maxVisibleLines handling used by createFoldStore so it reads the current setting through a getter and applies changes on the next fold check. Keep shouldAutoFold’s existing live behavior unchanged.src/lib/terminal/folds.test.ts (1)
941-942: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the
64bound in the compensation-reference assertion.The test name states that references are released. The assertion only bounds
pushedBlankRefs.lengthat 64. The origin of 64 is not stated. Derive it from the batch constant or add a short comment, so a later change toAUTO_FOLD_BATCH_LINESmakes the intent obvious.🤖 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/lib/terminal/folds.test.ts` around lines 941 - 942, Clarify the 64 bound in the assertion for store.getFold(1)?.pushedBlankRefs by deriving it from the existing AUTO_FOLD_BATCH_LINES constant or adding a concise comment that identifies this batch-size relationship, so the assertion remains understandable when the constant changes.
🤖 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/lib/components/TerminalPane.svelte`:
- Around line 238-243: Update TerminalPane.svelte#L238-L243 in fitTerminal to
compare terminal.cols and terminal.rows before and after fitAddon.fit(), and
call foldStore?.enforceAutoFold() when either the proposed or actual dimensions
changed. At TerminalPane.svelte#L1723-L1726, when visible is true, call
foldStore?.enforceAutoFold() directly rather than relying on the refit.
- Around line 1613-1616: Wrap the command block settings load using
loadCommandBlockSplitMode and loadCommandBlockMaxLines in try/catch so terminal
startup continues when either request fails. On failure, report the error with
toast.error(errMsg(e)) and fall back to the existing split-mode default plus
COMMAND_BLOCK_DEFAULT_MAX_LINES imported from src/lib/terminal/limits.ts,
allowing blockTracker, foldStore, paintScheduler, highlight loading, and
connectAndWire to run.
- Around line 284-294: Update the fold-count handling around the fold label and
unfold logic in TerminalPane so it does not report an exact hidden-line count
after enforceSavedLineBudget trims savedLines. Track the total restored lines
independently of the retained cache, or mark trimmed prefix folds as
non-unfoldable and update the label accordingly; ensure the displayed count
matches what unfold can reintegrate.
In `@src/lib/terminal/folds.test.ts`:
- Around line 946-961: Update the tests in the “FoldStore — automatic limit
(real xterm contract)” suite to run in a DOM-enabled environment such as jsdom
or a browser provider, ensuring the real `@xterm/xterm` Terminal construction
works without changing its coverage. If the suite must remain in the Node
environment, replace Terminal with a DOM-free headless emulation and retain
real-Terminal coverage in a supported environment.
---
Nitpick comments:
In `@src/lib/components/TerminalPane.svelte`:
- Around line 1639-1643: Update the terminal settings UI to state that command
block max visible lines changes apply only to newly opened terminals, or modify
the FoldStoreOptions/maxVisibleLines handling used by createFoldStore so it
reads the current setting through a getter and applies changes on the next fold
check. Keep shouldAutoFold’s existing live behavior unchanged.
In `@src/lib/terminal/block-content.test.ts`:
- Around line 311-327: Add a separate test near the existing prefix-fold test
that configures foldStore.getFold to return kind: "full" with cached rows, then
assert resolveBlockLines for fakeBlock returns only the prompt line and cached
rows. Use the explicit full discriminator rather than omitting kind, and
preserve the existing prefix test unchanged.
In `@src/lib/terminal/block-content.ts`:
- Around line 75-84: Extract the shared cursor-absolute and end-line resolution
from the block content logic into a local helper, then reuse it in both the
folded and unfolded paths, including the existing disposed-end fallback
behavior. Update the relevant code around the fold handling without changing the
resulting output.
In `@src/lib/terminal/folds.test.ts`:
- Around line 941-942: Clarify the 64 bound in the assertion for
store.getFold(1)?.pushedBlankRefs by deriving it from the existing
AUTO_FOLD_BATCH_LINES constant or adding a concise comment that identifies this
batch-size relationship, so the assertion remains understandable when the
constant changes.
In `@src/lib/terminal/folds.ts`:
- Around line 350-355: Update enforceAutoFold to iterate tracked blocks from
newest to oldest and stop folding once the cache budget is full. Preserve the
existing canAutoFold guard and foldBlockOverflow behavior, while preventing
older blocks from being folded after the cache reaches maxCachedLines.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ceee3575-b732-4c0d-ad9f-c304064c31b7
📒 Files selected for processing (12)
src/lib/components/CommandBlockSettings.sveltesrc/lib/components/TerminalPane.sveltesrc/lib/i18n/locales/en.tssrc/lib/i18n/locales/zh.tssrc/lib/stores/app.svelte.test.tssrc/lib/stores/app.svelte.tssrc/lib/terminal/block-content.test.tssrc/lib/terminal/block-content.tssrc/lib/terminal/folds.test.tssrc/lib/terminal/folds.tssrc/lib/terminal/limits.test.tssrc/lib/terminal/limits.ts
| describe("FoldStore — automatic limit (real xterm contract)", () => { | ||
| const writeP = (term: Terminal, data: string) => | ||
| new Promise<void>((resolve) => term.write(data, resolve)); | ||
|
|
||
| it("keeps a live block marker and the newest rows through a large write", async () => { | ||
| const term = new Terminal({ | ||
| cols: 80, | ||
| rows: 5, | ||
| allowProposedApi: true, | ||
| scrollback: 1_000, | ||
| }); | ||
| const tracker = createCommandBlockTracker(term); | ||
| const store = createFoldStore(term, tracker, { | ||
| maxVisibleLines: 3, | ||
| maxCachedLines: 20, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the Vitest environment configuration that applies to terminal tests.
set -euo pipefail
fd -t f -e ts -e js -e mts 'vite.config|vitest.config|vitest.workspace' --exec cat -n {}
echo "---- package.json test scripts and deps ----"
fd -t f -d 2 'package.json' --exec jq '{scripts, devDependencies}' {}
echo "---- environment docblocks in terminal tests ----"
rg -n '`@vitest-environment`|environment:' src vite.config.ts vitest.config.ts 2>/dev/null || trueRepository: shihuili1218/rssh
Length of output: 2706
🌐 Web query:
xterm.js Terminal constructor requires DOM browser globals without terminalRenderer open
💡 Result:
The xterm.js Terminal constructor and its open method are fundamentally designed for browser environments and require DOM globals (such as window, document, and an HTML element) to initialize the terminal renderer [1][2][3]. When attempting to use the standard @xterm/xterm package in a non-browser environment (like Node.js) without these DOM dependencies, you will encounter errors because the internal terminal service attempts to access browser-specific APIs to set up the rendering surface [3][4]. To use xterm.js functionality in a backend or headless context, you should use the official @xterm/headless package [5][6]. Key points for managing this requirement: 1. Use @xterm/headless: This package is explicitly maintained for Node.js environments [5][6]. It provides the core terminal logic—including buffer management and ANSI parsing—without requiring a DOM or a browser renderer [5]. 2. Addon Compatibility: Addons used with @xterm/headless must be compatible with Node.js and must not rely on DOM APIs [5][6]. 3. Architecture: If your application requires both terminal processing and visual rendering, you typically use @xterm/headless on the backend to maintain state (e.g., in a pseudoterminal context) and standard @xterm/xterm in the frontend to render that state to a DOM element [5][6]. If you are encountering a terminalRenderer error within an existing frontend project (e.g., in Electron or a browser-based app), ensure that you are calling the terminal's open method with a valid, visible DOM element only after the component or container has been mounted to the DOM [1][7][8]. Calling methods that rely on the renderer before the terminal is successfully attached to the DOM can trigger errors related to undefined rendering services [9][8].
Citations:
- 1: https://github.com/xtermjs/xterm.js/blob/master/README.md
- 2: https://github.com/xtermjs/xterm.js/
- 3: https://github.com/xtermjs/xterm.js/blob/d3fd3d0f2254a966238e575f3505d5cac31cff91/src/browser/Terminal.ts
- 4: Can I use xterm for backend only without dom object? xtermjs/xterm.js#854
- 5: https://github.com/xtermjs/xterm.js/tree/master/headless
- 6: https://cdn.jsdelivr.net/npm/@xterm/headless@6.0.0/README.md
- 7: https://www.npmjs.com/package/@xterm/xterm
- 8: https://github.com/superset-sh/superset/blob/2f2c2059/apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
- 9: The xterm.js dimensions error is a key finding - it suggests the terminal renderer is broken specifically on macOS 26 + Electron 39. microsoft/vscode#300192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- lockfile xterm entries ----"
if [ -f package-lock.json ]; then
jq -r '.packages["node_modules/@xterm/xterm"]? // empty, .packages["node_modules/xterm"]? // empty' package-lock.json
fi
if [ -f pnpm-lock.yaml ]; then
rg -n '`@xterm/xterm`|/xterm:|xterm:' pnpm-lock.yaml | head -50
fi
if [ -f yarn.lock ]; then
rg -n '`@xterm/xterm`|xterm@|node_modules/xterm' yarn.lock | head -50
fi
echo "---- source import/test setup around Terminal use ----"
rg -n 'new Terminal\(|from ["'"'"]`@xterm/xterm`["'"'"]|from ["'"'"]xterm["'"'"]|import \\(.*Terminal|environment:|jsdom' src/vitest* vitest.config.ts src/vitest* package.json 2>/dev/null || true
echo "---- terminal tests top-level ----"
sed -n '1,80p' src/lib/terminal/folds.test.tsRepository: shihuili1218/rssh
Length of output: 2937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- terminal fold tests imports and new Terminal occurrences ----"
rg -n '(^import .*Terminal|^import .*`@xterm/xterm`|new Terminal\()' src/lib/terminal/folds.test.ts
echo "---- terminal test file environment markers and Terminal import context ----"
sed -n '1,35p' src/lib/terminal/folds.test.ts
echo "---- context around real xterm contract tests ----"
sed -n '920,1045p' src/lib/terminal/folds.test.tsRepository: shihuili1218/rssh
Length of output: 5972
Run the real xterm terminal tests in a DOM-enabled environment.
vitest.config.ts sets environment: "node" for src/**/*.test.ts, but these cases import Terminal from @xterm/xterm and construct it without term.open(). The standard xterm renderer depends on browser globals, so this suite can fail in the configured node VM; use jsdom/browser provider for these tests, or replace the real Terminal with a DOM-free emulation/headless terminal and keep real-Terminal coverage in an environment that supports it.
🤖 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/lib/terminal/folds.test.ts` around lines 946 - 961, Update the tests in
the “FoldStore — automatic limit (real xterm contract)” suite to run in a
DOM-enabled environment such as jsdom or a browser provider, ensuring the real
`@xterm/xterm` Terminal construction works without changing its coverage. If the
suite must remain in the Node environment, replace Terminal with a DOM-free
headless emulation and retain real-Terminal coverage in a supported environment.
There was a problem hiding this comment.
No change. The configured Node Vitest suite constructs @xterm/xterm Terminal without opening a renderer, and the full suite passes here: 51 files and 653 tests. DOM globals are required by open(), not by the parser and buffer contract exercised by these tests.
#218
Summary
Entry points
Test plan
Summary by CodeRabbit
New Features
Bug Fixes