fix: command-block gutter hover repaint over-firing (issue #459 item 2) - #461
Conversation
The gutter hover-tint's repaint request fired the bare, zero-delay
ui.ctx().request_repaint() on every frame for the entire duration the
pointer rested over the gutter, not just on the enter/leave
transition, contradicting its own doc comment's stated intent ('force
one repaint on the frame the pointer leaves'). This bare zero-delay
request bypasses issue #436's ChromeMode::Replay idle-repaint
bookkeeping, forcing a full chrome (menu bar/tab bar) rebuild on
every frame while the mouse rests over the gutter strip -- confirmed
via real perf/flamegraph profiling in #459 (menu bar construction
showing up as a real idle-adjacent cost).
A naive hovered != was_hovering edge check is NOT sufficient here and
was caught by review before landing: Response::hovered() is keyed off
egui's interact_pos(), which lags PointerState::latest_pos() by one
frame specifically when the pointer leaves the OS window
(Event::PointerGone -- egui's own documented behavior). The naive
check would read stale-true on the exact frame CursorLeft is
processed, fail to fire, and since firing request_repaint() is what
schedules the next frame at all, nothing would then schedule the
frame where hovered() finally reads false -- risking an indefinitely
stuck hover tint when no other repaint source is active (hidden
cursor, non-blink style, echo-off).
Fix: fold in ctx().input(|i| i.pointer.latest_pos().is_some()) --
cleared same-frame on window-exit, unlike interact_pos() -- as an
override signal, so a pointer that has left the window is treated as
'not hovering' immediately rather than one frame late. Extracted the
decision into a pure gutter_hover_repaint_decision() function
(matching the existing bell_flash_outcome/cursor_blink_phase pattern
in this file) with 6 unit tests covering steady hover, both
transition directions, and the specific window-exit regression case.
Verification: cargo test --all, cargo clippy --all-targets
--all-features -- -D warnings, cargo machete all green. Adversarially
reviewed twice (first attempt's naive edge-check was caught as
BLOCKING before landing; the redesign's second review pass found no
blocking or should-fix items, independently re-verifying the egui
source claims and the outer event-loop scheduling guarantee).
Before/after flamegraph validation deferred -- a benchmark-sensitive
process was running on the test machine at implementation time.
See #459 for the full profiling investigation this follows up on.
📝 WalkthroughWalkthroughThe terminal widget now combines egui hover status with pointer presence inside the window when updating gutter hover state, cursor behavior, and repaint requests. A pure decision function and unit tests cover transitions and stale hover state. ChangesGutter hover repaint correction
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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}
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 `@freminal/src/gui/terminal/widget.rs`:
- Around line 1840-1842: Move the gutter hover cursor assignment from the
current effectively_hovered block into the final cursor-selection logic at the
end of show(), or incorporate cache.pointer_in_gutter_last_frame into that final
choice, ensuring gutter hover takes precedence over URL/OSC-22 and placeholder
hover cursor overrides.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40d5a28c-4aee-46a6-aa6f-041ee68ef781
📒 Files selected for processing (1)
freminal/src/gui/terminal/widget.rs
| if effectively_hovered { | ||
| ui.ctx().set_cursor_icon(egui::CursorIcon::PointingHand); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 3 'set_cursor_icon|cursor_icon\s*=' freminal/src/gui/terminal/widget.rsRepository: fredsystems/freminal
Length of output: 1583
🏁 Script executed:
#!/bin/bash
sed -n '1828,1848p' freminal/src/gui/terminal/widget.rs
printf '\n----\n'
sed -n '3380,3480p' freminal/src/gui/terminal/widget.rsRepository: fredsystems/freminal
Length of output: 5868
Move the gutter cursor override to the final cursor-selection block.
set_cursor_icon(PointingHand) here can be replaced later by the URL/OSC-22 cursor write and the placeholder hover override, so gutter hover won’t reliably win. Apply it at the end of show(), or fold cache.pointer_in_gutter_last_frame into the final cursor choice.
🤖 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 `@freminal/src/gui/terminal/widget.rs` around lines 1840 - 1842, Move the
gutter hover cursor assignment from the current effectively_hovered block into
the final cursor-selection logic at the end of show(), or incorporate
cache.pointer_in_gutter_last_frame into that final choice, ensuring gutter hover
takes precedence over URL/OSC-22 and placeholder hover cursor overrides.
There was a problem hiding this comment.
Confirmed valid, and confirmed pre-existing on main before this PR (the if hovered { set_cursor_icon(...) } line is unchanged in logic here, only the variable feeding it changed from hovered to effectively_hovered).
Verified the mechanism precisely: Context::set_cursor_icon (egui 0.35.0) is literally self.output_mut(|o| o.cursor_icon = cursor_icon) -- the exact same plain-write mechanism the later URL/OSC-22/placeholder-hover block (widget.rs:3323-3474) uses, which always executes one of its branches every frame and therefore always overwrites this earlier call. So the gutter's pointing-hand cursor currently has no visible effect, independent of anything in this PR.
Since it's pre-existing and unrelated to the repaint-firing fix this PR targets, filing it separately rather than scope-creeping this PR: #462
There was a problem hiding this comment.
@fredclausen, agreed. Thanks for verifying the exact egui write behavior and filing the focused follow-up in #462. This PR can remain scoped to the gutter repaint regression.
✏️ Learnings added
Learnt from: fredclausen
Repo: fredsystems/freminal PR: 461
File: freminal/src/gui/terminal/widget.rs:1840-1842
Timestamp: 2026-07-27T23:29:31.433Z
Learning: In `freminal/src/gui/terminal/widget.rs`, the command-block gutter’s early `Context::set_cursor_icon(CursorIcon::PointingHand)` is overwritten later in `FreminalTerminalWidget::show` by the URL/OSC-22/placeholder cursor-selection logic, because all paths perform plain writes to egui’s `output.cursor_icon`. This pre-existing cursor-priority bug is tracked separately in GitHub issue `#462` and is not part of PR `#461`’s gutter repaint fix.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Before/after validation (real workload, same scenario: mouse resting statically over the command-block gutter, 40s each)Captured with the same
Honesty note: the specific |
1 participant
Summary
Fixes the command-block gutter hover-tint's repaint request over-firing on every frame the pointer rests over the gutter, instead of only on the enter/leave transition. This is follow-up item 2 from #459's real-workload profiling investigation.
The bug
widget.rs:1780-1803'sif hovered || cache.pointer_in_gutter_last_frame { ui.ctx().request_repaint(); }fires the bare, zero-delayrequest_repaint()continuously for the entire duration the pointer rests over the gutter strip, contradicting its own doc comment's stated intent ("force one repaint on the frame the pointer leaves"). A bare zero-delayrequest_repaint()bypasses issue #436'sChromeMode::Replayidle-repaint bookkeeping (shortest_repaint_delay/pending_terminal_requested_delayonly track the app's ownrequest_repaint_aftercalls), forcingdecide_chrome_modetoFullon every such frame — which rebuilds the entire menu bar/tab bar. Confirmed via realperf/flamegraph profiling in #459:show_menu_bar_contents/MenuButton::uishowed up as a real on-CPU cost during an otherwise-idle capture.Why the obvious fix (a naive edge check) doesn't work
An earlier attempt at this PR used
if hovered != cache.pointer_in_gutter_last_frame. Adversarial review caught this as BLOCKING before it landed:Response::hovered()is keyed off egui'sinteract_pos(), which lagsPointerState::latest_pos()by exactly one frame specifically when the pointer leaves the OS window (Event::PointerGone— this is egui's own documented behavior,latest_posclears immediately,interact_posdoesn't until "next frame"). On the exact frameCursorLeftis processed,hovered()still reads stale-true, so the naive check evaluatestrue != true = falseand doesn't fire — and since firingrequest_repaint()is what schedules the next frame to run at all, nothing then schedules the frame wherehovered()finally readsfalse. Without another active repaint source (blink is suppressed by a hidden/DECTCEM cursor, a non-blink cursor style, or echo-off), this could leave the hover tint stuck indefinitely after the mouse leaves the window — a real regression, not a hypothetical.The actual fix
Fold in
ui.ctx().input(|i| i.pointer.latest_pos().is_some())— cleared same-frame on window-exit, unlikeinteract_pos()— as an override signal, so a pointer that has left the window is treated as "not hovering" immediately rather than one frame late:Extracted into a pure
gutter_hover_repaint_decision()function (matching this file's existingbell_flash_outcome/cursor_blink_phasepattern) with 6 unit tests, including the specific window-exit regression case that the naive fix got wrong.Validation
cargo test --all,cargo clippy --all-targets --all-features -- -D warnings,cargo machete— all green.interact_pos/latest_poslag, the entry-path non-lag, the outer event-loop repaint guarantee) and found no blocking or should-fix issues.perf --call-graph dwarf,65528methodology used for item 1 once the machine is free.Closes item 2 of #459.
Summary by CodeRabbit
Bug Fixes
Tests