Fixed visual line counting bug in expand functionality
Public
Fixed visual line counting bug in expand functionality
Agent
Pi
Model
Claude Opus 4.5 (latest)
Started
Jan 24, 2:17 AM
Messages75
Downloads0
Reviewed GitHub issue and related code files.
UnderstandIdentified root cause in line counting due to prepended newline.
UnderstandProposed and implemented fix for visual line counting.
BuildCommitted and pushed fix.
EditDisplayed file contents on user request.
Verify
Analyze GitHub issue(s): https://github.com/badlogic/pi-mono/issues/921
For each issue:
Read the issue in full, including all comments and linked issues/PRs.
For bugs:
For feature requests:
Do NOT implement unless explicitly asked. Analyze and propose only.
The user wants me to analyze GitHub issue #921. Let me fetch the issue details first.
>gh issue view 921 --repo badlogic/pi-mono --json title,body,comments,labels,stateThis is a bug report about an off-by-one error in the "ctrl+o to expand" functionality. The issue is:
This is in the TUI package, likely related to how collapsed output is displayed. Let me search for the relevant code.
Let me search for the relevant code handling the "ctrl+o to expand" functionality:
>rg -l "ctrl\+o" --type ts /Users/badlogic/workspaces/pi-monoHmm, those are keybinding files. Let me search for "earlier lines" which is the text that appears in the message.
>rg -l "earlier lines" --type ts /Users/badlogic/workspaces/pi-monoFound it. Let me read this file in full.
Now I need to find the truncateToVisualLines function since that's what calculates the skippedCount:
Let me read the visual-truncate.ts file which calculates the skippedCount:
Now I understand the issue. Looking at the code in tool-execution.ts:
const textContent = `\n${styledOutput}`;The text content starts with \n. This newline will be rendered as one visual line. So when we have 5 lines of actual output, we end up with:
\nThat's 6 visual lines total. When maxVisualLines = 5 (BASH_PREVIEW_LINES), we skip 1 visual line (the empty leading newline), but the message says "1 earlier lines" when really we're not hiding any meaningful content.
The issue is that the textContent includes a leading newline for spacing purposes, but that newline gets counted in the truncation calculation.
Let me verify this by understanding the flow:
styledOutput applies styling to each linetextContent = \\n${styledOutput}`` adds a leading newline, making it "\nline1\nline2\nline3\nline4\nline5"truncateToVisualLines renders this and gets 6 visual lines (empty + 5 content lines)skippedCount = 1The fix should be one of:
\n in the truncation calculationLooking at the code more carefully:
const textContent = `\n${styledOutput}`;// ...if (cachedSkipped && cachedSkipped > 0) { const hint = theme.fg("muted", `... (${cachedSkipped} earlier lines,`) + ` ${keyHint("expandTools", "to expand")})`; return ["", hint, ...cachedLines];}return cachedLines;When showing the hint, it returns ["", hint, ...cachedLines]. The empty string "" adds a blank line before the hint. The most straightforward fix would be to not include the leading \n in the truncation calculation, or adjust how the skipped count is reported to exclude the spacer line.
Let me trace through the actual flow: if the original output is "line1\nline2\nline3\nline4\nline5", adding the leading newline creates "\nline1\nline2\nline3\nline4\nline5". After rendering, this becomes ["", "line1", "line2", "line3", "line4", "line5"] — six elements total. If the limit is five lines, then skippedCount = 1, and the truncated array keeps the last five items. The issue is that the leading empty string inflates the line count, causing an off-by-one error in how many lines actually get skipped. we get 6 visual lines total: an empty line from the leading newline, then the 5 content lines. With maxVisualLines = 5, the slice takes the last 5 elements, which are indices 1-5 (skipping the empty leading line). This gives skippedCount = 1, explaining why the UI shows "1 earlier lines" even though there's no actual hidden content—it's just counting that empty line.
The issue is that the leading newline shouldn't factor into the truncation logic since it's purely for spacing. The fix involves excluding it from the calculation so the hint only appears when there's genuinely hidden content.
The proposed solution removes the leading \n from textContent before truncation, then ensures the blank line is added consistently to the output format—either as part of the hint display when content is skipped, or separately when returning the full output.
Looking at the return statements reveals an inconsistency: truncated output includes a blank line via ["", hint, ...], but untruncated output relies on the leading \n already embedded in styledOutput. When examining the flow more closely, the initial \n creates an extra line that gets counted during truncation, causing the function to return only the actual content lines without that leading blank. The display logic then re-adds the blank line before the hint, which is intentional for spacing purposes.
The real issue is that skippedCount=1 represents the formatting blank rather than actual content being omitted. A cleaner approach would be to exclude the leading newline from the truncation logic entirely and handle spacing separately in the output formatting.
When expanded, I'll display all lines with the styling applied. For the collapsed state, I need to cache the truncation results since they depend on the available width. The render function checks if the cache is still valid, recalculating only when the width changes. If there are skipped lines, I show a hint with the count and expansion prompt. Otherwise, I add a blank line for consistent spacing regardless of whether truncation occurred.
The implementation also needs to handle the leading newline properly—both expanded and collapsed cases currently include \n${styledOutput}, but the collapsed path should pass just styledOutput to the truncation function to avoid the extra newline affecting the line count.
Looking at the broader context, this issue appears specific to bash rendering where visual truncation applies. The fix involves removing the leading newline from textContent before truncation and ensuring the blank line for spacing is consistently added in the return statements.
When truncation occurs, the code correctly adds back the blank line with ["", hint, ...cachedLines]. However, when there's no truncation, it returns cachedLines directly, which already contains the leading blank from the original \n${styledOutput}. The problem emerges when the content has exactly BASH_PREVIEW_LINES of actual content—the truncation logic cuts off that leading blank, but the non-truncated path doesn't account for this inconsistency.
The solution is to remove the leading newline from the initial string construction and instead add the blank line consistently in both code paths. Remove the \n prefix from styledOutput on line ~302, then modify line ~320 to prepend the blank line with ["", ...cachedLines] when returning non-truncated content. This ensures the line count calculation stays accurate—5 lines of content won't trigger false truncation—while maintaining proper visual spacing in both cases.
Now I understand the bug. Let me analyze:
The bug is in tool-execution.ts in the renderBashContent() method (lines ~300-325).
// Line ~302const textContent = `\n${styledOutput}`;The code prepends a newline (\n) to the output for spacing purposes. This newline gets counted as a visual line during truncation.
Example with 5-line output:
output = "line1\nline2\nline3\nline4\nline5" (5 lines)textContent = "\nline1\nline2\nline3\nline4\nline5" (leading \n added)["", "line1", "line2", "line3", "line4", "line5"] = 6 visual linesBASH_PREVIEW_LINES = 5skippedCount = 6 - 5 = 1When no truncation occurs (line ~320):
return cachedLines;But when truncation occurs (line ~314-316):
return ["", hint, ...cachedLines];The truncated path adds a blank line ("") before the hint, but the non-truncated path doesn't add this spacing. This causes inconsistent visual spacing between collapsed and expanded states.
File: packages/coding-agent/src/modes/interactive/components/tool-execution.ts
Change 1 (line ~302): Remove the leading \n from textContent:
// Before:const textContent = `\n${styledOutput}`;
// After:const textContent = styledOutput;Change 2 (line ~320): Add blank line for consistent spacing:
// Before:return cachedLines;
// After:return ["", ...cachedLines];This ensures:
terrible analysis
The user is saying my analysis is wrong. Let me re-read the code more carefully and trace through what actually happens.
Let me look at the issue again:
Let me re-read the truncateToVisualLines function and understand what it actually does with line wrapping.
Actually, wait. The Text component's render() method handles line wrapping. So a single logical line could become multiple visual lines if it wraps. But that's not the issue here based on the user's description.
Let me trace through again more carefully. The issue says "1 earlier lines" appears when there are only 5 lines, and after expanding, it shows the same 5 lines. So there's no actual hidden content.
Looking at the code:
const result = truncateToVisualLines(textContent, BASH_PREVIEW_LINES, width);cachedLines = result.visualLines;cachedSkipped = result.skippedCount;BASH_PREVIEW_LINES = 5
If textContent has 5 lines of actual output plus a leading \n, that's 6 visual lines. truncateToVisualLines would return:
Then:
if (cachedSkipped && cachedSkipped > 0) { const hint = theme.fg("muted", `... (${cachedSkipped} earlier lines,`) + ... return ["", hint, ...cachedLines];}So it shows "1 earlier lines" when only the spacing newline was skipped.
Wait, my analysis was correct. The leading \n is being counted. Why did the user say it's terrible?
Let me reconsider how the Text component renders content. When given "\nline1\nline2...", does that leading \n actually produce a visible empty line? Maybe I'm misunderstanding the rendering behavior or there's a different code path I'm not seeing. The user's consistent observation that the count is off by exactly one suggests the leading newline is indeed being counted as a skipped line when it shouldn't be.
If removing that leading \n fixes it, then 5 lines of content would display as 5 visual lines without truncation, and 6 lines would correctly show "1 earlier lines" instead of "2 earlier lines". But since the user said my analysis was flawed, I should check if the problem exists elsewhere. Looking through the codebase, "earlier lines" only appears once at line 313 in the bash rendering section, so unless this message appears in a different tool's output, that's where the bug must be. The user's example clearly shows bash output since it displays the "earlier lines" message. Looking at the actual output in their issue, there are 5 visible lines (line1 through line5), yet the hint indicates "1 earlier lines" - which shouldn't happen if only 5 lines exist total.
The discrepancy likely stems from how visual lines are counted. If the output actually contains 6 visual lines (possibly due to a leading newline), and the truncation logic limits display to 5 lines, then skippedCount would calculate as 6 - 5 = 1, matching what the user sees. This suggests the count calculation itself may be working correctly, but there's an extra visual line being introduced somewhere in the output processing. ", "line4", "line5"] with skippedCount = 1. The message displays "1 earlier lines" but we're only skipping the empty formatting line, not actual content.
Let me reconsider whether I'm analyzing the right part of the code. Maybe the issue lies elsewhere in the expand logic or how "earlier" gets calculated. Actually, looking back at my proposed solution—removing the leading \n and prepending an empty string to cachedLines—I'm creating a blank line. But cachedLines from truncateToVisualLines already handles the spacing, so I might be double-adding that empty line.
Let me trace through both cases. Without truncation, if textContent has 5 lines with no leading newline, allVisualLines becomes ["line1", "line2", "line3", "line4", "line5"]. Since 5 ≤ 5, there's no truncation, skippedCount stays 0, and cachedLines equals the full array. Returning ["", ...cachedLines] adds that spacing prefix. For the truncated case with 6+ lines, I need to verify whether prepending the empty string is actually necessary or if it's creating redundant spacing.
Looking at the code flow around lines 287-289, I should check what's actually happening with the hint and whether my understanding of the truncation logic is correct.
The styledOutput gets a leading newline added (\n${styledOutput}), and this happens in both the expanded and non-expanded cases. When expanded, it directly creates a Text element with this format. In the truncated case, the hint is prepended to the cached lines. Both paths use the same newline prefix, so the behavior should be consistent between them.
But there's a deeper issue: when returning the result with ["", hint, ...cachedLines], an empty string is prepended again. This means after truncation already removed the leading empty line from the count, we're reintroducing it. The sequence becomes the newline, then the hint, then the content lines—creating an extra blank line that throws off the visual line count.
The core problem is that the leading newline gets counted during truncation but then gets added back in the return statement. Either the newline shouldn't be included in the initial textContent, or it shouldn't be reported as a skipped line if it's going to be restored anyway.
Removing the leading newline from textContent would mean allVisualLines contains only the actual content lines without the spacing artifact, so no truncation occurs and the spacing line can be cleanly added back to the output. But I should verify this logic more carefully by examining how truncateToVisualLines actually handles the text parameter and whether my understanding of the line counting is accurate. how textContent is constructed. Re-reading the original issue, the user noted that the count is consistently off by one across messages with more than 5 lines—showing "3 earlier lines" when there should only be 2. If there are 7 actual lines, with a leading newline that creates 8 visual lines total, truncating to 5 gives a skippedCount of 3, which matches the incorrect behavior. The leading newline is being counted as an extra visual line, causing the discrepancy.
My proposed fix removes that leading newline from textContent and adds back an empty string to the array for non-truncated cases. For truncated cases, the code already includes ["", hint, ...cachedLines], so that's handled. But I need to verify the non-truncated path—when textContent is "\nline1\nline2", split produces only 3 elements instead of the expected 4 visual lines.
Let me recount: "\nline1\nline2" splits into ["", "line1", "line2"]—that's 3 logical lines. For 5 content lines, textContent becomes "\nline1\nline2\nline3\nline4\nline5", which splits into 6 elements. Since maxVisualLines is 5 and we have 6, truncation triggers with skippedCount = 1. The leading newline is indeed creating that extra line in the count.
I'm not sure what aspect they found problematic. Maybe I should focus on a cleaner solution instead—the real problem is that line 302 adds a leading newline for spacing, but that gets counted during truncation. The fix is straightforward: exclude that newline from the line count calculation.
Let me re-read the code more carefully.
>rg -n "earlier lines" /Users/badlogic/workspaces/pi-mono --type ts -B5 -A5Let me look at the surrounding context more carefully, especially line 370 onwards.
Let me trace through more carefully.
Line 370: const textContent = \\n${styledOutput}`;`
The textContent includes a leading newline. This is passed to truncateToVisualLines.
When there are 5 lines of actual output:
When Text.render() processes this, what do we get?
Actually, I need to look at how Text.render() works. Let me check the TUI package.
Actually, the issue might be simpler. Let me think about what happens:
The leading \n means we have:
That's 6 visual lines. BASH_PREVIEW_LINES = 5.
So we skip 1 line (the empty one) and show the last 5 (all the content).
The message says "1 earlier lines" but we're not actually hiding content, just the spacing.
When cachedSkipped > 0, we return ["", hint, ...cachedLines].
So the output is:
When NOT truncated, we return cachedLines directly. But cachedLines would include the leading empty line from the \n.
So the inconsistency is:
Truncated (6+ visual lines): shows blank + hint + last 5 content lines
Not truncated (<=5 visual lines): shows whatever cachedLines is
For 4 content lines: textContent = "\nline1\nline2\nline3\nline4", allVisualLines = ["", "line1", "line2", "line3", "line4"] = 5 lines total, which doesn't exceed the limit, so no truncation happens and we return the full cachedLines without any hint. For 5 content lines: textContent = "\nline1\nline2\nline3\nline4\nline5", allVisualLines has 6 elements, exceeding the threshold.
When truncation occurs, cachedLines becomes the last 5 lines ["line1", "line2", "line3", "line4", "line5"] with skippedCount = 1, and the hint displays "1 earlier lines" even though only the leading newline was removed. The root cause is that the leading \n shouldn't count toward truncation. The user's report confirms this—they're seeing the count consistently off by one across multiple messages, where "3 earlier lines" actually means 2 lines were hidden.
Let me trace through more carefully with a concrete example:
With 5 lines of output:
output = "line1\nline2\nline3\nline4\nline5"textContent = "\nline1\nline2\nline3\nline4\nline5" (line 370 adds leading \n)truncateToVisualLines renders this and gets 6 visual lines: ["", "line1", "line2", "line3", "line4", "line5"]BASH_PREVIEW_LINES = 5cachedLines = ["line1", "line2", "line3", "line4", "line5"] (last 5)skippedCount = 1["", hint, ...cachedLines] showing "1 earlier lines" messageThe bug: The leading \n on line 370 is a formatting spacer, not content. It gets counted as a "hidden line", causing the off-by-one error.
With 7 lines: 8 visual lines (1 empty + 7 content), skips 3, reports "3 earlier lines" when only 2 actual content lines are hidden.
File: packages/coding-agent/src/modes/interactive/components/tool-execution.ts
Line 370: Change:
const textContent = `\n${styledOutput}`;to:
const textContent = styledOutput;Line 390: Change:
return cachedLines;to:
return ["", ...cachedLines];This removes the spacer from the truncation calculation while preserving the visual spacing in both truncated and non-truncated cases.
fix it
The user wants me to implement the fix. Let me make the changes to the file.
Now I need to change line 390 to add the blank line for spacing in the non-truncated case.
Now let me run the check command to make sure everything is good.
>cd /Users/badlogic/workspaces/pi-mono && npm run check 2>&1All checks passed. Now I should update the changelog.
Now let me update the changelog:
I see there's an ### Added section and ### Changed section, but no ### Fixed section yet under Unreleased. I need to add a Fixed section with the bug fix.
Let me run check again to make sure nothing broke.
>cd /Users/badlogic/workspaces/pi-mono && npm run check 2>&1+371 // Note: Don't include leading \n in truncation - it's just spacing, not content +372 const textContent = styledOutput;
why this assignment? and comment? do we still need this?
The user is right. Looking at the code, textContent is only used in one place - passed to truncateToVisualLines. Since we're just assigning styledOutput to textContent without modification, we can just use styledOutput directly and remove the unnecessary variable and comment.
You're right, it's pointless. Just use styledOutput directly:
Let me run check again to make sure it's still good.
>cd /Users/badlogic/workspaces/pi-mono && npm run check 2>&1Checks still pass.
Done.
commit and push
The user wants me to commit and push the changes. I need to be careful to only add the specific files I modified, not use git add -A or git add ..
Files I modified:
>cd /Users/badlogic/workspaces/pi-mono && git add packages/coding-agent/src/modes/interactive/components/tool-execution.ts packages/coding-agent/CHANGELOG.md && git commit -m "fix(coding-agent): off-by-one error in bash output 'earlier lines' count
fixes #921"Commit successful. Now let me push.
>cd /Users/badlogic/workspaces/pi-mono && git pushPushed successfully.
Done. Committed and pushed 3235926e.
do a cat lines.txt
The user wants me to run cat lines.txt to test something. Let me do that.
>cat lines.txtDone - showing the output of lines.txt which has 100 lines.
100 lines, line1 through line100.
const textContent = `\n${styledOutput}`;const textContent = `\n${styledOutput}`;// ...if (cachedSkipped && cachedSkipped > 0) { const hint = theme.fg("muted", `... (${cachedSkipped} earlier lines,`) + ` ${keyHint("expandTools", "to expand")})`; return ["", hint, ...cachedLines];}return cachedLines;// Line ~302const textContent = `\n${styledOutput}`;return cachedLines;return ["", hint, ...cachedLines];// Before:const textContent = `\n${styledOutput}`;
// After:const textContent = styledOutput;// Before:return cachedLines;
// After:return ["", ...cachedLines];const result = truncateToVisualLines(textContent, BASH_PREVIEW_LINES, width);cachedLines = result.visualLines;cachedSkipped = result.skippedCount;if (cachedSkipped && cachedSkipped > 0) { const hint = theme.fg("muted", `... (${cachedSkipped} earlier lines,`) + ... return ["", hint, ...cachedLines];}const textContent = `\n${styledOutput}`;const textContent = styledOutput;return cachedLines;return ["", ...cachedLines];