New Context Menu Item + Wsh Command to Save Scrollback of a Terminal Widget#2892
New Context Menu Item + Wsh Command to Save Scrollback of a Terminal Widget#2892
Conversation
…scrollback function to use it
WalkthroughAdds a new Cobra subcommand termscrollback (Go) that fetches terminal scrollback by block OID, supports start/end ranges, a last-command mode, and optional file output. Adds extensive SKILL.md documenting how to add wsh commands and inserts termscrollback docs into docs/docs/wsh-reference.mdx twice (duplicate). Adds an Electron IPC handler "save-text-file" and exposes api.saveTextFile in preload. Frontend: new "Save Session As..." menu item, TermWrap.getScrollbackContent(), a bufferLinesToText utility for converting terminal buffer lines to text (appears duplicated), and a types entry for saveTextFile. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if (line) { | ||
| const lineText = line.translateToString(true); | ||
| // If this line is wrapped (continuation of previous line), concatenate without newline | ||
| if (line.isWrapped && !isFirstLine) { |
There was a problem hiding this comment.
CRITICAL: Logic bug with wrapped lines at range start
When line.isWrapped is true AND isFirstLine is true, this condition evaluates to false, causing the code to start a new logical line (line 351) instead of concatenating. This can occur when startIndex begins in the middle of a wrapped line sequence.
Example scenario:
- Buffer line 5: "This is a very long line that gets w" (not wrapped)
- Buffer line 6: "rapped to the next line" (isWrapped = true)
- If
startIndex = 6, the first line processed hasisWrapped = trueandisFirstLine = true - The condition
line.isWrapped && !isFirstLineis false, so it starts a new line instead of treating it as a continuation
Fix: Change the condition to handle the first wrapped line correctly:
if (line.isWrapped) {
currentLine += lineText;
} else {
if (!isFirstLine) {
lines.push(currentLine);
}
currentLine = lineText;
}
isFirstLine = false;This ensures wrapped lines are always concatenated, regardless of whether they're the first line in the range.
Code Review SummaryStatus: Issues Previously Identified | Recommendation: Address existing comments before merge OverviewThis PR adds terminal scrollback functionality with a new
All critical issues have been previously identified and commented on inline. No new issues were found in this review pass. Previously Identified Issues (10 comments)
Files Reviewed (9 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.kilocode/skills/add-wshcmd/SKILL.md (1)
545-545: Duplicate#### Documentationheadings (MD024)The heading
#### Documentationis repeated verbatim for Examples 2 and 3 (lines 545 and 646). While intentional, it triggers MD024. Either qualify the headings (e.g.#### Documentation (Example 2)) or add an.markdownlint.jsonat the repo/skills level with"MD024": { "siblings_only": true }.Also applies to: 646-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.kilocode/skills/add-wshcmd/SKILL.md at line 545, The duplicate "#### Documentation" headings trigger markdownlint rule MD024; fix it by either renaming the repeated headings (e.g., change the second and third "#### Documentation" to "#### Documentation (Example 2)" and "#### Documentation (Example 3)") or add a markdownlint configuration at the skills level to relax MD024 for sibling headings (create an .markdownlint.json containing the MD024 setting and set "siblings_only": true), and update the SKILL.md occurrences of the heading text "#### Documentation" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.kilocode/skills/add-wshcmd/SKILL.md:
- Around line 344-380: The markdown example blocks that wrap inner fenced code
(the block beginning with "## mycommand" and its example sections containing
lines like "wsh mycommand [args] [flags]" and the inner ```sh fences) must use
four backticks for the outer fence instead of three to avoid nested-fence
ambiguity; update the outer fences around those example blocks (and the other
similar example blocks noted in the comment) from ``` to ```` so the inner ```sh
code fences remain valid and the MD040 linter warnings are resolved.
In `@cmd/wsh/cmd/wshcmd-termscrollback.go`:
- Around line 66-68: The error message from the view-type check in the
TermScRollback handler is ambiguous when the map assertion fails; update the
logic around metaData[waveobj.MetaKey_View] / viewType / ok so you distinguish
"key missing or not a string" from "present but not 'term'": if !ok return an
error that the view key is missing or not a string (include the raw value via a
safe %#v of metaData[waveobj.MetaKey_View] or note it's absent), otherwise when
viewType != "term" return an error stating the unexpected view type and include
viewType and fullORef.OID to aid debugging.
- Around line 82-99: The command implementation in wshcmd-termscrollback.go
currently prints errors directly to os.Stderr (the fmt.Fprintf calls around the
`err` checks that follow the terminal scrollback fetch and the os.WriteFile) and
then returns the same error, causing duplicate output because `rootCmd` does not
silence errors; remove those manual fmt.Fprintf prints and instead return
wrapped errors with context (e.g., return fmt.Errorf("getting terminal
scrollback: %w", err) and return fmt.Errorf("writing to %s: %w",
termScrollbackOutputFile, err)) so Cobra prints a single message, or
alternatively set `rootCmd.SilenceErrors = true` in root command initialization
if you prefer Cobra to be fully responsible for error output.
In `@docs/docs/wsh-reference.mdx`:
- Around line 807-812: Docs and frontend are inconsistent: the bottom-up
indexing in term-wsh.tsx (using totalLines - endLine and totalLines - startLine)
makes the current defaults produce an empty range; update docs in
docs/wsh-reference.mdx to describe that --start N skips the N most-recent lines
and --end N includes up to N lines from the most-recent (with 0 meaning "all" in
docs), and change the frontend handler in frontend/app/view/term/term-wsh.tsx to
treat endLine===0 as a sentinel for "all lines" (e.g. set endLine = totalLines
when endLine === 0 before computing startBufferIndex/endBufferIndex) and ensure
startLine/endLine clamping uses Math.min/Math.max consistently so
startBufferIndex <= endBufferIndex and the non-lastcommand branch returns all
lines by default.
In `@frontend/app/view/term/term-model.ts`:
- Around line 918-941: The click handler currently silently no-ops when
this.termRef.current is null; update the click callback in the same handler (the
function that calls this.termRef.current.getScrollbackContent()) to surface a
user-facing message via modalsModel.pushModal (similar to the !content branch)
when termRef.current is null (e.g., push a MessageModal stating the terminal is
unavailable or could not be accessed) so users receive consistent feedback; keep
the existing flow for the content and error branches unchanged.
In `@frontend/app/view/term/term-wsh.tsx`:
- Around line 165-170: The current calculation treats data.lineend==0 as zero
instead of a sentinel for "all lines", causing bufferLinesToText to receive an
empty range; update the endLine computation so that when data.lineend === 0 you
set endLine = totalLines, otherwise use Math.min(totalLines, data.lineend); then
recompute startBufferIndex/endBufferIndex (they remain totalLines - endLine and
totalLines - startLine) and pass those to bufferLinesToText (function name:
bufferLinesToText, variables: startLine, endLine, startBufferIndex,
endBufferIndex) so the default CLI/UI invocation returns the full scrollback.
---
Nitpick comments:
In @.kilocode/skills/add-wshcmd/SKILL.md:
- Line 545: The duplicate "#### Documentation" headings trigger markdownlint
rule MD024; fix it by either renaming the repeated headings (e.g., change the
second and third "#### Documentation" to "#### Documentation (Example 2)" and
"#### Documentation (Example 3)") or add a markdownlint configuration at the
skills level to relax MD024 for sibling headings (create an .markdownlint.json
containing the MD024 setting and set "siblings_only": true), and update the
SKILL.md occurrences of the heading text "#### Documentation" accordingly.
| viewType, ok := metaData[waveobj.MetaKey_View].(string) | ||
| if !ok || viewType != "term" { | ||
| return fmt.Errorf("block %s is not a terminal block (view type: %s)", fullORef.OID, viewType) |
There was a problem hiding this comment.
Ambiguous error when the view-type assertion fails.
When !ok (key absent or non-string value), viewType is "", producing "block ... is not a terminal block (view type: )". It's worth distinguishing the two cases:
📝 Proposed fix
- viewType, ok := metaData[waveobj.MetaKey_View].(string)
- if !ok || viewType != "term" {
- return fmt.Errorf("block %s is not a terminal block (view type: %s)", fullORef.OID, viewType)
- }
+ viewType, ok := metaData[waveobj.MetaKey_View].(string)
+ if !ok {
+ return fmt.Errorf("block %s is not a terminal block (no view type in metadata)", fullORef.OID)
+ }
+ if viewType != "term" {
+ return fmt.Errorf("block %s is not a terminal block (view type: %s)", fullORef.OID, viewType)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| viewType, ok := metaData[waveobj.MetaKey_View].(string) | |
| if !ok || viewType != "term" { | |
| return fmt.Errorf("block %s is not a terminal block (view type: %s)", fullORef.OID, viewType) | |
| viewType, ok := metaData[waveobj.MetaKey_View].(string) | |
| if !ok { | |
| return fmt.Errorf("block %s is not a terminal block (no view type in metadata)", fullORef.OID) | |
| } | |
| if viewType != "term" { | |
| return fmt.Errorf("block %s is not a terminal block (view type: %s)", fullORef.OID, viewType) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/wsh/cmd/wshcmd-termscrollback.go` around lines 66 - 68, The error message
from the view-type check in the TermScRollback handler is ambiguous when the map
assertion fails; update the logic around metaData[waveobj.MetaKey_View] /
viewType / ok so you distinguish "key missing or not a string" from "present but
not 'term'": if !ok return an error that the view key is missing or not a string
(include the raw value via a safe %#v of metaData[waveobj.MetaKey_View] or note
it's absent), otherwise when viewType != "term" return an error stating the
unexpected view type and include viewType and fullORef.OID to aid debugging.
| Flags: | ||
|
|
||
| - `-b, --block <blockid>` - specify target terminal block (default: current block) | ||
| - `--start <line>` - starting line number (0 = beginning, default: 0) | ||
| - `--end <line>` - ending line number (0 = all lines, default: 0) | ||
| - `--lastcommand` - get output of last command (requires shell integration) |
There was a problem hiding this comment.
--start/--end descriptions are inverted relative to the implementation.
The docs say --start 0 = "beginning" (implying oldest/top) and --end 0 = "all lines". The frontend handler in term-wsh.tsx implements a bottom-up coordinate system:
const startBufferIndex = totalLines - endLine;
const endBufferIndex = totalLines - startLine;With the defaults (linestart=0, lineend=0), endLine = Math.min(totalLines, 0) = 0, so startBufferIndex = endBufferIndex = totalLines — an empty range. Running wsh termscrollback with no flags produces zero output, directly contradicting the claim that it "retrieves all lines from the current terminal block."
The actual semantics appear to be:
--start N: skip the N most-recent lines (0 = include all recent lines)--end N: include up to N lines counting from the most recent (0 currently produces nothing, but docs imply it means "all lines")
The docs should be corrected, and the frontend handler needs to handle the lineend=0 sentinel:
📝 Suggested doc correction + code fix
Update docs:
- - `--start <line>` - starting line number (0 = beginning, default: 0)
- - `--end <line>` - ending line number (0 = all lines, default: 0)
+ - `--start <line>` - lines to skip from the most recent (0 = include most recent, default: 0)
+ - `--end <line>` - total lines to return counting from the most recent (0 = all lines, default: 0)Fix in frontend/app/view/term/term-wsh.tsx (non-lastcommand branch):
- const endLine = Math.min(totalLines, data.lineend);
+ const endLine = data.lineend === 0 ? totalLines : Math.min(totalLines, data.lineend);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/docs/wsh-reference.mdx` around lines 807 - 812, Docs and frontend are
inconsistent: the bottom-up indexing in term-wsh.tsx (using totalLines - endLine
and totalLines - startLine) makes the current defaults produce an empty range;
update docs in docs/wsh-reference.mdx to describe that --start N skips the N
most-recent lines and --end N includes up to N lines from the most-recent (with
0 meaning "all" in docs), and change the frontend handler in
frontend/app/view/term/term-wsh.tsx to treat endLine===0 as a sentinel for "all
lines" (e.g. set endLine = totalLines when endLine === 0 before computing
startBufferIndex/endBufferIndex) and ensure startLine/endLine clamping uses
Math.min/Math.max consistently so startBufferIndex <= endBufferIndex and the
non-lastcommand branch returns all lines by default.
| click: () => { | ||
| if (this.termRef.current) { | ||
| const content = this.termRef.current.getScrollbackContent(); | ||
| if (content) { | ||
| fireAndForget(async () => { | ||
| try { | ||
| const success = await getApi().saveTextFile("session.log", content); | ||
| if (!success) { | ||
| console.log("Save scrollback cancelled by user"); | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to save scrollback:", error); | ||
| const errorMessage = error?.message || "An unknown error occurred"; | ||
| modalsModel.pushModal("MessageModal", { | ||
| children: `Failed to save session scrollback: ${errorMessage}`, | ||
| }); | ||
| } | ||
| }); | ||
| } else { | ||
| modalsModel.pushModal("MessageModal", { | ||
| children: "No scrollback content to save.", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent no-op when termRef.current is null lacks user feedback
If this.termRef.current is null, the click handler exits silently — no modal, no toast. This is asymmetric with the !content branch which surfaces a MessageModal. While the terminal ref should always be set when a context menu is triggered, the inconsistency is surprising.
🛡️ Proposed fix
- if (this.termRef.current) {
- const content = this.termRef.current.getScrollbackContent();
- if (content) {
- // ...
- } else {
- modalsModel.pushModal("MessageModal", {
- children: "No scrollback content to save.",
- });
- }
- }
+ const content = this.termRef.current?.getScrollbackContent();
+ if (content) {
+ // ...
+ } else {
+ modalsModel.pushModal("MessageModal", {
+ children: "No scrollback content to save.",
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| click: () => { | |
| if (this.termRef.current) { | |
| const content = this.termRef.current.getScrollbackContent(); | |
| if (content) { | |
| fireAndForget(async () => { | |
| try { | |
| const success = await getApi().saveTextFile("session.log", content); | |
| if (!success) { | |
| console.log("Save scrollback cancelled by user"); | |
| } | |
| } catch (error) { | |
| console.error("Failed to save scrollback:", error); | |
| const errorMessage = error?.message || "An unknown error occurred"; | |
| modalsModel.pushModal("MessageModal", { | |
| children: `Failed to save session scrollback: ${errorMessage}`, | |
| }); | |
| } | |
| }); | |
| } else { | |
| modalsModel.pushModal("MessageModal", { | |
| children: "No scrollback content to save.", | |
| }); | |
| } | |
| } | |
| click: () => { | |
| const content = this.termRef.current?.getScrollbackContent(); | |
| if (content) { | |
| fireAndForget(async () => { | |
| try { | |
| const success = await getApi().saveTextFile("session.log", content); | |
| if (!success) { | |
| console.log("Save scrollback cancelled by user"); | |
| } | |
| } catch (error) { | |
| console.error("Failed to save scrollback:", error); | |
| const errorMessage = error?.message || "An unknown error occurred"; | |
| modalsModel.pushModal("MessageModal", { | |
| children: `Failed to save session scrollback: ${errorMessage}`, | |
| }); | |
| } | |
| }); | |
| } else { | |
| modalsModel.pushModal("MessageModal", { | |
| children: "No scrollback content to save.", | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/term-model.ts` around lines 918 - 941, The click
handler currently silently no-ops when this.termRef.current is null; update the
click callback in the same handler (the function that calls
this.termRef.current.getScrollbackContent()) to surface a user-facing message
via modalsModel.pushModal (similar to the !content branch) when termRef.current
is null (e.g., push a MessageModal stating the terminal is unavailable or could
not be accessed) so users receive consistent feedback; keep the existing flow
for the content and error branches unchanged.
| } | ||
|
|
||
| lines.reverse(); | ||
| const startBufferIndex = totalLines - endLine; |
There was a problem hiding this comment.
CRITICAL: Incorrect buffer index calculation
The conversion from line numbers to buffer indices is backwards. According to the documentation, startLine and endLine are line numbers from the beginning (0 = first line). In xterm.js, buffer index 0 is also the oldest/first line.
Current calculation:
- If user requests lines 100-200 from a 1000-line buffer
startBufferIndex = 1000 - 200 = 800endBufferIndex = 1000 - 100 = 900- This returns lines 800-900, NOT lines 100-200
The correct conversion should be:
const startBufferIndex = startLine;
const endBufferIndex = endLine;This will cause the wsh termscrollback --start X --end Y command to return completely wrong line ranges.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/view/term/termutil.ts (1)
324-333: JSDoc missing caveat about sub-range starting mid-logical-line.When
startIndex > 0and the buffer line atstartIndexhasisWrapped === true(i.e., the range begins in the middle of a logical line), the first element of the returned array will be a partial logical line — the prefix that starts beforestartIndexis silently dropped. This is acceptable for the primary callers (full-buffer export passesstartIndex = 0), but callers using arbitrary sub-ranges should know this.📝 Suggested JSDoc addition
* `@param` startIndex - Starting buffer index (inclusive, 0-based) + * If the line at startIndex has isWrapped=true (i.e., the range begins + * mid-logical-line), the first returned element will be a partial line. * `@param` endIndex - Ending buffer index (exclusive, 0-based)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termutil.ts` around lines 324 - 333, Update the JSDoc for the function that "Converts terminal buffer lines to text" to note that when startIndex > 0 and the buffer row at startIndex has isWrapped === true the returned first element may be a partial logical line because the prefix before startIndex is omitted; reference the parameters startIndex and endIndex and the isWrapped property so callers know this behavior and advise callers to adjust startIndex backwards to the start of a logical line (or treat the first returned line as potentially truncated) when extracting arbitrary sub-ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/termutil.ts`:
- Around line 344-360: The loop building logical lines uses
line.translateToString(true) which right-trims every physical row and can drop
trailing spaces on wrapped rows; change the concatenation logic in the for-loop
(the code using buffer.getLine, line.isWrapped, currentLine, isFirstLine, and
lines.push) so that for any physical row that is followed by another wrapped row
you call translateToString(false) (to preserve trailing spaces) and only call
translateToString(true) for the final physical row of a logical line; detect the
“followed by another wrapped row” case by peeking buffer.getLine(i+1) (or
checking that line.isWrapped is true and the next row also exists/isWrapped) and
use that to choose translateToString(false) vs translateToString(true) when
building currentLine.
---
Nitpick comments:
In `@frontend/app/view/term/termutil.ts`:
- Around line 324-333: Update the JSDoc for the function that "Converts terminal
buffer lines to text" to note that when startIndex > 0 and the buffer row at
startIndex has isWrapped === true the returned first element may be a partial
logical line because the prefix before startIndex is omitted; reference the
parameters startIndex and endIndex and the isWrapped property so callers know
this behavior and advise callers to adjust startIndex backwards to the start of
a logical line (or treat the first returned line as potentially truncated) when
extracting arbitrary sub-ranges.
| for (let i = clampedStart; i < clampedEnd; i++) { | ||
| const line = buffer.getLine(i); | ||
| if (line) { | ||
| const lineText = line.translateToString(true); | ||
| // If this line is wrapped (continuation of previous line), concatenate without newline | ||
| if (line.isWrapped && !isFirstLine) { | ||
| currentLine += lineText; | ||
| } else { | ||
| // This is a new logical line | ||
| if (!isFirstLine) { | ||
| lines.push(currentLine); | ||
| } | ||
| currentLine = lineText; | ||
| isFirstLine = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
translateToString(true) on intermediate wrapped rows may silently drop a trailing space.
translateToString(true) right-trims each physical buffer row before concatenation. When xterm.js hard-wraps exactly at a word boundary (e.g., "…Hello " occupies the last column of the row), trimming strips the trailing space. The next row is then appended directly, producing "…HelloWorld" instead of "…Hello World".
The fix is to use translateToString(false) for every physical row that is immediately followed by another wrapped row (i.e., not the last physical row of the logical line), and reserve translateToString(true) for the final physical row only:
🐛 Proposed fix
for (let i = clampedStart; i < clampedEnd; i++) {
const line = buffer.getLine(i);
if (line) {
- const lineText = line.translateToString(true);
+ // Trim trailing whitespace only on the last physical row of a logical
+ // line; intermediate wrapped rows may carry a significant trailing
+ // space at the wrap boundary.
+ const nextLine = i + 1 < clampedEnd ? buffer.getLine(i + 1) : undefined;
+ const trimRight = !nextLine?.isWrapped;
+ const lineText = line.translateToString(trimRight);
// If this line is wrapped (continuation of previous line), concatenate without newline
if (line.isWrapped && !isFirstLine) {
currentLine += lineText;
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/termutil.ts` around lines 344 - 360, The loop building
logical lines uses line.translateToString(true) which right-trims every physical
row and can drop trailing spaces on wrapped rows; change the concatenation logic
in the for-loop (the code using buffer.getLine, line.isWrapped, currentLine,
isFirstLine, and lines.push) so that for any physical row that is followed by
another wrapped row you call translateToString(false) (to preserve trailing
spaces) and only call translateToString(true) for the final physical row of a
logical line; detect the “followed by another wrapped row” case by peeking
buffer.getLine(i+1) (or checking that line.isWrapped is true and the next row
also exists/isWrapped) and use that to choose translateToString(false) vs
translateToString(true) when building currentLine.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/app/view/term/term-wsh.tsx (2)
12-14: Remove unuseddebug/dlog— wrong namespace and dead code.
dlogis never called anywhere in this file (neither is thedebugimport used transitively). The namespace"wave:vdom"is also wrong for a terminal scrollback module. Both declarations are dead code.♻️ Proposed removal
-import debug from "debug"; -const dlog = debug("wave:vdom"); - export class TermWshClient extends WshClient {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term-wsh.tsx` around lines 12 - 14, Remove the unused debug import and the dlog constant declaration (debug and dlog) from this file — they are dead code and use the wrong namespace ("wave:vdom") for the terminal scrollback module; simply delete the import line `import debug from "debug";` and the `const dlog = debug("wave:vdom");` declaration so no unused symbols remain.
130-147: Consider returning an empty result when no prompt markers exist.When
promptMarkers.length === 0the code falls through tostartBufferIndex=0 / endBufferIndex=totalLines, silently returning the full scrollback as "last command" output. Shell integration is confirmed active at this point (the== nullguard above), but no command has been run yet — returning the entire buffer as a "command" result is misleading.♻️ Suggested early-return for the zero-markers case
let startBufferIndex = 0; let endBufferIndex = totalLines; if (termWrap.promptMarkers.length > 0) { + // ... existing marker logic ... + } else { + // Shell integration active but no commands run yet + return { totallines: totalLines, linestart: 0, lines: [], lastupdated: termWrap.lastUpdated }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term-wsh.tsx` around lines 130 - 147, The current logic treats no prompt markers as a full-scrollback "last command"; change the branch handling termWrap.promptMarkers so that if termWrap.promptMarkers.length === 0 you return an empty result immediately (instead of using startBufferIndex = 0 and endBufferIndex = totalLines). Update the function that contains startBufferIndex/endBufferIndex (reference termWrap.promptMarkers, startBufferIndex, endBufferIndex, totalLines) to short-circuit and return an empty command output when promptMarkers is empty, keeping the existing logic for length > 0 unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/term-wsh.tsx`:
- Around line 150-155: The trim logic mixes logical lines from bufferLinesToText
with a physical index (startBufferIndex): replace the current calculation of
returnStartLine = startBufferIndex + (lines.length - 1000) with a mapping that
converts the trimmed logical-start to the corresponding physical buffer index
(i.e., count how many physical lines were dropped or walk the original physical
buffer starting at startBufferIndex to find the physical index that corresponds
to the new logical-first line). Also normalize the coordinate semantics so both
the lastcommand path and the non-lastcommand path return the same coordinate
type (prefer returning an absolute buffer index like linestart) and update any
callers accordingly; use identifiers returnLines, returnStartLine,
startBufferIndex, bufferLinesToText, lastcommand, and linestart to locate and
change the logic.
---
Duplicate comments:
In `@frontend/app/view/term/term-wsh.tsx`:
- Around line 166-167: The conditional handling for the end line sentinel is
correct: update endLine calculation in the term rendering logic to use
data.lineend === 0 ? totalLines : Math.min(totalLines, data.lineend); ensure
startLine remains Math.max(0, data.linestart) and that variables startLine and
endLine are used consistently in rendering/iteration paths (e.g., in functions
that read or slice lines) so the `--end 0` case returns all lines as intended.
---
Nitpick comments:
In `@frontend/app/view/term/term-wsh.tsx`:
- Around line 12-14: Remove the unused debug import and the dlog constant
declaration (debug and dlog) from this file — they are dead code and use the
wrong namespace ("wave:vdom") for the terminal scrollback module; simply delete
the import line `import debug from "debug";` and the `const dlog =
debug("wave:vdom");` declaration so no unused symbols remain.
- Around line 130-147: The current logic treats no prompt markers as a
full-scrollback "last command"; change the branch handling
termWrap.promptMarkers so that if termWrap.promptMarkers.length === 0 you return
an empty result immediately (instead of using startBufferIndex = 0 and
endBufferIndex = totalLines). Update the function that contains
startBufferIndex/endBufferIndex (reference termWrap.promptMarkers,
startBufferIndex, endBufferIndex, totalLines) to short-circuit and return an
empty command output when promptMarkers is empty, keeping the existing logic for
length > 0 unchanged.
frontend/app/view/term/term-wsh.tsx
Outdated
|
|
||
| let returnLines = lines; | ||
| let returnStartLine = startLine; | ||
| let returnStartLine = startBufferIndex; | ||
| if (lines.length > 1000) { | ||
| returnLines = lines.slice(lines.length - 1000); | ||
| returnStartLine = startLine + (lines.length - 1000); | ||
| returnStartLine = startBufferIndex + (lines.length - 1000); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=go -C4 'linestart|LineStart|GetScrollbackLines'Repository: wavetermdev/waveterm
Length of output: 11438
🏁 Script executed:
# Let's also look at the full context of the function in question
fd -n "term-wsh.tsx" -type fRepository: wavetermdev/waveterm
Length of output: 294
🏁 Script executed:
# Search for lineStart usage in the TypeScript/JavaScript files
rg -n 'lineStart|linestart' --type=ts --type=tsxRepository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
fd 'term-wsh.tsx' --type fRepository: wavetermdev/waveterm
Length of output: 99
🏁 Script executed:
# Look at the full context around lines 150-155 in term-wsh.tsx
fd 'term-wsh.tsx' --type f -x cat -n {} | head -200Repository: wavetermdev/waveterm
Length of output: 8293
🏁 Script executed:
# Also search for bufferLinesToText to understand the line conversion
rg -n 'bufferLinesToText' --type tsRepository: wavetermdev/waveterm
Length of output: 805
🏁 Script executed:
sed -n '334,380p' frontend/app/view/term/termutil.tsRepository: wavetermdev/waveterm
Length of output: 2047
🏁 Script executed:
# Let's also check how the Go backend uses the linestart value in both paths
rg -n -B5 -A10 'effectiveLineEnd = result.LineStart' pkg/aiusechat/tools_term.goRepository: wavetermdev/waveterm
Length of output: 570
🏁 Script executed:
# Search for places where linestart is interpreted or used after being returned
rg -n 'LineStart|linestart' pkg/aiusechat/tools_term.go -A2 -B2Repository: wavetermdev/waveterm
Length of output: 1819
🏁 Script executed:
# Check if there are other consumers of this RPC response
rg -n 'TermGetScrollbackLines' frontend/ -A3 -B3Repository: wavetermdev/waveterm
Length of output: 2129
🏁 Script executed:
# Search for other places that call TermGetScrollbackLinesCommand and use the result
rg -n 'TermGetScrollbackLinesCommand' frontend/ -A5 -B2Repository: wavetermdev/waveterm
Length of output: 772
🏁 Script executed:
# Also check the CLI to see how it uses linestart
rg -n 'result.LineStart\|result\.linestart' cmd/wsh/ -B3 -A3Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Search for all places that use result.linestart after the RPC call
rg -n 'result\.linestart\|result\.LineStart' --type ts --type tsx -B3 -A3Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
# Let's trace through the AI use case which actually uses LineStart
rg -n 'getTermScrollbackOutput\|TermGetScrollbackLinesCommand' pkg/aiusechat/tools_term.go -B5 -A15Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Search for usages of linestart in TypeScript files (with a simpler approach)
rg -n 'result\.linestart' frontend/ -B2 -A2Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Check the full context of the AI tool that uses TermGetScrollbackOutput
sed -n '84,150p' pkg/aiusechat/tools_term.goRepository: wavetermdev/waveterm
Length of output: 2068
🏁 Script executed:
# Let me check the full context to understand the trim calculation better
sed -n '148,165p' frontend/app/view/term/term-wsh.tsxRepository: wavetermdev/waveterm
Length of output: 653
🏁 Script executed:
# And verify the exact coordinates being used - check both buffer indices used
sed -n '125,180p' frontend/app/view/term/term-wsh.tsxRepository: wavetermdev/waveterm
Length of output: 2427
🏁 Script executed:
# Let me verify one more thing - check if linestart from lastcommand path is actually used anywhere
# that would interpret it as scroll-from-bottom
rg -n 'lastcommand.*true\|LastCommand.*true' frontend/ -B5 -A10Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
# Also check to confirm that nextStart is used for the NEXT request
sed -n '118,160p' pkg/aiusechat/tools_term.goRepository: wavetermdev/waveterm
Length of output: 1532
returnStartLine trim calculation mixes logical and physical line counts; coordinate semantics are inconsistent between code paths.
The trim logic at line 155 computes returnStartLine = startBufferIndex + (lines.length - 1000), but this is incorrect when terminal lines wrap. The bufferLinesToText function merges wrapped physical lines into logical lines, so lines.length is a logical line count. When added to startBufferIndex (a physical buffer index), the result is inaccurate — for example, if 50 wrapped physical lines produce 25 logical lines, the offset calculation would be off by 25 lines.
Additionally, the lastcommand path returns linestart as an absolute buffer index (0 = oldest line), while the non-lastcommand path returns it as a scroll-from-bottom coordinate (0 = newest line). Although these paths are currently isolated, this semantic inconsistency could lead to bugs if the response handling is refactored or reused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/term-wsh.tsx` around lines 150 - 155, The trim logic
mixes logical lines from bufferLinesToText with a physical index
(startBufferIndex): replace the current calculation of returnStartLine =
startBufferIndex + (lines.length - 1000) with a mapping that converts the
trimmed logical-start to the corresponding physical buffer index (i.e., count
how many physical lines were dropped or walk the original physical buffer
starting at startBufferIndex to find the physical index that corresponds to the
new logical-first line). Also normalize the coordinate semantics so both the
lastcommand path and the non-lastcommand path return the same coordinate type
(prefer returning an absolute buffer index like linestart) and update any
callers accordingly; use identifiers returnLines, returnStartLine,
startBufferIndex, bufferLinesToText, lastcommand, and linestart to locate and
change the logic.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/view/term/term-wsh.tsx`:
- Around line 156-163: The trimming logic for the terminal buffer incorrectly
computes a physical start line after wrapping: update the computation that sets
returnLines and returnStartLine (using the variables lines, totalLines,
endBufferIndex, and returnStartLine) so that when you slice the last 1000
logical lines you also account for wrapped-to-physical-line differences (i.e.,
compute the slice offset from logical line counts before wrapping or compute the
number of wrapped lines removed and subtract that from returnStartLine); modify
the logic around the block that currently does returnLines =
lines.slice(lines.length - 1000) and returnStartLine = (totalLines -
endBufferIndex) + (lines.length - 1000) to use the correct logical-to-physical
mapping so linestart remains accurate for pagination consumers.
…Widget (wavetermdev#2892) This pull request adds a new command-line feature for exporting terminal scrollback, improves the accuracy of scrollback extraction (especially for wrapped lines), and introduces a "Save Session As..." menu option in the frontend to make exporting session logs more user-friendly. The changes touch both the backend CLI and the frontend, ensuring users can easily capture and save terminal output for processing or archiving.
1 participant
This pull request adds a new command-line feature for exporting terminal scrollback, improves the accuracy of scrollback extraction (especially for wrapped lines), and introduces a "Save Session As..." menu option in the frontend to make exporting session logs more user-friendly. The changes touch both the backend CLI and the frontend, ensuring users can easily capture and save terminal output for processing or archiving.