fix(repl): fix multi-line history display corruption when editing#27561
fix(repl): fix multi-line history display corruption when editing#27561
Conversation
…alled code
When multi-line code was recalled from history using the up arrow key,
each keystroke (backspace, etc.) caused the entire multi-line content to
be reprinted, creating duplicated output. This happened because
refreshLine() only cleared the current terminal line but the buffer
contained embedded newlines spanning multiple lines.
Fix refreshLine() to properly handle multi-line content by:
- Tracking the number of extra terminal lines from the previous render
- Moving cursor up and clearing those lines before redrawing
- Rendering continuation prompts ("... ") for subsequent lines
- Properly positioning the cursor within multi-line content
Also fix history file persistence: multi-line entries containing newlines
were corrupted when saved (fragmented into separate entries) because the
file format used newline as both the entry delimiter and internal line
separator. Now uses ASCII Record Separator (0x1E) as the entry delimiter
when multi-line entries are present, with backward compatibility for
legacy newline-delimited files.
Closes #27560
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 11:36 AM PT - Feb 28th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 27561That installs a local version of the PR into your bun-27561 --bun |
WalkthroughThis pull request enhances the REPL's multi-line editing capabilities by introducing a RECORD_SEP delimiter for history persistence, refactoring the rendering system to properly track and display multi-line outputs, and implementing cursor management logic to maintain consistent display state during editing operations. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/repl/repl.test.ts`:
- Around line 1055-1079: Remove the artificial delay: in the test "recalling
multi-line history and editing works correctly" replace the Bun.sleep(100) after
send("\x15") with an awaited condition instead of a fixed timeout; delete the
Bun.sleep(100) call and use await waitFor(...) (for example await
waitFor(/\u276f|> /) or another appropriate waitFor that confirms the
prompt/line was cleared) so input is processed sequentially before calling
send("222 + 333\n"); locate this change around the withTerminalRepl block where
send, waitFor and Bun.sleep are used.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/repl.zigtest/js/bun/repl/repl.test.ts
|
|
||
| test("recalling multi-line history and editing works correctly", async () => { | ||
| // Regression test for https://github.com/oven-sh/bun/issues/27560 | ||
| // When recalling multi-line history with up arrow and then pressing backspace, | ||
| // the remaining code was reprinted in its entirety after each deletion because | ||
| // refreshLine() didn't clear previous multi-line rendering. | ||
| await withTerminalRepl(async ({ send, waitFor }) => { | ||
| // Enter multi-line code | ||
| send("if (true) {\n"); | ||
| await waitFor("..."); | ||
| send("111\n"); | ||
| send("}\n"); | ||
| await waitFor(/\u276f|> /); | ||
|
|
||
| // Press up arrow to recall the multi-line history entry | ||
| send("\x1b[A"); | ||
| await waitFor("if (true)"); | ||
|
|
||
| // Delete all content with Ctrl+U (delete to start of line) and type new code | ||
| send("\x15"); // Ctrl+U - clear line | ||
| await Bun.sleep(100); | ||
| send("222 + 333\n"); | ||
| await waitFor("555"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good regression test for issue #27560.
The test correctly exercises the multi-line history recall and editing scenario. Consider removing the Bun.sleep(100) at line 1075 - terminal input should be processed sequentially, and the final waitFor("555") verifies the expected outcome. As per coding guidelines, tests should await conditions rather than fixed time delays.
// Delete all content with Ctrl+U (delete to start of line) and type new code
send("\x15"); // Ctrl+U - clear line
- await Bun.sleep(100);
send("222 + 333\n");
await waitFor("555");📝 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.
| test("recalling multi-line history and editing works correctly", async () => { | |
| // Regression test for https://github.com/oven-sh/bun/issues/27560 | |
| // When recalling multi-line history with up arrow and then pressing backspace, | |
| // the remaining code was reprinted in its entirety after each deletion because | |
| // refreshLine() didn't clear previous multi-line rendering. | |
| await withTerminalRepl(async ({ send, waitFor }) => { | |
| // Enter multi-line code | |
| send("if (true) {\n"); | |
| await waitFor("..."); | |
| send("111\n"); | |
| send("}\n"); | |
| await waitFor(/\u276f|> /); | |
| // Press up arrow to recall the multi-line history entry | |
| send("\x1b[A"); | |
| await waitFor("if (true)"); | |
| // Delete all content with Ctrl+U (delete to start of line) and type new code | |
| send("\x15"); // Ctrl+U - clear line | |
| await Bun.sleep(100); | |
| send("222 + 333\n"); | |
| await waitFor("555"); | |
| }); | |
| }); | |
| test("recalling multi-line history and editing works correctly", async () => { | |
| // Regression test for https://github.com/oven-sh/bun/issues/27560 | |
| // When recalling multi-line history with up arrow and then pressing backspace, | |
| // the remaining code was reprinted in its entirety after each deletion because | |
| // refreshLine() didn't clear previous multi-line rendering. | |
| await withTerminalRepl(async ({ send, waitFor }) => { | |
| // Enter multi-line code | |
| send("if (true) {\n"); | |
| await waitFor("..."); | |
| send("111\n"); | |
| send("}\n"); | |
| await waitFor(/\u276f|> /); | |
| // Press up arrow to recall the multi-line history entry | |
| send("\x1b[A"); | |
| await waitFor("if (true)"); | |
| // Delete all content with Ctrl+U (delete to start of line) and type new code | |
| send("\x15"); // Ctrl+U - clear line | |
| send("222 + 333\n"); | |
| await waitFor("555"); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/repl/repl.test.ts` around lines 1055 - 1079, Remove the
artificial delay: in the test "recalling multi-line history and editing works
correctly" replace the Bun.sleep(100) after send("\x15") with an awaited
condition instead of a fixed timeout; delete the Bun.sleep(100) call and use
await waitFor(...) (for example await waitFor(/\u276f|> /) or another
appropriate waitFor that confirms the prompt/line was cleared) so input is
processed sequentially before calling send("222 + 333\n"); locate this change
around the withTerminalRepl block where send, waitFor and Bun.sleep are used.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (self.prev_extra_lines > 0) { | ||
| var i: usize = 0; | ||
| while (i < self.prev_extra_lines) : (i += 1) { | ||
| // Move cursor up one line and clear it | ||
| self.write(CSI ++ "1A"); | ||
| self.write("\r"); | ||
| self.write(Cursor.clear_line); | ||
| } |
There was a problem hiding this comment.
🔴 The clearing loop in refreshLine() (lines 999-1006) moves up prev_extra_lines times from the current terminal cursor position, but after multi-line rendering the terminal cursor is on cursor_line (lines 1079-1085), not necessarily the last line. This causes two problems: (1) if cursor_line < prev_extra_lines, the loop moves above line 0 and clears content above the REPL prompt, while lines below cursor_line are never cleared; (2) the loop moves up then clears, so the starting line is never cleared—when transitioning from multi-line to single-line (e.g., Ctrl+U), the bottom line(s) of the old rendering remain as stale visible content. Fix: track the previous cursor_line so clearing knows its actual starting position, or move to the last rendered line first before clearing upward.
Extended reasoning...
Bug Analysis
The refreshLine() function introduced in this PR has a clearing loop at lines 999-1006 that is responsible for erasing previously rendered multi-line content before redrawing. The loop moves the terminal cursor up prev_extra_lines times, clearing each line it arrives at. However, it incorrectly assumes the terminal cursor starts on the last rendered line.
Root Cause: Cursor Position Mismatch
After the multi-line rendering path writes all lines, the code at lines 1079-1085 repositions the terminal cursor to the line containing the editing cursor:
const lines_to_move_up = total_lines - 1 - cursor_line;
if (lines_to_move_up > 0) {
// moves cursor UP from last line to cursor_line
}This means after rendering, the terminal cursor is on visual line cursor_line, and prev_extra_lines is set to extra_lines. On the next refreshLine() call, the clearing loop moves up prev_extra_lines times from cursor_line, not from the last line. When cursor_line < prev_extra_lines, this overshoots above line 0.
Step-by-Step Proof (Bug 1: Clearing above REPL content)
- User recalls multi-line history
"a\nb\nc"(3 visual lines,extra_lines=2). Cursor at end →cursor_line=2, terminal cursor on line 2,prev_extra_lines=2. - User presses Ctrl+A (move to start).
refreshLine()runs: clearing loop starts at line 2, moves up 2 → arrives at line 0 (correct). Renders content, positions cursor on line 0 (cursor_line=0,lines_to_move_up=2). Setsprev_extra_lines=2. - User types any character.
refreshLine()runs again: terminal cursor is on line 0, clearing loop moves upprev_extra_lines=2times → goes to line -2, clearing two lines of content above the REPL prompt. Lines 1 and 2 are never cleared, leaving stale content.
Step-by-Step Proof (Bug 2: Stale bottom lines)
The clearing loop moves up first, then clears (CSI 1A then clear_line). This means it clears the lines it arrives at, but never the line it starts from.
- Content
"a\nb\nc"rendered with cursor at end → terminal on line 2,prev_extra_lines=2. - User presses Ctrl+U (clear buffer).
refreshLine()runs: loop iteration i=0 moves from line 2 to line 1 (clears 1), i=1 moves from line 1 to line 0 (clears 0). Post-loop clears line 0 again. Line 2 is never cleared. - New content is empty (single-line path), written only on line 0. The old
"c"text on line 2 remains visible as stale terminal output.
This second bug occurs even when the cursor IS on the last line, making it distinct from the first bug.
Addressing the Duplicate Objection
One verifier argued bug_005 is a duplicate of bug_001. While both bugs live in the same clearing loop (lines 999-1006), they have different root causes and different trigger conditions. Bug 001 requires cursor_line < prev_extra_lines (cursor not on last line). Bug 005 happens because the loop structurally skips the starting line—it fires even when the cursor IS on the last line (the common case of cursor at end of content). A fix for bug 001 alone (tracking prev_cursor_line) would not fix bug 005 unless the loop structure is also changed to clear the starting line.
Impact
Both bugs cause visible terminal display corruption during normal REPL usage: recalling multi-line history entries and navigating with cursor keys (Ctrl+A, arrow keys) or editing (Ctrl+U, backspace). The corruption manifests as cleared lines above the prompt and/or stale text below the current content.
Suggested Fix
Track a prev_cursor_line field alongside prev_extra_lines. At the start of clearing: first move down by (prev_extra_lines - prev_cursor_line) to reach the last rendered line, then move up prev_extra_lines times, clearing each line including the starting line. This ensures all previously rendered lines are cleared regardless of where the cursor was positioned.
|
|
||
| // Delete all content with Ctrl+U (delete to start of line) and type new code | ||
| send("\x15"); // Ctrl+U - clear line | ||
| await Bun.sleep(100); |
There was a problem hiding this comment.
🟡 The new test at line 1075 uses await Bun.sleep(100) instead of a condition-based wait, which violates the CLAUDE.md guideline: "never wait for time to pass in tests. Always wait for the condition to be met." After sending Ctrl+U, consider using await waitFor(/\u276f|> /) to wait for the prompt to redraw after the multi-line content is cleared, rather than relying on a fixed delay.
Extended reasoning...
What the bug is
The newly added test "recalling multi-line history and editing works correctly" uses await Bun.sleep(100) at line 1075 between sending Ctrl+U (clear line) and sending the next input (222 + 333\n). Both CLAUDE.md (line 102) and test/CLAUDE.md (line 21) explicitly prohibit time-based waits in tests.
The specific code path
The test flow is:
- Enter multi-line code (
if (true) { 111 }) and wait for prompt - Press up arrow to recall the multi-line history entry, wait for
"if (true)" - Send Ctrl+U (
\x15) to clear the line await Bun.sleep(100)— fixed delay instead of condition-based wait- Send
222 + 333\nand wait for"555"
After Ctrl+U clears the multi-line recalled content, refreshLine() in the Zig code redraws the prompt with an empty line. The test could wait for observable state change rather than sleeping.
Why existing code doesn't prevent it
There is a pre-existing instance of the same pattern at line 993 (await Bun.sleep(100) in the "up arrow recalls previous command" test), which may have served as a template for this new code. However, the existence of a pre-existing violation doesn't justify adding new ones.
Addressing the refutation
One verifier argued this should not be flagged because: (a) tests are non-production code, (b) the pre-existing pattern exists, and (c) after Ctrl+U there may be no clear observable state to wait for. These are reasonable concerns. However, after Ctrl+U clears a multi-line entry, the terminal redraws with just the prompt and an empty line — the disappearance of the multi-line content (e.g., "if (true)" no longer being present in the recent output) or the prompt redrawing could serve as a waitFor condition. That said, this is a pragmatic concern in a PTY test where timing interactions with terminal redraws can be subtle.
Impact
The Bun.sleep(100) introduces a potential source of test flakiness. On slow CI machines or under heavy load, 100ms may not be enough for the terminal to finish processing; on fast machines, it adds unnecessary delay. Condition-based waits are more robust and faster on average.
How to fix
Replace await Bun.sleep(100) with a condition-based wait. One approach: after sending Ctrl+U, wait for the multi-line content to disappear from the terminal output, or wait for the prompt to redraw. For example, you could waitFor the prompt pattern /\u276f|> / which would indicate the terminal has redrawn after clearing the line. The same fix could also be applied to the pre-existing instance at line 993.
| terminal_width: u16 = 80, | ||
| terminal_height: u16 = 24, | ||
| ctrl_c_pressed: bool = false, | ||
| prev_extra_lines: usize = 0, // number of extra terminal lines from previous refreshLine render |
There was a problem hiding this comment.
🟡 handleTab() prints completion results (newlines + items) directly to the terminal without first calling movePastMultilineContent(). When prev_extra_lines > 0 (multi-line history recalled), the printed text shifts the cursor down, and the subsequent refreshLine() moves up the wrong number of lines, corrupting the display. Both handleEnter() (line 1927) and handleCtrlC() (line 2018) were updated to call movePastMultilineContent(), but handleTab() was missed — add the same call before the self.print("\n", ...) statements at lines 2062, 2139, and 2157.
Extended reasoning...
Bug Analysis
This PR introduces prev_extra_lines tracking and movePastMultilineContent() to handle multi-line history entries that now contain embedded newlines in the line editor buffer. When the terminal has rendered multi-line content, prev_extra_lines > 0 and any code that prints output before calling refreshLine() must first call movePastMultilineContent() to move the cursor past the rendered multi-line content. Otherwise, the printed text shifts the cursor position, and refreshLine() incorrectly uses the stale prev_extra_lines to move up from the wrong position.
The Missing Call
handleEnter() (line 1927) and handleCtrlC() (line 2018) were both correctly updated to call self.movePastMultilineContent() before printing. However, handleTab() has three code paths that print to the terminal without this call:
- Line 2062: REPL command completion with multiple matches — prints
\nfollowed by each matching command - Line 2139: JS property completion with multiple matches (≤50) — prints
\nfollowed by each completion - Line 2157: Too many completions (>50) — prints
\nfollowed by count message
Step-by-Step Proof
- User enters multi-line code:
if (true) {\n console.lo\n}which gets saved to history. - User recalls this entry with up arrow.
refreshLine()renders it across 3 terminal lines.prev_extra_lines = 2. The terminal cursor is positioned on the line containing the editing cursor (e.g., line 1 forconsole.lo). - User presses Tab to complete
lo→ multiple matches (e.g.,log,toLowerCase). handleTab()at line 2139 prints\nand completion items starting from the current terminal cursor position (somewhere in the middle of the 3-line rendering).refreshLine()is then called. It seesprev_extra_lines = 2and tries to move cursor up 2 lines from the current position. But the cursor has been shifted down by the printed completion text. Moving up 2 lines now clears completion text instead of the original multi-line content.refreshLine()writes the new rendering at the wrong position while old multi-line content remains visible above — display corruption.
Why This Is New
Before this PR, the line_editor buffer never contained \n characters — multi-line input was accumulated in multiline_buffer separately. So prev_extra_lines was always 0 when Tab was pressed, and this code path was safe. Now that recalled multi-line history entries contain embedded newlines, prev_extra_lines can be non-zero during Tab completion.
Fix
Add self.movePastMultilineContent() before the self.print("\n", ...) calls in the three affected branches of handleTab(), matching the pattern used by handleEnter() and handleCtrlC(). This is a cosmetic display corruption bug that is recoverable with Ctrl+L, and requires the specific scenario of Tab-completing while editing recalled multi-line history.
| // Move to end of the last line | ||
| self.write("\r"); | ||
| self.write(Cursor.clear_to_end); |
There was a problem hiding this comment.
🟡 movePastMultilineContent() at lines 1919-1921 does \r (move to column 0) followed by Cursor.clear_to_end (\x1b[0K), which erases the entire last displayed line of multi-line content from the terminal instead of positioning the cursor at its end. When pressing Enter on recalled multi-line history like if (true) {\n 111\n}, the last line (... }) disappears from scrollback. The fix should move the cursor to the end of the last line content (e.g., using cursor-right positioning) rather than clearing it.
Extended reasoning...
Bug Analysis
The movePastMultilineContent() function is meant to move the cursor past all rendered lines of multi-line content so that subsequent output (newline, evaluation result) appears below it. After correctly moving the cursor down to the last rendered line, it executes:
self.write("\r");
self.write(Cursor.clear_to_end);The comment says "Move to end of the last line" but the code does the opposite: \r is a carriage return that moves the cursor to column 0, and Cursor.clear_to_end is \x1b[0K which erases from the cursor position to the end of the line. Starting at column 0, this erases the entire visible content of the line.
Step-by-step proof
- User enters multi-line code
if (true) {\n 111\n}which gets saved to history. - User presses up arrow to recall it.
refreshLine()renders three terminal lines:❯ if (true) { ... 111 ... } set()places the cursor at the end of the buffer, socursor_lineequalsprev_extra_lines(both are 2).- User presses Enter.
handleEnter()callsmovePastMultilineContent(). lines_below = prev_extra_lines - cursor_line = 2 - 2 = 0, so the move-down is skipped (cursor is already on the last line).\rmoves the cursor to column 0 of the line containing... }.clear_to_enderases from column 0 to end of line, wiping... }entirely.handleEnterthen prints\nand the evaluation result111.- The scrollback now shows a blank line where
... }was:❯ if (true) { ... 111 <- was "... }", now blank 111 ❯ _
The same issue occurs when cursor is on an earlier line: the code correctly moves down, but then still clears the last line.
Impact
This is a cosmetic issue affecting terminal scrollback only. The code evaluates correctly regardless. It manifests whenever the user presses Enter or Ctrl+C (both call movePastMultilineContent()) on recalled multi-line history entries. The erased line is visible if the user scrolls up to review previous input.
Fix
The \r + clear_to_end sequence should be replaced with cursor positioning that moves to the end of the last line content without erasing it. For example, after moving down to the last line, compute the visual width of the last segment (including its continuation prompt) and use \r followed by CSI {width}C to position the cursor at the end. Alternatively, simply remove the clear_to_end call since refreshLine() already properly rendered each line with clear_to_end after the segment content, so there is no leftover content that needs clearing.
1 participant
Summary
refreshLine()in the REPL to properly handle multi-line content recalled from history by tracking rendered lines, clearing them before redraw, and correctly positioning the cursorCloses #27560
What was the bug?
When multi-line code (e.g.,
if (1) { ... }) was entered in the REPL and then recalled via the up arrow key, each keystroke (backspace, cursor movement, etc.) caused the entire multi-line content to be reprinted on screen, creating duplicated output that grew with each keypress.Root cause:
refreshLine()only cleared one terminal line (\x1b[2K) before redrawing, but the buffer from multi-line history entries contained embedded\ncharacters that spanned multiple terminal lines. Each redraw would output new lines below without clearing the previous rendering.Secondary issue: The history file used
\nas both the entry delimiter and internal line separator, so multi-line entries were fragmented into separate entries when saved and reloaded.Changes
src/repl.zigrefreshLine(): Added multi-line rendering path that moves cursor up to clear previously rendered extra lines, writes continuation prompts (...) for each line, and properly positions the cursor within multi-line contentmovePastMultilineContent(): New helper that moves the cursor past multi-line content before Enter/Ctrl+C so output appears below itHistory.save()/History.load(): Uses ASCII Record Separator (0x1E) as entry delimiter when multi-line entries exist, with backward compatibility for legacy newline-delimited filesprev_extra_lines: New state field tracking extra terminal lines from the previousrefreshLinerendertest/js/bun/repl/repl.test.tsTest plan
🤖 Generated with Claude Code