fix(tui): fix rendering corruption over multipass + Windows Terminal#709
fix(tui): fix rendering corruption over multipass + Windows Terminal#709euxaristia wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 48 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 ignored due to path filters (1)
📒 Files selected for processing (58)
WalkthroughBubble Tea v2 is redirected to a local patched implementation containing terminal runtime, rendering, input, command, lifecycle, and message APIs. TUI streaming now throttles newline-triggered full redraws, defers pending clears, and supports an environment-controlled opt-out. ChangesBubble Tea 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 |
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
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
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
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
| @@ -0,0 +1 @@ | |||
| [?25l[?2004h[>4;2m[=1;1u[Jsuccess[>4m[=0;1u[J[?25h[?2004l[?2026$p[?2027$p]10;?]11;?]12;? | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not modify files under third_party/.
This golden fixture is changed in a vendored path, contrary to the repository rule. Move the maintained fixture/test outside third_party/, or obtain and document an explicit policy exception before merging.
As per coding guidelines: third_party/**: Never edit files under third_party/.
🤖 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/testdata/TestClearMsg/bg_fg_cur_color.golden`
at 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.
Source: Coding guidelines
There was a problem hiding this comment.
Addressed in d7b3525.
The maintained bubbletea/v2 fork no longer lives under third_party/. It was moved to patches/bubbletea-v2, and go.mod now has:
replace charm.land/bubbletea/v2 => ./patches/bubbletea-v2
Reason: AGENTS.md marks third_party/ as immutable vendored content, so a maintained fork with behavior patches does not belong there. patches/ is repository-owned source with an explicit replace, which keeps the hard-scroll disable (SetScrollOptim(false)) without touching third_party/.
The fork remains necessary: upstream only gates hard scroll on GOOS=="windows", which misses a Linux zero process whose display is Windows Terminal over a remote shell (the multipass case this PR targets). There is no reliable remote-hop detection from inside the VM.
Remaining release concern (separate from this thread): a root-module replace still breaks go install github.com/Gitlawb/zero/cmd/zero@version. Publishing a versioned remote fork, or another install-safe approach, still needs a maintainer decision before this can leave draft for a release cut. Source-checkout builds are fine.
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
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.
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
|
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
ZERO_NO_STREAM_CLEAR).