Repository navigation
[tui] Port - #2
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Go-based terminal user interface (TUI) library under console/shell, featuring modules for environment management, forms, keyboard input, structured outputs (grids, tables, datatables), interactive prompts, and status indicators (spinners, progress bars, streams, tasks). Feedback on the implementation highlights several critical areas for improvement: resolving a state-retention bug in datatable validation, deferring choice rendering until after successful validation, preventing terminal flooding in stream rendering, and adding a redraw mechanism for task logging. Additionally, the review recommends addressing concurrency race conditions in the global environment state, refactoring recursive key handling to loops to prevent stack overflows, and optimizing
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var submitted *dataTableSelection | ||
| return prompt.UntilValid( | ||
| prompt.Options[any]{Message: options.Message, Default: defaultValue, Required: options.Required, Validate: options.Validate}, | ||
| func(_ int) (any, error) { | ||
| selection, err := readDataTableSelection(options) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| value := selection.Value | ||
| if options.Transform != nil { | ||
| value, err = options.Transform(value) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| if selection.Submitted && !selection.Cancelled { | ||
| submitted = &selection | ||
| } | ||
| return value, nil | ||
| }, |
There was a problem hiding this comment.
The submitted variable is not reset to nil at the start of each read attempt. If a previous attempt succeeded but failed validation, and the subsequent attempt is cancelled, submitted remains non-nil, causing the cancelled row to be rendered as successfully submitted. Reset submitted to nil at the start of the reader closure.
var submitted *dataTableSelection
return prompt.UntilValid(
prompt.Options[any]{Message: options.Message, Default: defaultValue, Required: options.Required, Validate: options.Validate},
func(_ int) (any, error) {
submitted = nil
selection, err := readDataTableSelection(options)
if err != nil {
return nil, err
}
value := selection.Value
if options.Transform != nil {
value, err = options.Transform(value)
if err != nil {
return nil, err
}
}
if selection.Submitted && !selection.Cancelled {
submitted = &selection
}
return value, nil
},| func Select(options ChoiceOptions) (any, error) { | ||
| if options.Scroll == 0 { | ||
| options.Scroll = 5 | ||
| } | ||
| validate := preserveInvalidChoiceDefault(&options) | ||
| defaultValue := transformedChoiceDefault(options) | ||
|
|
||
| return prompt.UntilValid( | ||
| prompt.Options[any]{Message: options.Message, Default: defaultValue, Required: requiredDefault(options.Required, true), Validate: validate}, | ||
| func(_ int) (any, error) { | ||
| value, label, submitted, err := readSelectedChoice(options) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| value, err = transformChoice(options, value) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if submitted { | ||
| renderSubmittedChoice(options.Message, label) | ||
| } | ||
| return value, nil | ||
| }, | ||
| nil, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The submitted choice frame is rendered before validation. If validation fails, the terminal will display a "submitted" frame followed by a validation error and another prompt, messing up the TUI layout. Use the onValid callback of UntilValid to render the submitted frame only after successful validation. This issue also applies to Multiselect, Confirm, Search, and MultiSearch.
func Select(options ChoiceOptions) (any, error) {
if options.Scroll == 0 {
options.Scroll = 5
}
validate := preserveInvalidChoiceDefault(&options)
defaultValue := transformedChoiceDefault(options)
var submittedLabel string
return prompt.UntilValid(
prompt.Options[any]{Message: options.Message, Default: defaultValue, Required: requiredDefault(options.Required, true), Validate: validate},
func(_ int) (any, error) {
value, label, submitted, err := readSelectedChoice(options)
if err != nil {
return nil, err
}
value, err = transformChoice(options, value)
if err != nil {
return nil, err
}
if submitted {
submittedLabel = label
} else {
submittedLabel = ""
}
return value, nil
},
func(any) error {
if submittedLabel != "" {
renderSubmittedChoice(options.Message, submittedLabel)
}
return nil
},
)| func (stream *Stream) render() { | ||
| environment.Current().Output.Write(RenderStreamFrame(stream.buffer.StableValue(), stream.buffer.Fading, StreamFadeStyles(false), 60)) | ||
| } |
There was a problem hiding this comment.
The render method writes the new stream frame to the output but never erases the previously rendered frame. Since RenderStreamFrame renders the entire stream history, each append will print the entire history again, flooding the terminal with duplicated output. Track the previously rendered frame and erase it using EraseRenderedFrame before writing the new frame.
| func Task[T any](label string, callback func(*Logger) (T, error), limit int, keepSummary bool, subLabel string) (T, error) { | ||
| var zero T | ||
| if callback == nil { | ||
| return zero, errMissingCallback("task") | ||
| } | ||
| logger := NewLogger(limit, label, subLabel) | ||
| output := environment.Current().Output | ||
| output.Write(terminal.HideCursorSequence()) | ||
| defer output.Write(terminal.ShowCursorSequence()) | ||
|
|
||
| frame := RenderTaskFrame(TaskFrameOptions{Label: logger.LabelValue, SubLabel: logger.SubLabelValue, Limit: logger.Limit(), Lines: logger.Lines, StableMessages: logger.Stable}) | ||
| output.Write(frame) | ||
|
|
||
| result, err := callback(logger) | ||
| EraseRenderedFrame(frame) | ||
| if err != nil { | ||
| output.Write(RenderTaskFrame(TaskFrameOptions{Label: logger.LabelValue, SubLabel: logger.SubLabelValue, Limit: logger.Limit(), Lines: logger.Lines, StableMessages: logger.Stable})) | ||
| return zero, err | ||
| } | ||
| output.Write(RenderTaskFrame(TaskFrameOptions{Finished: true, KeepSummary: keepSummary, Label: logger.LabelValue, SubLabel: logger.SubLabelValue, Limit: logger.Limit(), Lines: logger.Lines, StableMessages: logger.Stable})) | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
The Logger has no mechanism to trigger a redraw of the task frame when new logs are written. As a result, logs written during the execution of the task are completely invisible to the user until the task finishes, and the spinner remains static. Consider passing a redraw/render callback to the Logger so it can refresh the frame on state changes, similar to how Progress works.
| func With(patch Patch, callback func() error) error { | ||
| currentMu.Lock() | ||
| previous := current | ||
| current = merge(current, patch) | ||
| currentMu.Unlock() | ||
|
|
||
| defer func() { | ||
| currentMu.Lock() | ||
| current = previous | ||
| currentMu.Unlock() | ||
| }() | ||
|
|
||
| return callback() | ||
| } |
There was a problem hiding this comment.
The With function modifies the global current environment variable. If multiple goroutines call With concurrently, they will overwrite each other's environments and restore incorrect states, leading to race conditions. Consider using a context-based approach (context.Context) to propagate environment-scoped values instead of a global variable.
| func (input revertInput) ReadKey() (string, bool, error) { | ||
| pressed, ok, err := input.previous.ReadKey() | ||
| if err != nil || !ok { | ||
| return pressed, ok, err | ||
| } | ||
|
|
||
| if pressed != key.First(key.CtrlU) { | ||
| return pressed, ok, nil | ||
| } | ||
|
|
||
| if input.canRevert { | ||
| return "", false, ErrReverted | ||
| } | ||
|
|
||
| environment.Current().ErrorOutput.Write(theme.Red(" ⚠ This cannot be reverted.") + "\n") | ||
| return input.ReadKey() | ||
| } |
There was a problem hiding this comment.
Using recursion to handle non-revertible Ctrl+U key presses can lead to deep recursion and potential stack overflow if there are many consecutive Ctrl+U inputs (e.g., in scripted/automated testing). Refactor this to use a loop instead.
| func (input revertInput) ReadKey() (string, bool, error) { | |
| pressed, ok, err := input.previous.ReadKey() | |
| if err != nil || !ok { | |
| return pressed, ok, err | |
| } | |
| if pressed != key.First(key.CtrlU) { | |
| return pressed, ok, nil | |
| } | |
| if input.canRevert { | |
| return "", false, ErrReverted | |
| } | |
| environment.Current().ErrorOutput.Write(theme.Red(" ⚠ This cannot be reverted.") + "\n") | |
| return input.ReadKey() | |
| } | |
| func (input revertInput) ReadKey() (string, bool, error) { | |
| for { | |
| pressed, ok, err := input.previous.ReadKey() | |
| if err != nil || !ok { | |
| return pressed, ok, err | |
| } | |
| if pressed != key.First(key.CtrlU) { | |
| return pressed, ok, nil | |
| } | |
| if input.canRevert { | |
| return "", false, ErrReverted | |
| } | |
| environment.Current().ErrorOutput.Write(theme.Red(" ⚠ This cannot be reverted.") + "\n") | |
| } | |
| } |
| result := "" | ||
| activeCodes := "" | ||
| markerWidth := VisibleWidth(suffix) | ||
|
|
||
| for _, segment := range ParseAnsiSegments(value) { | ||
| if segment.Codes != activeCodes { | ||
| if activeCodes != "" { | ||
| result += ansiCloseSequence(activeCodes) | ||
| } | ||
| if segment.Codes != "" { | ||
| result += segment.Codes | ||
| } | ||
| activeCodes = segment.Codes | ||
| } | ||
|
|
||
| for _, char := range segment.Text { | ||
| if VisibleWidth(result+string(char))+markerWidth > width { | ||
| if activeCodes != "" { | ||
| result += ansiCloseSequence(activeCodes) | ||
| } | ||
|
|
||
| return result + suffix | ||
| } | ||
|
|
||
| result += string(char) | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling VisibleWidth(result+string(char)) inside a loop over every character is highly inefficient ($O(N^2)$) because VisibleWidth parses the entire accumulated string from scratch on every iteration. Keep a running visible width counter and use runewidth.RuneWidth(char) to update it in
result := ""
activeCodes := ""
markerWidth := VisibleWidth(suffix)
currentWidth := 0
for _, segment := range ParseAnsiSegments(value) {
if segment.Codes != activeCodes {
if activeCodes != "" {
result += ansiCloseSequence(activeCodes)
}
if segment.Codes != "" {
result += segment.Codes
}
activeCodes = segment.Codes
}
for _, char := range segment.Text {
charWidth := runewidth.RuneWidth(char)
if currentWidth+charWidth+markerWidth > width {
if activeCodes != "" {
result += ansiCloseSequence(activeCodes)
}
return result + suffix
}
result += string(char)
currentWidth += charWidth
}
}| func insertPrintable(value []string, cursor int, pressed string) (cursorResult, bool) { | ||
| if !IsPrintable(pressed) { | ||
| return cursorResult{}, false | ||
| } | ||
|
|
||
| next := append([]string{}, value...) | ||
| nextCursor := cursor | ||
| for _, char := range Characters(pressed) { | ||
| next = insertAt(next, nextCursor, char) | ||
| nextCursor++ | ||
| } | ||
|
|
||
| return cursorResult{cursor: nextCursor, value: next}, true | ||
| } |
There was a problem hiding this comment.
Calling insertAt in a loop allocates a new slice and copies elements on every single character of the inserted string, resulting in
func insertPrintable(value []string, cursor int, pressed string) (cursorResult, bool) {
if !IsPrintable(pressed) {
return cursorResult{}, false
}
chars := Characters(pressed)
next := make([]string, 0, len(value)+len(chars))
next = append(next, value[:cursor]...)
next = append(next, chars...)
next = append(next, value[cursor:]...)
return cursorResult{cursor: cursor + len(chars), value: next}, true
}
Summary
console/shellworkspace and build output underconsole/binconsole/shell/portal/main.goto the new shell packagePhase commits
chore(shell): move app stub into console shell workspacefeat(shell): port terminal key string and theme primitivesfeat(shell): port prompt environment and typed input enginefeat(shell): port prompt lifecycle and basic promptsfeat(shell): port choice search and suggest promptsfeat(shell): port output helpers and tablesfeat(shell): port status helpersfeat(shell): port form builderfeat(shell): expose public Go shell APIVerification
go test ./...fromconsole/shellpnpm --filter @ollin/shell buildpnpm --filter @ollin/shell testpnpm --filter @ollin/shell typecheckconsole/bin/shell versionconsole/bin/shellNotes
make format-allbecause it is TS-only and would operate on unrelated dirty TS files.