fix(tui): fall back to CellMotion where AllMotion is unreliable - #872
fix(tui): fall back to CellMotion where AllMotion is unreliable#872Vasanthdev2004 wants to merge 2 commits into
Conversation
AllMotion (1003) buys exactly one thing over CellMotion (1002): hover highlighting. Wheel, click and drag all arrive under either. But a terminal that does not implement 1003 does not degrade to 1002, it sends nothing at all, and because the TUI holds the alternate screen the terminal's own scrollback and wheel are gone too. The user is left unable to scroll by any means, and a long answer then reads as a hang rather than as a missing highlight. That is how #870 arrived: a completed turn with "59 more" lines below the fold, reported as a freeze. The existing code already conceded 1003 is fragile by carving out PRoot. The legacy Windows console is the same case and was not covered, so a bare zero.exe outside Windows Terminal asked for a mode its terminal does not answer. Move the decision into mouseModeFor, which trusts terminals that identify themselves (WT_SESSION for Windows Terminal, TERM_PROGRAM for hosts like VS Code and mintty) and assumes an unidentified Windows terminal is the legacy console. Guessing wrong that way costs a hover highlight; guessing wrong the other way costs every mouse event. ZERO_MOUSE_MODE=all|cell|off overrides it, so a user on a terminal we guessed wrong about can fix it without waiting for a release. An unrecognised value is ignored rather than treated as "off", so a typo cannot silently kill the mouse. Refs #870.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe TUI now centralizes mouse-mode selection. It supports environment overrides and selects compatible modes for PRoot, Windows terminals, and other platforms. Tests cover fallback behavior, terminal detection, override precedence, and invalid overrides. ChangesMouse mode selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@anandh8x @gnanam1990 small one, but there is a judgement call in it rather than a correctness question, so I would like a second opinion. The mechanical part is straightforward. AllMotion (1003) buys only hover highlighting over CellMotion, and a terminal that does not implement it sends nothing at all rather than degrading, which combined with the alt screen leaves the user no way to scroll. That is #870: a completed turn with 59 lines below the fold, reported as a freeze. The judgement is this. I treat an unidentified Windows terminal as the legacy console and drop it to CellMotion, so a Windows user outside Windows Terminal or VS Code loses hover highlighting. That is deliberate, since the opposite mistake costs every mouse event rather than one highlight, but it is a real behaviour change on Windows and I would rather two of you agreed with the trade than just me. Worth knowing: the reporter has not told us which terminal they use yet, so the root cause is inferred, not confirmed. One unrelated thing I tripped over: |
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
The mechanical change looks sound, and I agree with the judgement call in your comment. A few follow-ups are worth tracking, but none of them overturn the trade you are asking reviewers to sign off on.
Findings
-
[P3] Inherited
WT_SESSION/TERM_PROGRAMcan still defeat the legacy-console fallback
internal/tui/mouse.go:57-59,internal/tui/mouse.go:42-44
You treat unidentified Windows consoles (noWT_SESSION, noTERM_PROGRAM) as legacy conhost and fall back to CellMotion. That trade is separate from this edge case: on Windows, child processes inherit the parent environment, so azero.exesession attached to legacy conhost can still carryWT_SESSIONfrom Windows Terminal orTERM_PROGRAMfrom Git Bash / VS Code even though the active console host is not that parent emulator. In that casewindowsTerminalReportsAllMotionreturns true and AllMotion is requested on a host that may drop every mouse event — the same #870 failure class. This is outside the judgement call you raised, and it is probably not the reporter's path if they double-clickedzero.exeor ran from a plaincmdwindow with a clean env. I would still add a regression test (goos=windows, inheritedWT_SESSIONorTERM_PROGRAM, want CellMotion unless you intentionally trust inheritance). If it shows up in the #870 thread,ZERO_MOUSE_MODE=cellis already the right diagnostic. Detecting the actual console host would be nicer than env markers alone, but I would not block merge on it given your diagnostic workflow. -
[P3] Ctrl+E "re-enabled" notice ignores a
ZERO_MOUSE_MODE=offoverride
internal/tui/model.go:1446-1451,internal/tui/mouse.go:36-37,internal/tui/model.go:2791-2799
WithZERO_MOUSE_MODE=offornone,mouseModeForalways returnsMouseModeNone, but pressing Ctrl+E again still posts "Mouse interaction re-enabled." even thoughView()keeps emittingMouseModeNone. Minor UX mismatch for anyone using the override deliberately; fine to fix in a follow-up.
On the trade you asked about
You framed the real question correctly: unidentified Windows should default to CellMotion because guessing the other way costs every mouse event, not just hover. I agree with that trade for the likely #870 paths (double-click / plain cmd.exe). Losing hover on capable third-party Windows terminals that do not set WT_SESSION or TERM_PROGRAM (some Alacritty / WezTerm installs) is an acceptable cost, and ZERO_MOUSE_MODE=all is a reasonable way to confirm that with the reporter before hardening detection further.
Root cause for #870 is still inferred, as you noted. Shipping the safe default now rather than waiting for the reporter's terminal answer is reasonable; the override gives you a one-line diagnostic either way.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Checked out and probed rather than read. The reasoning that makes this correct is the asymmetry, and the PR states it plainly: AllMotion buys one thing (hover highlighting), while a terminal that does not implement 1003 sends nothing — and because the app holds the alternate screen, the terminal's own wheel and scrollback are gone too. So the failure is not "no highlight", it is "no way to scroll at all", which reads as a hang. Trading a highlight for that is obviously right.
Additivity verified directly. I asserted mouseModeFor on darwin/linux/freebsd with no env and no PRoot: all still AllMotion. So every platform that worked before is untouched, and the change is confined to Windows-without-a-known-terminal and PRoot — the two cases that were already broken.
The risk direction is the safe one. An unidentified Windows terminal is assumed to be the legacy console host, which is exactly what a user gets double-clicking zero.exe. Guessing wrong that way costs a highlight; guessing wrong the other way costs every mouse event. Correct default for a case you cannot detect.
Edge cases I probed, all correct:
- a typo in
ZERO_MOUSE_MODE(celll,allmotion,true,1,yes) is ignored, not read as "off" — a mistyped variable cannot silently kill the mouse, which matters for a knob whose failure mode is invisible - the override wins even where the heuristic would fall back (
windows+ PRoot +all→ AllMotion), so a user on a terminal you guessed wrong about can recover without editing config - a blank-but-present
WT_SESSION(" ") does not count as a real terminal — theTrimSpaceis load-bearing, not decoration
No per-render cost: isRunningUnderPRoot is a sync.OnceValue, so hoisting it into the argument list does not add a /proc read per frame, and os.Getenv per frame is negligible.
The six-case table (linux, macOS, PRoot, legacy console, Windows Terminal, VS Code) covers the matrix, and the env-override table covers casing and whitespace. go test ./internal/tui/ green on the branch.
Non-blocking: ZERO_MOUSE_MODE is a real escape hatch for a failure the user cannot otherwise diagnose — worth a line in the docs or zero doctor output, so someone with a dead mouse can find it without reading source.
WT_SESSION and TERM_PROGRAM are inherited, so they answer for whichever process set them rather than for the console host attached now. A shell started from Windows Terminal or Git Bash that later runs zero.exe against the legacy console still carries them, and mouseModeFor then asks for AllMotion on a host that may drop every mouse event: the #870 failure class through a narrower door. Not fixed, and deliberately so. Telling an inherited variable from a live one needs the console host rather than the environment, and erring the other way costs every Windows Terminal user their hover highlighting for a case that takes an unusual launch path to reach. So the limit is written down in the code and pinned by a test that also asserts ZERO_MOUSE_MODE=cell still overrides it, since that override is the only recourse an affected user has. If real host detection lands later the test fails by design, and its message says to delete it. Raised by @jatmn on #872.
8adc625
|
@jatmn 8adc625. You are right about the mechanism, and I have not fixed it, deliberately. Here is the reasoning so you can push back if you disagree. Telling an inherited So the limit is now written down where the decision is made, and If you would rather I go the other way and default Windows to CellMotion outright, accepting the lost hover everywhere, say so and I will. It is a smaller diff than host detection and it has no false-positive surface at all. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-approving after 8adc625d. My earlier approval was auto-dismissed by the push.
Verified the new commit changes no behavior: git show 8adc625d -- internal/tui/mouse.go is comment-only once blank and // lines are filtered out. The +13 is the KNOWN LIMIT block; the executable part is the test.
The limit itself is worth calling out for anyone reading this later, because the commit is choosing to leave a narrow instance of the bug the PR fixes. WT_SESSION/TERM_PROGRAM are inherited, so they answer for the process that set them, not for the console host attached right now — a shell launched from Windows Terminal that later runs zero.exe against the legacy console still carries them, and mouseModeFor asks for AllMotion on a host that may drop every event. That is #870 arriving by a narrower door, and the comment says so plainly rather than hoping nobody notices.
Leaving it is the right call on the trade the comment describes: deciding it properly means asking the console host instead of the environment, and erring the other way costs every Windows Terminal user their hover highlighting to cover a case that needs an unusual launch path to reach. ZERO_MOUSE_MODE=cell is a real recourse in the meantime.
TestInheritedWindowsTerminalEnvStillAsksForAllMotion passes and pins it. Worth being explicit that this test asserts behavior we know is imperfect — if the host-query fix ever lands, this test is the thing to change, not a regression to debug. The comment naming the test makes that discoverable, which is exactly what a pinned limit needs.
go test ./internal/tui/ green. Approving.
3 participants
Fixes the scroll failure behind #870.
The problem
AllMotion(1003) buys exactly one thing overCellMotion(1002): hover highlighting. Wheel, click and drag all arrive under either. The asymmetry is in the failure: a terminal that does not implement 1003 does not fall back to 1002, it sends nothing at all. Since the TUI holds the alternate screen, the terminal's own scrollback and wheel are gone too, so the user cannot scroll by any means.That is what #870 reported as a freeze. The screenshot shows a completed turn (
Thought for 1.9s, answer rendered, composer back to its idle placeholder) with↓ 59 more · PgDnin the footer. Nothing was hung. The output ran past the fold and no scroll input worked.The code already conceded 1003 is fragile by carving out PRoot. The legacy Windows console is the same case and was not covered, so
zero.exerun outside Windows Terminal asks for a mode its terminal never answers.The change
mouseModeFornow makes the decision. It trusts terminals that identify themselves,WT_SESSIONfor Windows Terminal andTERM_PROGRAMfor hosts like VS Code and mintty, and assumes an unidentified Windows terminal is the legacy console. Guessing wrong in that direction costs a hover highlight. Guessing wrong the other way costs every mouse event.ZERO_MOUSE_MODE=all|cell|offoverrides it, so anyone on a terminal we guessed wrong about can fix it without waiting for a release, and I can ask a reporter to test one specific mode. An unrecognised value is ignored rather than treated as "off", so a typo cannot silently kill the mouse.Tests
mouse_mode_test.gocovers linux, macOS, PRoot, legacy Windows console, Windows Terminal, VS Code on Windows, all three override values with case and whitespace variants, and an unknown override. Written against the old behaviour first: onlylegacy_Windows_consoleand the override cases failed, so they pin the gap rather than describe the fix.go build ./...,GOOS=windows go build ./...,go vetandgofmtare clean.Notes
The root cause is not confirmed with the reporter yet, since I have not heard back on which terminal they use. I would rather ship the safe default than wait, because the override gives us a one-line diagnostic to hand them either way.
Unrelated,
TestAltScreenTranscriptScrollKeepsFooterFixedcurrently fails on pristinemainat edf660a on Windows. Not touched by this PR, but somebody should look.Summary by CodeRabbit
ZERO_MOUSE_MODEsetting.