fix(tui): fix rendering corruption over multipass + Windows Terminal#709
fix(tui): fix rendering corruption over multipass + Windows Terminal#709euxaristia wants to merge 10 commits into
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR vendors a patched Bubble Tea v2 module, adds terminal rendering and runtime APIs, repairs throttled streaming redraws, documents local-build requirements, and runs both the root and patched-module test suites. ChangesBubble Tea patch integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/bubbletea-v2-patched/commands.go`:
- Around line 1-175: Move the maintained Bubble Tea fork out of third_party/ to
a repository-approved location, preserving package structure and references for
commands.go, environ.go, profile.go, and options.go. Move options_test.go and
tea_test.go beside the relocated runtime, along with all four listed
TestViewModel golden fixtures. Do not leave maintained source or tests under
third_party/; obtain an explicit policy exception instead if relocation is not
possible.
In `@third_party/bubbletea-v2-patched/cursed_renderer.go`:
- Around line 605-614: Move the patched Bubble Tea integration out of
third_party into a maintained fork or non-vendored local module, preserving the
cursedRenderer scroll-optimization change in cursed_renderer.go and its
cursor.go, renderer.go, logging.go, logging_test.go, and all listed testdata
golden fixtures. Update module/package references so the application and tests
use the relocated implementation, leaving no edited dependency files under
third_party.
- Around line 734-741: Update the wrapping-row calculation in the loop over
lines to count only the additional rows required when a line exceeds the
terminal width: use ceiling division for the total wrapped rows, then add one
fewer row beyond the initial row. Ensure an exact multiple such as 160 columns
at width 80 contributes one additional row, preventing blank lines in unmanaged
output.
- Around line 410-431: Replace direct color.Color comparisons in the render diff
loop and viewEquals with a shared nil-safe helper that compares normalized RGBA
values, avoiding interface equality and panics from non-comparable
implementations. Use the helper for cc/lcc, view.ForegroundColor/lfg, and
view.BackgroundColor/lbg while preserving nil reset behavior.
In `@third_party/bubbletea-v2-patched/exec_test.go`:
- Around line 105-113: Move the spyRenderer assignment for p.renderer to before
invoking p.Run(), ensuring the wrapped renderer observes execution-time
operations. Keep the existing calledReset assertion using the injected
spyRenderer after execution.
- Around line 48-51: Implement a reset() method on spyRenderer that sets
calledReset to true instead of delegating to the embedded renderer, so reset
invocations are intercepted and recorded.
In `@third_party/bubbletea-v2-patched/focus.go`:
- Around line 1-9: Move the patched Bubble Tea implementation containing
FocusMsg and BlurMsg out of third_party/ into an allowed repository location,
then update the module wiring and imports so all producers and consumers use the
relocated implementation. Do not leave modifications under third_party/.
In `@third_party/bubbletea-v2-patched/go.mod`:
- Line 1: Remove the patched dependency and all listed metadata files from
third_party/bubbletea-v2-patched, including go.mod, .gitattributes, both issue
templates, .gitignore, .golangci.yml, .goreleaser.yml, LICENSE, README.md,
Taskfile.yaml, and UPGRADE_GUIDE_V2.md. Maintain the dependency outside
third_party and update the project’s root go.mod with a replace directive
pointing to the maintained fork.
In `@third_party/bubbletea-v2-patched/key.go`:
- Line 155: Update the key mapping entry for KeyMediaRecord to use
uv.KeyMediaRecord instead of uv.KeyMediaPrev, while leaving the neighboring
media-key mappings unchanged.
In `@third_party/bubbletea-v2-patched/keyboard.go`:
- Line 1: Move the maintained Bubble Tea fork out of third_party while
preserving the patched implementations and tests. Relocate keyboard.go,
screen.go, tty.go, termios_unix.go, termios_windows.go, input.go,
screen_test.go, and all three listed TestViewModel golden fixtures to a
maintained fork/patch directory, update the root replace directive to reference
the new module location, and leave third_party vendored sources unmodified.
In `@third_party/bubbletea-v2-patched/logging_test.go`:
- Around line 10-28: Update TestLogToFile to call t.Fatal for LogToFile and
os.ReadFile failures, preventing execution after setup or read errors. Capture
the default logger’s output, prefix, and flags before invoking LogToFile, then
restore all three with cleanup (including restoring output before the test
exits) so later tests cannot inherit the mutated logger state or closed file.
In `@third_party/bubbletea-v2-patched/mouse.go`:
- Line 41: Update the MouseButton11 enum mapping to explicitly use
uv.MouseButton11, preventing it from inheriting the MouseButton10 value.
In `@third_party/bubbletea-v2-patched/nil_renderer.go`:
- Around line 1-10: Move the patched Bubble Tea module out of third_party/ or
obtain an explicit vendoring exception. Apply this to
third_party/bubbletea-v2-patched/nil_renderer.go (lines 1-10), exec.go (lines
1-10), tty_unix.go (lines 1-10), tty_windows.go (lines 1-10), signals_unix.go
(lines 1-10), signals_windows.go (lines 1-10), termios_bsd.go (lines 1-10),
termios_other.go (lines 1-9), cursed_renderer_test.go (lines 1-10), and
exec_test.go (lines 1-10); preserve the patched module’s package structure and
update references accordingly.
In `@third_party/bubbletea-v2-patched/tea_test.go`:
- Around line 255-260: Update the goroutine surrounding the repeated p.Quit()
calls to observe a completion channel, and close that channel immediately after
Run returns so the loop exits even when shutdowns equals preventCount. Preserve
the existing shutdown-count condition while ensuring the goroutine stops
promptly on Run completion.
In `@third_party/bubbletea-v2-patched/tea.go`:
- Around line 1-1439: Relocate the maintained Bubble Tea fork out of
third_party: move third_party/bubbletea-v2-patched/tea.go#L1-L1439,
clipboard.go#L1-L70, color.go#L1-L91, commands_test.go#L1-L67, key.go#L1-L371,
mod.go#L1-L27, mouse.go#L1-L144, paste.go#L1-L20, raw.go#L1-L37,
termcap.go#L1-L48, and xterm.go#L1-L22 into a non-vendored directory, keeping
the files together and preserving package symbols such as Program and
NewProgram. Update the root module replacement to target the new directory and
ensure all imports and tests use that replacement; the listed original sites
require no direct changes beyond removal after relocation.
In `@third_party/bubbletea-v2-patched/termios_unix.go`:
- Line 13: Update the `useBackspace` assignment to read the `BSDLY`/`BS0`
output-delay flags from `s.Oflag` instead of `s.Lflag`, preserving the existing
comparison and behavior.
In `@third_party/bubbletea-v2-patched/tty_unix.go`:
- Around line 15-35: Unify the platform-specific implementations into shared
cross-platform functions while preserving each platform’s behavior. In
third_party/bubbletea-v2-patched/tty_unix.go:15-35 and tty_windows.go:13-60,
merge initInput; in tty_unix.go:37-47 and tty_windows.go:62-65, merge
suspendProcess; in signals_unix.go:15-33 and signals_windows.go:8-10, merge
listenForResize; and in termios_bsd.go:11-13 and termios_other.go:8-8, merge
checkOptimizedMovements. Replace build-tag duplication with runtime or standard
conditional checks, retain the existing Unix, Windows, BSD, and other-platform
behavior, and remove the redundant platform-specific definitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e73a66b8-bd0f-4df9-b0bc-9d7cfc68117b
⛔ Files ignored due to path filters (1)
third_party/bubbletea-v2-patched/go.sumis excluded by!**/*.sum
📒 Files selected for processing (76)
go.modinternal/tui/model.gothird_party/bubbletea-v2-patched/.gitattributesthird_party/bubbletea-v2-patched/.github/ISSUE_TEMPLATE/bug.ymlthird_party/bubbletea-v2-patched/.github/ISSUE_TEMPLATE/bug_report.mdthird_party/bubbletea-v2-patched/.github/ISSUE_TEMPLATE/config.ymlthird_party/bubbletea-v2-patched/.github/ISSUE_TEMPLATE/feature_request.mdthird_party/bubbletea-v2-patched/.github/dependabot.ymlthird_party/bubbletea-v2-patched/.github/workflows/build.ymlthird_party/bubbletea-v2-patched/.github/workflows/coverage.ymlthird_party/bubbletea-v2-patched/.github/workflows/dependabot-sync.ymlthird_party/bubbletea-v2-patched/.github/workflows/examples.ymlthird_party/bubbletea-v2-patched/.github/workflows/lint-sync.ymlthird_party/bubbletea-v2-patched/.github/workflows/lint.ymlthird_party/bubbletea-v2-patched/.github/workflows/release.ymlthird_party/bubbletea-v2-patched/.gitignorethird_party/bubbletea-v2-patched/.golangci.ymlthird_party/bubbletea-v2-patched/.goreleaser.ymlthird_party/bubbletea-v2-patched/LICENSEthird_party/bubbletea-v2-patched/README.mdthird_party/bubbletea-v2-patched/Taskfile.yamlthird_party/bubbletea-v2-patched/UPGRADE_GUIDE_V2.mdthird_party/bubbletea-v2-patched/clipboard.gothird_party/bubbletea-v2-patched/color.gothird_party/bubbletea-v2-patched/commands.gothird_party/bubbletea-v2-patched/commands_test.gothird_party/bubbletea-v2-patched/cursed_renderer.gothird_party/bubbletea-v2-patched/cursed_renderer_test.gothird_party/bubbletea-v2-patched/cursor.gothird_party/bubbletea-v2-patched/environ.gothird_party/bubbletea-v2-patched/exec.gothird_party/bubbletea-v2-patched/exec_test.gothird_party/bubbletea-v2-patched/focus.gothird_party/bubbletea-v2-patched/go.modthird_party/bubbletea-v2-patched/input.gothird_party/bubbletea-v2-patched/key.gothird_party/bubbletea-v2-patched/keyboard.gothird_party/bubbletea-v2-patched/logging.gothird_party/bubbletea-v2-patched/logging_test.gothird_party/bubbletea-v2-patched/mod.gothird_party/bubbletea-v2-patched/mouse.gothird_party/bubbletea-v2-patched/nil_renderer.gothird_party/bubbletea-v2-patched/options.gothird_party/bubbletea-v2-patched/options_test.gothird_party/bubbletea-v2-patched/paste.gothird_party/bubbletea-v2-patched/profile.gothird_party/bubbletea-v2-patched/raw.gothird_party/bubbletea-v2-patched/renderer.gothird_party/bubbletea-v2-patched/screen.gothird_party/bubbletea-v2-patched/screen_test.gothird_party/bubbletea-v2-patched/signals_unix.gothird_party/bubbletea-v2-patched/signals_windows.gothird_party/bubbletea-v2-patched/tea.gothird_party/bubbletea-v2-patched/tea_test.gothird_party/bubbletea-v2-patched/termcap.gothird_party/bubbletea-v2-patched/termios_bsd.gothird_party/bubbletea-v2-patched/termios_other.gothird_party/bubbletea-v2-patched/termios_unix.gothird_party/bubbletea-v2-patched/termios_windows.gothird_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.goldenthird_party/bubbletea-v2-patched/tty.gothird_party/bubbletea-v2-patched/tty_unix.gothird_party/bubbletea-v2-patched/tty_windows.gothird_party/bubbletea-v2-patched/xterm.go
628818b to
6e39e6e
Compare
The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs Gitlawb#709
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Builds and vets clean and the tui suite passes on Windows, and the fix is sound for the corruption case: vendoring bubbletea/v2 with scroll-region optimization disabled, plus forcing a ClearScreen on streamed newlines, addresses the ghost-caret/repaint issue. The one thing I'd flag for kevin before this lands is that the tradeoff is now unconditional — every terminal does full repaints and a per-newline clear, not just the multipass-over-Windows-Terminal combo that motivated it. An env var to opt back into the fast path for terminals that handle scroll regions correctly would soften the blast radius; if the call is that detection isn't reliable, that's defensible, but it's a product call worth making consciously rather than a clear ship-it from me.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not ship a local
replacedirective in the release module
go.mod:45
This makes the standard versioned install path fail:go install github.com/Gitlawb/zero/cmd/zero@<version>evaluates the downloaded module without a main module, and Go rejects a targetgo.modcontaining behavior-changingreplacedirectives. Source-checkout builds and the CI matrix therefore pass while users cannot install a release by its version. Publish the patched Bubble Tea as a resolvable versioned fork (or otherwise avoid a root-modulereplace) before releasing; this also avoids silently pinning all future Bubble Tea upgrades to this copied v2.0.8 snapshot. -
[P1] Make the newline redraw synchronous with the render it is meant to repair
internal/tui/model.go:1916
tea.ClearScreenis sent asynchronously, so the renderer can flush the new streamed view first. When the later clear message arrives,clearScreen()only appends an erase to the renderer buffer; the next frame sees the samelastViewand returns without flushing that buffer. The erase/redraw is then deferred until an unrelated view change (or shutdown), leaving the ghost caret intact under the timing the workaround is intended to handle. Trigger a forced redraw in an ordered path, or invalidate the renderer's last view when handling the clear, and cover the reordered/fade-disabled case. -
[P2] Avoid a full-screen repaint for every streamed newline on unaffected terminals
internal/tui/model.go:1915
Every coalesced streaming delta containing a newline now schedulestea.ClearScreen. In the always-alt-screen TUI, that erases the renderer buffer and makes the following frame repaint the complete viewport. Normal code, logs, and diffs therefore turn into repeated full-screen writes for every terminal, including SSH and slow links unrelated to the Multipass/Windows Terminal failure, causing a substantial throughput and latency regression. Scope this workaround to an explicit opt-in/affected terminal path, or coalesce/rate-limit a reliable full redraw rather than clearing once per newline.
|
Pushed 52e7d36 addressing the concrete findings in the fork and the perf scoping: KeyMediaRecord and MouseButton11 no longer inherit the previous const's expression; BSDLY/BS0 is read from Oflag; wrapped-row counting uses (lineWidth-1)/w so exact-width lines don't insert blank rows; color comparisons go through a nil-safe normalized-RGBA helper instead of interface equality; clearScreen invalidates lastView so the queued erase flushes with the next frame instead of deferring until an unrelated view change (the reordering behind the P1 ghost-caret case); the spy renderer implements reset() and is injected before Run; the logging test restores the default logger; and the Quit-pump goroutine stops when Run returns. On the P2 repaint cost: the streamed-newline ClearScreen is now rate-limited to ~10/s and ZERO_NO_STREAM_CLEAR=1 opts a correctly-rendering terminal back into the fast incremental path. Still open and genuinely a maintainer call: the fork's location under third_party/ and the root go.mod replace directive (which breaks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
third_party/bubbletea-v2-patched/renderer.go (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate comment to match the constant name.
The comment refers to
defaultFramerate, but the constant is nameddefaultFPS.📝 Proposed fix
- // defaultFramerate specifies the maximum interval at which we should + // defaultFPS specifies the maximum interval at which we should // update the view. defaultFPS = 60🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@third_party/bubbletea-v2-patched/renderer.go` around lines 11 - 13, Update the comment immediately above the defaultFPS constant to refer to defaultFPS instead of defaultFramerate, keeping the existing description unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/bubbletea-v2-patched/renderer.go`:
- Line 1: Revert changes to the vendored package under third_party, including
renderer.go. Manage the patched Bubble Tea implementation externally through a
patch workflow or a go.mod replace directive targeting a separate module,
without editing vendored files directly.
---
Nitpick comments:
In `@third_party/bubbletea-v2-patched/renderer.go`:
- Around line 11-13: Update the comment immediately above the defaultFPS
constant to refer to defaultFPS instead of defaultFramerate, keeping the
existing description unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45da6c20-cd0f-4c13-b5cd-6fe443d67408
⛔ Files ignored due to path filters (1)
third_party/bubbletea-v2-patched/go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
go.modinternal/tui/model.gothird_party/bubbletea-v2-patched/LICENSEthird_party/bubbletea-v2-patched/clipboard.gothird_party/bubbletea-v2-patched/color.gothird_party/bubbletea-v2-patched/commands.gothird_party/bubbletea-v2-patched/commands_test.gothird_party/bubbletea-v2-patched/cursed_renderer.gothird_party/bubbletea-v2-patched/cursed_renderer_test.gothird_party/bubbletea-v2-patched/cursor.gothird_party/bubbletea-v2-patched/environ.gothird_party/bubbletea-v2-patched/exec.gothird_party/bubbletea-v2-patched/exec_test.gothird_party/bubbletea-v2-patched/focus.gothird_party/bubbletea-v2-patched/go.modthird_party/bubbletea-v2-patched/input.gothird_party/bubbletea-v2-patched/key.gothird_party/bubbletea-v2-patched/keyboard.gothird_party/bubbletea-v2-patched/logging.gothird_party/bubbletea-v2-patched/logging_test.gothird_party/bubbletea-v2-patched/mod.gothird_party/bubbletea-v2-patched/mouse.gothird_party/bubbletea-v2-patched/nil_renderer.gothird_party/bubbletea-v2-patched/options.gothird_party/bubbletea-v2-patched/options_test.gothird_party/bubbletea-v2-patched/paste.gothird_party/bubbletea-v2-patched/profile.gothird_party/bubbletea-v2-patched/raw.gothird_party/bubbletea-v2-patched/renderer.gothird_party/bubbletea-v2-patched/screen.gothird_party/bubbletea-v2-patched/screen_test.gothird_party/bubbletea-v2-patched/signals_unix.gothird_party/bubbletea-v2-patched/signals_windows.gothird_party/bubbletea-v2-patched/tea.gothird_party/bubbletea-v2-patched/tea_test.gothird_party/bubbletea-v2-patched/termcap.gothird_party/bubbletea-v2-patched/termios_bsd.gothird_party/bubbletea-v2-patched/termios_other.gothird_party/bubbletea-v2-patched/termios_unix.gothird_party/bubbletea-v2-patched/termios_windows.gothird_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.goldenthird_party/bubbletea-v2-patched/tty.gothird_party/bubbletea-v2-patched/tty_unix.gothird_party/bubbletea-v2-patched/tty_windows.gothird_party/bubbletea-v2-patched/xterm.go
🚧 Files skipped from review as they are similar to previous changes (54)
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.golden
- third_party/bubbletea-v2-patched/LICENSE
- third_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.golden
- third_party/bubbletea-v2-patched/termios_other.go
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.golden
- third_party/bubbletea-v2-patched/mod.go
- third_party/bubbletea-v2-patched/profile.go
- third_party/bubbletea-v2-patched/screen.go
- third_party/bubbletea-v2-patched/input.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.golden
- third_party/bubbletea-v2-patched/go.mod
- third_party/bubbletea-v2-patched/environ.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.golden
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.golden
- third_party/bubbletea-v2-patched/paste.go
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.golden
- third_party/bubbletea-v2-patched/clipboard.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.golden
- third_party/bubbletea-v2-patched/focus.go
- third_party/bubbletea-v2-patched/termios_unix.go
- third_party/bubbletea-v2-patched/signals_windows.go
- third_party/bubbletea-v2-patched/termios_windows.go
- third_party/bubbletea-v2-patched/nil_renderer.go
- third_party/bubbletea-v2-patched/tty_unix.go
- third_party/bubbletea-v2-patched/options_test.go
- third_party/bubbletea-v2-patched/signals_unix.go
- third_party/bubbletea-v2-patched/screen_test.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.golden
- third_party/bubbletea-v2-patched/keyboard.go
- third_party/bubbletea-v2-patched/termcap.go
- third_party/bubbletea-v2-patched/xterm.go
- third_party/bubbletea-v2-patched/logging_test.go
- third_party/bubbletea-v2-patched/cursor.go
- go.mod
- third_party/bubbletea-v2-patched/cursed_renderer_test.go
- third_party/bubbletea-v2-patched/tty_windows.go
- third_party/bubbletea-v2-patched/commands_test.go
- third_party/bubbletea-v2-patched/raw.go
- third_party/bubbletea-v2-patched/color.go
- third_party/bubbletea-v2-patched/mouse.go
- third_party/bubbletea-v2-patched/commands.go
- third_party/bubbletea-v2-patched/logging.go
- third_party/bubbletea-v2-patched/key.go
- third_party/bubbletea-v2-patched/exec.go
- internal/tui/model.go
- third_party/bubbletea-v2-patched/options.go
- third_party/bubbletea-v2-patched/tty.go
- third_party/bubbletea-v2-patched/exec_test.go
- third_party/bubbletea-v2-patched/tea_test.go
- third_party/bubbletea-v2-patched/cursed_renderer.go
- third_party/bubbletea-v2-patched/tea.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Rebase this branch before merging
package.json:3
The head's merge-base is9043baed, while the current review base is378d538; it is missing the 13 intervening mainline commits. Consequently, merging this renderer-only PR deletes 9,159 current lines across 149 files, including the released 0.4.0 changelog and package metadata (this line regresses the version to 0.3.0), semantic tool-output budgeting, completion policy, execution profiles, and the missing-nativezero doctorfallback. Rebase/reapply the TUI patch on currentmainso none of those shipped changes are reverted. -
[P2] Preserve a redraw for rate-limited newline transitions
internal/tui/model.go:1936
The new throttle drops every streamed newline arriving within 100 ms instead of scheduling a deferred clear. A second newline-bearingagentTextMsgcan therefore take the same incremental path that leaves the old caret behind; if that is the final delta, no later clear repairs it. Coalesce to a guaranteed deferred redraw (and add a two-newline-within-the-window regression test) rather than discarding the event.
|
Pushed 62fd5dc. Fixed:
Not fixing, with reasons:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 1963-1971: Use a shared streamClearThrottle constant instead of
duplicating the 100ms duration, and update both agentTextMsg
(internal/tui/model.go:1963-1971) and streamClearFlushMsg
(internal/tui/model.go:2069-2074) to calculate elapsed time and assign
timestamps through m.now() rather than time.Since or time.Now().
In `@internal/tui/stream_clear_test.go`:
- Around line 21-37: Update msgIncludesClearScreen to inspect tea.BatchMsg
commands without invoking c(), avoiding synchronous side effects such as timer
scheduling. Add the reflect import and compare each command’s function pointer
with the tea.ClearScreen function pointer, preserving nil handling and the
existing false result when no match is found; nested batches do not need to be
supported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b4bab70-41ba-47da-9f7b-b8083805180c
📒 Files selected for processing (4)
internal/tui/model.gointernal/tui/stream_clear_test.gothird_party/bubbletea-v2-patched/logging_test.gothird_party/bubbletea-v2-patched/renderer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- third_party/bubbletea-v2-patched/logging_test.go
- third_party/bubbletea-v2-patched/renderer.go
The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs Gitlawb#709
62fd5dc to
cb25d5b
Compare
|
@jatmn follow-up on your latest review (52e7d36): [P1] Rebase — resolved. Branch is on tip of [P2] Preserve redraw for rate-limited newlines — resolved across
Still open as a maintainer call (not a code fix in this pass): publishing the bubbletea fork as a versioned remote module so the root |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/stream_clear_test.go (1)
30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid executing side effects and prevent potential CI flakiness.
While the 20ms timeout prevents the test from blocking on
Tickcommands, executingcmd()in a background goroutine still triggers the command's side effects. Additionally, a 20ms timeout can cause test flakiness in heavily loaded CI environments where goroutine scheduling might be delayed.You can safely identify a
tea.Batchcommand without evaluating it by comparing its code pointer to a known batch closure. This allows you to expand batches instantly and synchronously, safely ignoring all other commands.⚡ Proposed fix
- // Expand batches (and any other immediately-returning cmds). Bound the - // wait so a deferred stream-clear Tick is never a multi-100ms sleep. - ch := make(chan tea.Msg, 1) - go func() { ch <- cmd() }() - select { - case msg := <-ch: - return msgIncludesClearScreen(msg) - case <-time.After(20 * time.Millisecond): - return false - } + // Identify tea.Batch closures by code pointer to safely expand them + // without executing arbitrary side-effect commands (like Tick). + batchPtr := reflect.ValueOf(tea.Batch(tea.ClearScreen)).Pointer() + if reflect.ValueOf(cmd).Pointer() == batchPtr { + // It's a batch. Executing it just returns the BatchMsg instantly. + return msgIncludesClearScreen(cmd()) + } + + return false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/stream_clear_test.go` around lines 30 - 39, Update the command inspection helper around the goroutine and 20ms timeout so it never executes commands or waits on scheduling. Identify tea.Batch commands by comparing their function code pointer with the known batch closure, expand those batches synchronously, and ignore all other commands while preserving clear-screen detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/bubbletea-v2-patched/tty_windows.go`:
- Around line 1-64: Remove direct edits under third_party/ by relocating or
otherwise managing the patched Bubble Tea fork outside the vendored tree. Apply
this to third_party/bubbletea-v2-patched/tty_windows.go (lines 1-64),
third_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.golden
(line 1), and third_party/bubbletea-v2-patched/logging_test.go (lines 1-42),
preserving the patched implementation and its associated test data and test
coverage.
---
Nitpick comments:
In `@internal/tui/stream_clear_test.go`:
- Around line 30-39: Update the command inspection helper around the goroutine
and 20ms timeout so it never executes commands or waits on scheduling. Identify
tea.Batch commands by comparing their function code pointer with the known batch
closure, expand those batches synchronously, and ignore all other commands while
preserving clear-screen detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80c24974-c2ae-4bfc-a707-1d1e9f2cd122
⛔ Files ignored due to path filters (1)
third_party/bubbletea-v2-patched/go.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
go.modinternal/tui/model.gointernal/tui/stream_clear_test.gothird_party/bubbletea-v2-patched/LICENSEthird_party/bubbletea-v2-patched/clipboard.gothird_party/bubbletea-v2-patched/color.gothird_party/bubbletea-v2-patched/commands.gothird_party/bubbletea-v2-patched/commands_test.gothird_party/bubbletea-v2-patched/cursed_renderer.gothird_party/bubbletea-v2-patched/cursed_renderer_test.gothird_party/bubbletea-v2-patched/cursor.gothird_party/bubbletea-v2-patched/environ.gothird_party/bubbletea-v2-patched/exec.gothird_party/bubbletea-v2-patched/exec_test.gothird_party/bubbletea-v2-patched/focus.gothird_party/bubbletea-v2-patched/go.modthird_party/bubbletea-v2-patched/input.gothird_party/bubbletea-v2-patched/key.gothird_party/bubbletea-v2-patched/keyboard.gothird_party/bubbletea-v2-patched/logging.gothird_party/bubbletea-v2-patched/logging_test.gothird_party/bubbletea-v2-patched/mod.gothird_party/bubbletea-v2-patched/mouse.gothird_party/bubbletea-v2-patched/nil_renderer.gothird_party/bubbletea-v2-patched/options.gothird_party/bubbletea-v2-patched/options_test.gothird_party/bubbletea-v2-patched/paste.gothird_party/bubbletea-v2-patched/profile.gothird_party/bubbletea-v2-patched/raw.gothird_party/bubbletea-v2-patched/renderer.gothird_party/bubbletea-v2-patched/screen.gothird_party/bubbletea-v2-patched/screen_test.gothird_party/bubbletea-v2-patched/signals_unix.gothird_party/bubbletea-v2-patched/signals_windows.gothird_party/bubbletea-v2-patched/tea.gothird_party/bubbletea-v2-patched/tea_test.gothird_party/bubbletea-v2-patched/termcap.gothird_party/bubbletea-v2-patched/termios_bsd.gothird_party/bubbletea-v2-patched/termios_other.gothird_party/bubbletea-v2-patched/termios_unix.gothird_party/bubbletea-v2-patched/termios_windows.gothird_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.goldenthird_party/bubbletea-v2-patched/tty.gothird_party/bubbletea-v2-patched/tty_unix.gothird_party/bubbletea-v2-patched/tty_windows.gothird_party/bubbletea-v2-patched/xterm.go
🚧 Files skipped from review as they are similar to previous changes (54)
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.golden
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.golden
- third_party/bubbletea-v2-patched/focus.go
- third_party/bubbletea-v2-patched/LICENSE
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.golden
- third_party/bubbletea-v2-patched/xterm.go
- third_party/bubbletea-v2-patched/input.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.golden
- third_party/bubbletea-v2-patched/signals_unix.go
- third_party/bubbletea-v2-patched/screen.go
- third_party/bubbletea-v2-patched/termios_windows.go
- third_party/bubbletea-v2-patched/profile.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.golden
- third_party/bubbletea-v2-patched/termcap.go
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.golden
- third_party/bubbletea-v2-patched/raw.go
- third_party/bubbletea-v2-patched/paste.go
- third_party/bubbletea-v2-patched/mod.go
- third_party/bubbletea-v2-patched/termios_bsd.go
- third_party/bubbletea-v2-patched/logging.go
- third_party/bubbletea-v2-patched/go.mod
- third_party/bubbletea-v2-patched/termios_unix.go
- go.mod
- third_party/bubbletea-v2-patched/keyboard.go
- third_party/bubbletea-v2-patched/tty_unix.go
- third_party/bubbletea-v2-patched/options_test.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.golden
- third_party/bubbletea-v2-patched/termios_other.go
- third_party/bubbletea-v2-patched/color.go
- third_party/bubbletea-v2-patched/clipboard.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.golden
- third_party/bubbletea-v2-patched/nil_renderer.go
- third_party/bubbletea-v2-patched/renderer.go
- third_party/bubbletea-v2-patched/exec_test.go
- third_party/bubbletea-v2-patched/commands.go
- third_party/bubbletea-v2-patched/environ.go
- third_party/bubbletea-v2-patched/screen_test.go
- third_party/bubbletea-v2-patched/signals_windows.go
- third_party/bubbletea-v2-patched/cursor.go
- third_party/bubbletea-v2-patched/cursed_renderer_test.go
- third_party/bubbletea-v2-patched/exec.go
- third_party/bubbletea-v2-patched/mouse.go
- third_party/bubbletea-v2-patched/key.go
- third_party/bubbletea-v2-patched/options.go
- third_party/bubbletea-v2-patched/tty.go
- third_party/bubbletea-v2-patched/tea_test.go
- third_party/bubbletea-v2-patched/commands_test.go
- third_party/bubbletea-v2-patched/cursed_renderer.go
- internal/tui/model.go
- third_party/bubbletea-v2-patched/tea.go
The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs Gitlawb#709
c889f6a to
9fdccbc
Compare
|
Rebased onto current main (fbf8598), no conflicts. Full test suite passes locally except TestAltScreenTranscriptScrollKeepsFooterFixed, which also fails on unmodified main when the checkout path is long enough to push the title bar into a narrower width tier, so it is pre-existing and unrelated to this branch. The vendored fork tests pass as well. The remaining review items (the third_party placement question and the newline throttle) are still open; this push is just the rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@third_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.golden`:
- Line 1: Revert the modification to the vendored golden fixture under
third_party/. If the fixture must be maintained, relocate the test and fixture
to a repository-owned path outside third_party/; otherwise leave the third-party
file unchanged and obtain a documented policy exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea849e73-d98b-4f9b-bb74-20c9cca4fddc
⛔ Files ignored due to path filters (1)
third_party/bubbletea-v2-patched/go.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
go.modinternal/tui/model.gointernal/tui/stream_clear_test.gothird_party/bubbletea-v2-patched/LICENSEthird_party/bubbletea-v2-patched/clipboard.gothird_party/bubbletea-v2-patched/color.gothird_party/bubbletea-v2-patched/commands.gothird_party/bubbletea-v2-patched/commands_test.gothird_party/bubbletea-v2-patched/cursed_renderer.gothird_party/bubbletea-v2-patched/cursed_renderer_test.gothird_party/bubbletea-v2-patched/cursor.gothird_party/bubbletea-v2-patched/environ.gothird_party/bubbletea-v2-patched/exec.gothird_party/bubbletea-v2-patched/exec_test.gothird_party/bubbletea-v2-patched/focus.gothird_party/bubbletea-v2-patched/go.modthird_party/bubbletea-v2-patched/input.gothird_party/bubbletea-v2-patched/key.gothird_party/bubbletea-v2-patched/keyboard.gothird_party/bubbletea-v2-patched/logging.gothird_party/bubbletea-v2-patched/logging_test.gothird_party/bubbletea-v2-patched/mod.gothird_party/bubbletea-v2-patched/mouse.gothird_party/bubbletea-v2-patched/nil_renderer.gothird_party/bubbletea-v2-patched/options.gothird_party/bubbletea-v2-patched/options_test.gothird_party/bubbletea-v2-patched/paste.gothird_party/bubbletea-v2-patched/profile.gothird_party/bubbletea-v2-patched/raw.gothird_party/bubbletea-v2-patched/renderer.gothird_party/bubbletea-v2-patched/screen.gothird_party/bubbletea-v2-patched/screen_test.gothird_party/bubbletea-v2-patched/signals_unix.gothird_party/bubbletea-v2-patched/signals_windows.gothird_party/bubbletea-v2-patched/tea.gothird_party/bubbletea-v2-patched/tea_test.gothird_party/bubbletea-v2-patched/termcap.gothird_party/bubbletea-v2-patched/termios_bsd.gothird_party/bubbletea-v2-patched/termios_other.gothird_party/bubbletea-v2-patched/termios_unix.gothird_party/bubbletea-v2-patched/termios_windows.gothird_party/bubbletea-v2-patched/testdata/TestClearMsg/bg_fg_cur_color.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.goldenthird_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.goldenthird_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.goldenthird_party/bubbletea-v2-patched/tty.gothird_party/bubbletea-v2-patched/tty_unix.gothird_party/bubbletea-v2-patched/tty_windows.gothird_party/bubbletea-v2-patched/xterm.go
🚧 Files skipped from review as they are similar to previous changes (56)
- third_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen_autoexit.golden
- third_party/bubbletea-v2-patched/termios_windows.go
- third_party/bubbletea-v2-patched/signals_unix.go
- third_party/bubbletea-v2-patched/focus.go
- third_party/bubbletea-v2-patched/environ.go
- third_party/bubbletea-v2-patched/input.go
- third_party/bubbletea-v2-patched/xterm.go
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/clear_screen.golden
- third_party/bubbletea-v2-patched/cursor.go
- third_party/bubbletea-v2-patched/signals_windows.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hide.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_cellmotion.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_disable.golden
- third_party/bubbletea-v2-patched/termios_bsd.go
- third_party/bubbletea-v2-patched/testdata/TestClearMsg/read_set_clipboard.golden
- third_party/bubbletea-v2-patched/paste.go
- third_party/bubbletea-v2-patched/logging.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bp_stop_start.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/altscreen.golden
- third_party/bubbletea-v2-patched/termios_unix.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/cursor_hideshow.golden
- third_party/bubbletea-v2-patched/LICENSE
- third_party/bubbletea-v2-patched/testdata/TestViewModel/mouse_allmotion.golden
- third_party/bubbletea-v2-patched/testdata/TestViewModel/kitty_stop_startreleases.golden
- third_party/bubbletea-v2-patched/termcap.go
- third_party/bubbletea-v2-patched/nil_renderer.go
- third_party/bubbletea-v2-patched/profile.go
- third_party/bubbletea-v2-patched/screen.go
- third_party/bubbletea-v2-patched/mod.go
- third_party/bubbletea-v2-patched/raw.go
- third_party/bubbletea-v2-patched/tty_windows.go
- third_party/bubbletea-v2-patched/testdata/TestViewModel/bg_set_color.golden
- third_party/bubbletea-v2-patched/go.mod
- third_party/bubbletea-v2-patched/clipboard.go
- third_party/bubbletea-v2-patched/termios_other.go
- third_party/bubbletea-v2-patched/keyboard.go
- third_party/bubbletea-v2-patched/logging_test.go
- third_party/bubbletea-v2-patched/commands_test.go
- third_party/bubbletea-v2-patched/screen_test.go
- third_party/bubbletea-v2-patched/tty.go
- third_party/bubbletea-v2-patched/renderer.go
- third_party/bubbletea-v2-patched/options_test.go
- internal/tui/stream_clear_test.go
- third_party/bubbletea-v2-patched/tty_unix.go
- third_party/bubbletea-v2-patched/exec_test.go
- third_party/bubbletea-v2-patched/cursed_renderer_test.go
- third_party/bubbletea-v2-patched/exec.go
- third_party/bubbletea-v2-patched/color.go
- third_party/bubbletea-v2-patched/key.go
- third_party/bubbletea-v2-patched/mouse.go
- third_party/bubbletea-v2-patched/options.go
- third_party/bubbletea-v2-patched/tea_test.go
- internal/tui/model.go
- third_party/bubbletea-v2-patched/commands.go
- third_party/bubbletea-v2-patched/cursed_renderer.go
- third_party/bubbletea-v2-patched/tea.go
Dune was rendering as a light/sandy theme with a 55/55/55 grey surface, which did not match Claude Code's dark colorblind mode. Rebase the palette on a near-black surface (#0e0e10) with the brand-orange accent from openclaude's dark-daltonized theme, and move the warm grey to the user-prompt bubble only. Brighten the muted/faint/faintest gray ramp and darken selBg/addBg/delBg so all WCAG-AA and xterm-256 contrast invariants hold. Updates the stale test that still marked Dune as light. Refs Gitlawb#709
The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs Gitlawb#709
9fdccbc to
d7b3525
Compare
Follow-up (rebase + review fixes)[P1] RebaseRebased onto current [P2] Throttled stream-clear redrawAlready on the branch (from earlier commits, still present after rebase):
third_party policyMoved the maintained bubbletea/v2 fork from The fork is still required for unconditional Still open before non-draft / releaseRoot-module Verification
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve the project’s intended versioned Go-install contract
go.mod:47
The localreplace charm.land/bubbletea/v2 => ./patches/bubbletea-v2makes versionedgo install github.com/Gitlawb/zero/cmd/zero@<version>unusable: versioned installs cannot use a module containing a replacement directive, and the local replacement is not a resolvable dependency for that install. The PR discussion identifies this as a release concern. If versioned Go installs are a supported distribution path, publish the patched module at a resolvable version or use an install-safe integration; otherwise document that this path is intentionally unsupported before merging. -
[P2] Exercise the patched Bubble Tea module in CI
go.mod:47
patches/bubbletea-v2is a separate Go module, so the repository's existinggo test ./...jobs stop at that module boundary. None of the seven fork test files added here run in the PR's required CI checks, despite the fork carrying renderer, terminal, platform, and lifecycle changes in addition to the targeted scroll setting. Add a CI step that runs the fork's tests (including the appropriate platform coverage) so future changes to the replacement cannot pass solely because the consumer module compiles.
|
Thanks for chasing this down @euxaristia, the corruption repro is real, and the model.go side of it (forcing a repaint on the streamed newline to clear the ghost caret) is a solid, self-contained fix. The part we can't take is the vendored bubbletea fork. jatmn's P1 is a blocker on its own: a So the direction we would like: drop the fork, keep the lightweight fix. For the scroll-region issue the better homes are (1) upstream it to bubbletea, or (2) a runtime toggle if bubbletea already exposes one, so we set it without vendoring anything. If neither works out and a patch really is unavoidable, that is an owner-level call since it means owning a fork, not something to land via Genuinely appreciate the work, the debugging was the hard part here. If you want, split the model.go fix into its own small PR and we can move that one quickly. |
Vendors a patched charm.land/bubbletea/v2 (via a local go.mod replace) with hard scroll optimization (using terminal scroll-region sequences to shift existing lines instead of repainting) disabled unconditionally, instead of only on GOOS=="windows". That GOOS check only catches a Windows terminal driving the program directly. It misses a Linux process running inside a multipass VM whose output is ultimately displayed by Windows Terminal over a remote shell, which corrupts rendering badly (garbled/overlapping text, worse on resize) the same way the check exists to prevent. There's no reliable way to detect that remote hop from inside the VM, so scroll optimization is now off in all cases.
The streaming caret (appendStreamingCursor) is appended to whichever visual line is currently last. Over a remote terminal chain (multipass VM output displayed by Windows Terminal), the renderer's incremental cell diff sometimes fails to clear the caret's old cell when a newline moves it to a new line, leaving ghost carets behind. Fire tea.ClearScreen when a streamed delta contains a newline: that's exactly the moment the caret moves and the risk exists, so it forces a full redraw right then instead of leaving it to the incremental diff, without doing so on every animation tick.
The vendored bubbletea-v2-patched module is ~99% byte-identical to upstream charm.land/bubbletea/v2 v2.0.8; only cursed_renderer.go differs (one line disabling hard scroll optimization). The .github workflows, README.md, UPGRADE_GUIDE_V2.md, Taskfile.yaml and CI config files are upstream repo metadata that serve no purpose inside this repo, so remove them to slim the PR. The Go source and testdata golden files are kept because Go's replace directive requires the full module to be present and buildable. Refs Gitlawb#709
…ed fork Fork correctness fixes (all flagged by review): - key.go: KeyMediaRecord repeated the previous const expression and decoded record presses as previous-track; map it to uv.KeyMediaRecord. - mouse.go: MouseButton11 likewise collapsed onto MouseButton10. - termios_unix.go: BSDLY/BS0 are output-delay bits; read them from Oflag instead of Lflag so useBackspace is set correctly. - cursed_renderer.go: count wrapped rows with (lineWidth-1)/w so an exact terminal-width multiple no longer inserts a blank line; compare colors by normalized RGBA via a nil-safe helper instead of interface equality (which panics on non-comparable implementations); and make clearScreen invalidate lastView so the queued erase flushes with the next frame instead of deferring until an unrelated view change — the reordering that could leave the ghost caret in place. - exec_test.go: implement reset() on the spy renderer and inject it before Run; assert exec preserves renderer state (releaseTerminal with reset=false). - logging_test.go: restore the default logger's output/prefix/flags and fail fast on setup errors. - tea_test.go: tie the Quit-pumping goroutine to a channel closed when Run returns instead of leaking it until process exit. TUI scoping (review P2): the forced redraw on streamed newlines is now rate-limited to ~10/s so heavy streaming output no longer turns every newline into a full-screen repaint, and ZERO_NO_STREAM_CLEAR=1 opts a correctly-rendering terminal back into the fast incremental path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A throttled stream-clear could drop the redraw entirely when a newline arrived in the last delta of a turn, leaving a ghost caret on screen. Add a one-shot deferred flush timer plus a flush on stream end. Fix a stale comment and a test that leaked global logger state in the vendored bubbletea fork.
Drive stream-clear rate limiting through the mockable clock and a shared streamClearThrottle constant so both the agentTextMsg and flush paths stay in sync. Add an explicit two-newline-within-window regression and detect ClearScreen by function pointer so tests do not evaluate Tick side effects.
Move the maintained bubbletea/v2 hard-scroll patch from third_party/bubbletea-v2-patched to patches/bubbletea-v2 and update the root go.mod replace. third_party/ is immutable vendored content per AGENTS.md; a maintained fork belongs outside that tree.
…ted go install path patches/bubbletea-v2 is its own Go module so `go test ./...` from the repo root never touched its 7 test files. Add a CI step (and a make test-patches target for local use) that runs the fork's own suite across the smoke matrix. The root go.mod's local `replace charm.land/bubbletea/v2 => ./patches/bubbletea-v2` also breaks `go install github.com/Gitlawb/zero/cmd/zero@<version>`, since a versioned install can't resolve a replace pointing at a filesystem path. Document that path as unsupported in README.md and docs/INSTALL.md and point at git clone + go build instead, per jatmn's review findings on PR Gitlawb#709.
d7b3525 to
8cb5469
Compare
|
Rebased this branch onto current upstream main (was 17 commits behind and had regressed package.json back to 0.4.0) and pushed a fixup commit for the two objective findings from jatmn's latest review:
I did not act on Vasanthdev2004's comment about the overall approach of vendoring a patched bubbletea fork versus splitting out the model.go fix or pursuing an upstream fix/runtime toggle. That's a product/architecture call for the maintainers and me to make, not something to resolve as part of these two fixes. Also, the CodeRabbit thread objecting to edits under |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/bubbletea-v2/cursed_renderer_test.go (1)
1-81: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRun the vendored bubbletea tests with
-racein CI. Thepatches/bubbletea-v2job in.github/workflows/ci.ymlusesgo test ./...today, so this concurrency regression can still pass even if it introduces a race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/bubbletea-v2/cursed_renderer_test.go` around lines 1 - 81, Update the `patches/bubbletea-v2` CI job in `.github/workflows/ci.yml` to run the vendored Bubble Tea tests with Go’s race detector by adding `-race` to its `go test ./...` command. Keep the existing package scope and job behavior unchanged.Source: Coding guidelines
🧹 Nitpick comments (3)
patches/bubbletea-v2/tea.go (1)
192-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate/orphaned
KeyboardEnhancementsdoc blocks.Lines 192-197 are dangling comments attached to nothing, and 199-205 repeats the 196-197 text. Collapse into a single doc block above the type declaration to avoid confusing future fork maintainers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/bubbletea-v2/tea.go` around lines 192 - 205, Remove the orphaned and duplicated KeyboardEnhancements documentation, leaving one consolidated doc block immediately above the KeyboardEnhancements type declaration that retains the relevant request and response behavior.patches/bubbletea-v2/tty.go (1)
62-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal var
termshadows the importedtermpackage.
term := p.environ.Getenv("TERM")shadows thegithub.com/charmbracelet/x/termimport for the rest ofinitInputReader. Harmless today, but risky if someone adds aterm.*call to this function later.♻️ Proposed rename
- term := p.environ.Getenv("TERM") + termEnv := p.environ.Getenv("TERM") ... - drv := uv.NewTerminalReader(p.cancelReader, term) + drv := uv.NewTerminalReader(p.cancelReader, termEnv)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/bubbletea-v2/tty.go` at line 62, Rename the local variable term in initInputReader to a non-conflicting name such as termName, and update its subsequent references while preserving the imported term package for any package-qualified calls.patches/bubbletea-v2/cursed_renderer_test.go (1)
52-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSleep-based readiness makes this regression test flaky.
time.Sleep(150 * time.Millisecond)before firing 100 concurrent mouse events is a best-effort guess thatp.Run()has reached a renderable state. Under CI load this can under-shoot (test doesn't actually exercise the race window it's meant to catch, giving false confidence) or just add flakiness. Since this specifically guards against a hard-to-repro race (charmbracelet/bubbletea#1690), a deterministic readiness signal (e.g., block on a first-render/ready notification fromProgramrather than a fixed delay) would make the regression test dependable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/bubbletea-v2/cursed_renderer_test.go` around lines 52 - 59, Replace the fixed time.Sleep readiness delay in the cursed renderer regression test with a deterministic signal that confirms p.Run has reached its first renderable state, such as a first-render or program-ready notification. Start firing the concurrent mouse events only after that signal, while preserving the existing runDone cleanup and regression scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 55-57: Update the “Test vendored bubbletea fork” workflow step to
run the fork’s Go test suite with the race detector by adding the -race option
to go test ./....
In `@patches/bubbletea-v2/tea.go`:
- Around line 1372-1390: Update Program.Println and Program.Printf to send their
printLineMessage values through the existing Send method instead of writing
directly to p.msgs. Preserve the current message formatting and ensure both
methods honor ctx.Done() so calls after shutdown cannot block.
In `@patches/bubbletea-v2/tty.go`:
- Around line 12-38: Update suspend and restoreTerminalState to surface errors
from RestoreTerminal and flush instead of discarding them; route failures
through the same p.errs mechanism used by checkResize, while preserving the
existing release and restore flow.
---
Outside diff comments:
In `@patches/bubbletea-v2/cursed_renderer_test.go`:
- Around line 1-81: Update the `patches/bubbletea-v2` CI job in
`.github/workflows/ci.yml` to run the vendored Bubble Tea tests with Go’s race
detector by adding `-race` to its `go test ./...` command. Keep the existing
package scope and job behavior unchanged.
---
Nitpick comments:
In `@patches/bubbletea-v2/cursed_renderer_test.go`:
- Around line 52-59: Replace the fixed time.Sleep readiness delay in the cursed
renderer regression test with a deterministic signal that confirms p.Run has
reached its first renderable state, such as a first-render or program-ready
notification. Start firing the concurrent mouse events only after that signal,
while preserving the existing runDone cleanup and regression scenario.
In `@patches/bubbletea-v2/tea.go`:
- Around line 192-205: Remove the orphaned and duplicated KeyboardEnhancements
documentation, leaving one consolidated doc block immediately above the
KeyboardEnhancements type declaration that retains the relevant request and
response behavior.
In `@patches/bubbletea-v2/tty.go`:
- Line 62: Rename the local variable term in initInputReader to a
non-conflicting name such as termName, and update its subsequent references
while preserving the imported term package for any package-qualified calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd7aae1f-6145-4a05-90d0-bd45e2cdcb53
⛔ Files ignored due to path filters (1)
patches/bubbletea-v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (62)
.github/workflows/ci.ymlMakefileREADME.mddocs/INSTALL.mdgo.modinternal/tui/model.gointernal/tui/stream_clear_test.gopatches/bubbletea-v2/LICENSEpatches/bubbletea-v2/clipboard.gopatches/bubbletea-v2/color.gopatches/bubbletea-v2/commands.gopatches/bubbletea-v2/commands_test.gopatches/bubbletea-v2/cursed_renderer.gopatches/bubbletea-v2/cursed_renderer_test.gopatches/bubbletea-v2/cursor.gopatches/bubbletea-v2/environ.gopatches/bubbletea-v2/exec.gopatches/bubbletea-v2/exec_test.gopatches/bubbletea-v2/focus.gopatches/bubbletea-v2/go.modpatches/bubbletea-v2/input.gopatches/bubbletea-v2/key.gopatches/bubbletea-v2/keyboard.gopatches/bubbletea-v2/logging.gopatches/bubbletea-v2/logging_test.gopatches/bubbletea-v2/mod.gopatches/bubbletea-v2/mouse.gopatches/bubbletea-v2/nil_renderer.gopatches/bubbletea-v2/options.gopatches/bubbletea-v2/options_test.gopatches/bubbletea-v2/paste.gopatches/bubbletea-v2/profile.gopatches/bubbletea-v2/raw.gopatches/bubbletea-v2/renderer.gopatches/bubbletea-v2/screen.gopatches/bubbletea-v2/screen_test.gopatches/bubbletea-v2/signals_unix.gopatches/bubbletea-v2/signals_windows.gopatches/bubbletea-v2/tea.gopatches/bubbletea-v2/tea_test.gopatches/bubbletea-v2/termcap.gopatches/bubbletea-v2/termios_bsd.gopatches/bubbletea-v2/termios_other.gopatches/bubbletea-v2/termios_unix.gopatches/bubbletea-v2/termios_windows.gopatches/bubbletea-v2/testdata/TestClearMsg/bg_fg_cur_color.goldenpatches/bubbletea-v2/testdata/TestClearMsg/clear_screen.goldenpatches/bubbletea-v2/testdata/TestClearMsg/read_set_clipboard.goldenpatches/bubbletea-v2/testdata/TestViewModel/altscreen.goldenpatches/bubbletea-v2/testdata/TestViewModel/altscreen_autoexit.goldenpatches/bubbletea-v2/testdata/TestViewModel/bg_set_color.goldenpatches/bubbletea-v2/testdata/TestViewModel/bp_stop_start.goldenpatches/bubbletea-v2/testdata/TestViewModel/cursor_hide.goldenpatches/bubbletea-v2/testdata/TestViewModel/cursor_hideshow.goldenpatches/bubbletea-v2/testdata/TestViewModel/kitty_stop_startreleases.goldenpatches/bubbletea-v2/testdata/TestViewModel/mouse_allmotion.goldenpatches/bubbletea-v2/testdata/TestViewModel/mouse_cellmotion.goldenpatches/bubbletea-v2/testdata/TestViewModel/mouse_disable.goldenpatches/bubbletea-v2/tty.gopatches/bubbletea-v2/tty_unix.gopatches/bubbletea-v2/tty_windows.gopatches/bubbletea-v2/xterm.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- internal/tui/stream_clear_test.go
- internal/tui/model.go
CodeRabbit review on this PR. The fork exists specifically to patch a concurrency bug (charmbracelet/bubbletea#1690), so its regression test needs the race detector to actually catch a reintroduced race instead of just passing on a benign-looking run.
|
Addressed CodeRabbit's review, mostly. Fixed: the vendored bubbletea fork's CI step now runs with -race. The fork exists specifically to patch a concurrency bug (charmbracelet/bubbletea#1690), so the race detector actually matters here, not just as general hygiene. Not fixing here: the rest of CodeRabbit's findings (duplicate doc comments in tea.go, a shadowed |
Dune was rendering as a light/sandy theme with a 55/55/55 grey surface, which did not match Claude Code's dark colorblind mode. Rebase the palette on a near-black surface (#0e0e10) with the brand-orange accent from openclaude's dark-daltonized theme, and move the warm grey to the user-prompt bubble only. Brighten the muted/faint/faintest gray ramp and darken selBg/addBg/delBg so all WCAG-AA and xterm-256 contrast invariants hold. Updates the stale test that still marked Dune as light. Refs Gitlawb#709
Maintainers asked not to ship an in-tree bubbletea replace: it breaks versioned go install and is a large maintenance surface for a narrow multipass/Windows Terminal scroll bug. Remove patches/bubbletea-v2 and the replace directive; keep the model.go full-redraw-on-streamed-newline workaround (ZERO_NO_STREAM_CLEAR to opt out). Restore normal install docs. Refs Gitlawb#709
|
Followed maintainer direction ( Removed
Kept
Versioned |
|
@jatmn following up on your latest review (the go.mod replace directive and the missing CI coverage for patches/bubbletea-v2). Both are moot as of 5d81745: I dropped the vendored bubbletea fork entirely per Vasanthdev2004's objection to keeping it. There is no more replace directive in go.mod, no patches/bubbletea-v2 module, and no separate CI step for it. Versioned go install works again through the normal path. What's left on the branch is just the model.go stream-clear workaround (ZERO_NO_STREAM_CLEAR to opt out) plus its existing tests. Still marked draft until you or another maintainer confirms that's sufficient for the multipass/Windows Terminal rendering issue without a hard-scroll disable. |
3 participants
Summary
Zero's TUI rendered corrupted/overlapping text when run inside a multipass VM and displayed through Windows Terminal on the host, worse on window resize. Two separate bugs, both in the bubbletea v2 renderer:
GOOS == "windows". That misses this case: the zero process itself runs on Linux inside the VM, but the terminal actually displaying the output is Windows Terminal reached over a remote shell. This vendors a patchedbubbletea/v2(local go.mod replace) with scroll optimization disabled unconditionally, since there's no reliable way to detect that remote hop from inside the VM.tea.ClearScreenspecifically when a streamed delta contains a newline, forcing a full redraw at exactly the moment the caret moves rather than relying on the diff.Test plan
go build ./...andGOOS=linux GOARCH=amd64 go build ./...go test ./...(full suite passes)Summary by CodeRabbit
Bug Fixes
Documentation
Tests