fix prompt input truncating #24020
fix prompt input truncating #24020
Conversation
ominit
force-pushed
the
ominit/fix-prompt-input-length
branch
from
d8b3014 to
a7654da
Compare
Walkthrough
Changes
Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/bun.js/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
🧠 Learnings (9)📚 Learning: 2025-11-03T20:40:59.655ZApplied to files:
📚 Learning: 2025-10-24T10:43:09.398ZApplied to files:
📚 Learning: 2025-08-30T00:13:36.815ZApplied to files:
📚 Learning: 2025-08-30T00:11:57.076ZApplied to files:
📚 Learning: 2025-08-30T00:11:00.890ZApplied to files:
📚 Learning: 2025-09-05T19:49:26.188ZApplied to files:
📚 Learning: 2025-09-08T00:41:12.052ZApplied to files:
📚 Learning: 2025-09-05T19:49:26.188ZApplied to files:
📚 Learning: 2025-09-02T18:25:27.976ZApplied to files:
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/webcore/prompt.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/webcore/prompt.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/prompt.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/prompt.zig
🔇 Additional comments (2)
src/bun.js/webcore/prompt.zig (2)
327-331: Line redraw math: verify off‑by‑one cases (mid‑line delete/insert).The sequences use len - cursor_index (+1) moves. Edge cases (delete in middle, then immediate insert) can drift. Please add tests for:
- "abc", move left to between b|c, backspace, then type "X"
- Long lines near terminal width (wrap)
Also applies to: 384-387, 399-403
315-317: Possible lifetime hazard: does toJS copy bytes from input.items?You deinit the ArrayList on scope exit. If jsc.ZigString.toJS doesn’t copy, this becomes a use‑after‑free. Please confirm ownership/copy semantics.
Run this repo search to verify ZigString’s toJS behavior:
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/webcore/prompt.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/webcore/prompt.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/prompt.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/prompt.zig
🔇 Additional comments (5)
src/bun.js/webcore/prompt.zig (5)
197-201: Comment updated correctly—no false "history" claim.The comment now accurately describes cursor movement without claiming unimplemented history support. This addresses the previous review concern.
292-301: Raw-mode configuration looks correct.The termios setup now properly clears ISIG and IEXTEN, and sets VMIN=1/VTIME=0 for deterministic non-canonical reads. This addresses previous concerns about incomplete raw-mode setup.
417-420: Ctrl+D (EOT) is now handled correctly.In raw mode, Ctrl+D arrives as byte 4, not as an EOF condition. This explicit handling addresses a previous concern about treating read errors as Ctrl+D.
328-349: UTF-8 codepoint handling is correct for backspace.The use of
std.unicode.utf8Prevensures backspace deletes whole UTF-8 codepoints rather than individual bytes, with proper fallback for invalid UTF-8. This addresses a previous major concern.
422-434: Byte-by-byte insertion works but has minor UTF-8 considerations.The byte-by-byte insertion means multi-byte UTF-8 characters (e.g., emoji) arrive and are echoed as separate bytes across multiple loop iterations. This can temporarily write incomplete UTF-8 sequences to the terminal, though modern terminals handle this gracefully.
The cursor positioning logic (using
utf8Prev/utf8Next) ensurescursor_indexis always at codepoint boundaries, so inserting bytes sequentially builds valid UTF-8 at those boundaries.This is an acceptable approach for raw-mode input, though it does have edge cases (e.g., pressing Enter mid-character) that are unlikely in practice.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/webcore/prompt.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/webcore/prompt.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/prompt.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
Files:
src/bun.js/webcore/prompt.zig
🧠 Learnings (9)
📚 Learning: 2025-11-03T20:40:59.655Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24273
File: src/bun.js/bindings/JSValue.zig:545-586
Timestamp: 2025-11-03T20:40:59.655Z
Learning: In Bun's Zig codebase, JSErrors (returned as `bun.JSError!T`) must always be properly handled. Using `catch continue` or `catch { break; }` to silently suppress JSErrors is a bug. Errors should either be explicitly handled or propagated with `try`. This applies to snapshot serializer error handling where Jest's behavior is to throw when serializers throw.
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun's Zig codebase, `.js_undefined` is a valid way to represent JavaScript's undefined value when working with JSPromise.resolve() and similar JavaScript interop functions. This is the correct pattern to use rather than `jsc.JSValue.jsUndefined()`.
Applied to files:
src/bun.js/webcore/prompt.zig
📚 Learning: 2025-09-02T18:25:27.976Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:284-314
Timestamp: 2025-09-02T18:25:27.976Z
Learning: In bun's custom Zig implementation, the `#` prefix for private fields is valid syntax and should not be flagged as invalid. The syntax `#fieldname` creates private fields that cannot be accessed from outside the defining struct, and usage like `self.#fieldname` is correct within the same struct. This applies to fields like `#parent`, `#state`, `#allocator`, `#trace`, etc. throughout the codebase.
Applied to files:
src/bun.js/webcore/prompt.zig
🔇 Additional comments (3)
src/bun.js/webcore/prompt.zig (3)
318-322: LGTM: Comment accurately describes the two-mode implementation.The updated comment correctly describes the interactive TTY mode with cursor movement and the fallback buffered line reader, without claiming unimplemented features.
403-422: LGTM: Terminal configuration and signal handling are now correct.The code properly:
- Uses a
pending_sigintflag to defer signal propagation until after terminal restoration- Clears ICANON, ECHO, ISIG, and IEXTEN for proper raw mode
- Sets VMIN=1 and VTIME=0 for deterministic byte-by-byte reads
- Restores original terminal state in the defer block before raising any signals
This addresses all previously identified critical issues with terminal handling.
163-203: LGTM: UTF-8 handling is correctly implemented.The helper functions properly:
handleBackspace: Deletes entire UTF-8 codepoints usingutf8Prevutf8Prev: Walks backward past continuation bytes to find codepoint boundariesutf8Next: Usesbun.strings.utf8ByteSequenceLengthfor correct codepoint lengthThis ensures cursor movement, backspace, and delete operations work correctly with multibyte UTF-8 characters.
| const c_termios = @cImport({ | ||
| @cInclude("termios.h"); | ||
| @cInclude("unistd.h"); | ||
| @cInclude("signal.h"); | ||
| }); | ||
|
|
||
| if (c_termios.isatty(bun.FD.stdin().native()) == 1) { | ||
| var original_termios: c_termios.termios = undefined; | ||
| var pending_sigint: bool = false; | ||
| if (c_termios.tcgetattr(bun.FD.stdin().native(), &original_termios) != 0) { | ||
| return .null; | ||
| } | ||
|
|
||
| defer { | ||
| _ = c_termios.tcsetattr(bun.FD.stdin().native(), c_termios.TCSADRAIN, &original_termios); | ||
| // Move cursor to next line after input is done | ||
| _ = bun.Output.writer().writeAll("\n") catch {}; | ||
| bun.Output.flush(); | ||
| if (pending_sigint) { | ||
| _ = c_termios.raise(c_termios.SIGINT); | ||
| } | ||
| } | ||
|
|
||
| var raw_termios = original_termios; | ||
| // Unset canonical mode, echo, signal generation, and extended input processing | ||
| raw_termios.c_lflag &= ~@as(c_termios.tcflag_t, c_termios.ICANON | c_termios.ECHO | c_termios.ISIG | c_termios.IEXTEN); | ||
| // Set VMIN=1 and VTIME=0 for non-canonical read (read returns after 1 byte) | ||
| raw_termios.c_cc[c_termios.VMIN] = 1; | ||
| raw_termios.c_cc[c_termios.VTIME] = 0; | ||
|
|
||
| if (c_termios.tcsetattr(bun.FD.stdin().native(), c_termios.TCSADRAIN, &raw_termios) != 0) { | ||
| return .null; | ||
| } |
There was a problem hiding this comment.
setting raw mode can be replaced with Bun__ttySetMode. see init_command.zig for an example of this
| @cInclude("signal.h"); | ||
| }); | ||
|
|
||
| if (c_termios.isatty(bun.FD.stdin().native()) == 1) { |
There was a problem hiding this comment.
| if (c_termios.isatty(bun.FD.stdin().native()) == 1) { | |
| if (bun.c.isatty(bun.FD.stdin().native()) == 1) { |
also let's invert this check and break out of this block if it's false. this means all the code inside of the if will have one less indent
|
|
||
| var result = jsc.ZigString.init(input.items); | ||
| result.markUTF8(); | ||
| return result.toJS(globalObject); |
There was a problem hiding this comment.
| return result.toJS(globalObject); | |
| return bun.String.createUTF8ForJS(globalObject, input.items); |
|
|
||
| // End of input | ||
| '\n', KEY_ENTER => { | ||
| if (input.items.len == 0 and !has_default) return jsc.ZigString.init("").toJS(globalObject); |
There was a problem hiding this comment.
| if (input.items.len == 0 and !has_default) return jsc.ZigString.init("").toJS(globalObject); | |
| if (input.items.len == 0 and !has_default) return bun.String.empty.toJS(globalObject); |
| if (has_message) { | ||
| const message = try arguments[0].toSlice(globalObject, allocator); | ||
| defer message.deinit(); | ||
| prompt_width += columnWidth(message.slice()); | ||
| } | ||
| prompt_width += columnWidth(if (has_message) " " else "Prompt "); | ||
|
|
||
| if (has_default) { | ||
| const default_string = try arguments[1].toSlice(globalObject, allocator); | ||
| defer default_string.deinit(); | ||
|
|
||
| prompt_width += columnWidth("[") + columnWidth(default_string.slice()) + columnWidth("] "); | ||
| } |
There was a problem hiding this comment.
instead of getting the string arguments again, would it be possible to reuse the results of arguments[0/1].toSlice from above
| const KEY_CTRL_C = 3; | ||
| const KEY_CTRL_D = 4; | ||
| const KEY_BACKSPACE = 8; | ||
| const KEY_TAB = 9; | ||
| const KEY_ENTER = 13; | ||
| const KEY_ESC = 27; | ||
| const KEY_DEL = 127; |
There was a problem hiding this comment.
I think these should be deleted and replaced with std.ascii.control_code
|
|
||
| fn fullRedraw(stdout_writer: anytype, input: *std.ArrayList(u8), cursor_index: usize, prompt_width: usize) !void { | ||
| // 1. Move cursor to the start of the input area using absolute positioning (column prompt_width + 1). | ||
| _ = stdout_writer.print("\x1b[{d}G", .{prompt_width + 1}) catch {}; |
There was a problem hiding this comment.
why catch {}? Looks like it would be okay to return writer errors from this function
dylan-conway
left a comment
dylan-conway
left a comment
There was a problem hiding this comment.
Left a few comments/suggestions. Looks like this is headed in the right direction.
Testing locally I noticed terminal width handling bug. If you reach the end of a line and continue typing it will redraw the entire line plus the new character
2 participants
What does this PR do?
Previously, prompt's input would be truncated by canonical mode in the terminal.
This temporarily turns off canonical mode if it is enabled on unix based systems and manually parses the inputs.
How did you verify your code works?