fix(ui): only show add-note affordance after pointer movement#331
Conversation
benvinegar
force-pushed
the
fix/add-note-pointer-move-only
branch
from
47694bc to
a80ac1a
Compare
Greptile SummaryReplaces the timeout-based hover-suppression mechanism with a signal counter (
Confidence Score: 4/5The change is safe to merge. The core fix—switching from onMouseOver to onMouseMove and clearing state via a counter signal rather than a timed suppress/resume—is correctly implemented and well-tested. The signal-counter approach cleanly replaces the old timeout mechanism. The one rough edge is in the idle-timeout callback inside activateHoveredRow: onActiveAddNoteAffordanceChange?.(null) is called regardless of whether the state updater actually changed hoveredRowKey, creating a subtle asymmetry. It works today because clearHoverIdleTimeout is always invoked before hoveredRowKey can change, but the assumption is implicit. The test coverage for the new behavior is solid. src/ui/diff/PierreDiffView.tsx — the idle-timeout callback has an unconditional onActiveAddNoteAffordanceChange call that is worth tightening up. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DiffPane
participant DiffSection
participant PierreDiffView
participant DiffRowView
Note over User,DiffRowView: Scroll clears affordance
User->>DiffPane: wheel / keyboard / programmatic scroll
DiffPane->>DiffPane: "clearAddNoteHoverForScroll()<br/>setAddNoteHoverClearSignal(n+1)<br/>onActiveAddNoteAffordanceChange(null)"
DiffPane->>DiffSection: "hoverClearSignal=n+1"
DiffSection->>PierreDiffView: "hoverClearSignal=n+1"
PierreDiffView->>PierreDiffView: "useEffect detects signal change<br/>clearHoveredRow() / setHoveredRowKey(null)"
Note over User,DiffRowView: Only pointer movement re-activates
User->>DiffRowView: onMouseMove
DiffRowView->>PierreDiffView: onHoverRow()
PierreDiffView->>PierreDiffView: "activateHoveredRow(rowKey)<br/>setHoveredRowKey(rowKey) / schedule idle timer"
PierreDiffView->>DiffPane: onActiveAddNoteAffordanceChange(affordance)
DiffRowView->>DiffRowView: "showAddNoteBadge=true renders [+]"
Note over User,DiffRowView: Idle timeout hides affordance
PierreDiffView->>PierreDiffView: "idle timer fires<br/>setHoveredRowKey(null) if current===rowKey<br/>onActiveAddNoteAffordanceChange(null)"
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/ui/diff/PierreDiffView.tsx:119-123
The idle-timeout callback calls `onActiveAddNoteAffordanceChange?.(null)` unconditionally, but only clears `hoveredRowKey` when `current === rowKey`. If somehow `current !== rowKey` when the timer fires, the parent's affordance state would be cleared while the child still has a different row highlighted. The invariant that `clearHoverIdleTimeout` is always called before `hoveredRowKey` changes makes this safe today, but the asymmetry is easy to misread. Guarding the callback with the same condition makes the code self-documenting and future-proof.
```suggestion
hoverIdleTimeoutRef.current = setTimeout(() => {
setHoveredRowKey((current) => {
if (current === rowKey) {
onActiveAddNoteAffordanceChange?.(null);
return null;
}
return current;
});
hoverIdleTimeoutRef.current = null;
}, ADD_NOTE_IDLE_HIDE_DELAY_MS);
```
Reviews (1): Last reviewed commit: "fix(ui): only show add-note affordance a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| hoverIdleTimeoutRef.current = setTimeout(() => { | ||
| if (hoverIdleDeadlineRef.current !== deadline) { | ||
| return; | ||
| } | ||
|
|
||
| if (clearCurrentRow || lastHoveredRowKeyRef.current === rowKey) { | ||
| lastHoveredRowKeyRef.current = null; | ||
| setHoveredRowKey(null); | ||
| onActiveAddNoteAffordanceChange?.(null); | ||
| } | ||
| hoverIdleDeadlineRef.current = null; | ||
| setHoveredRowKey((current) => (current === rowKey ? null : current)); | ||
| onActiveAddNoteAffordanceChange?.(null); | ||
| hoverIdleTimeoutRef.current = null; | ||
| }, ADD_NOTE_IDLE_HIDE_DELAY_MS); |
There was a problem hiding this comment.
onActiveAddNoteAffordanceChange?.(null) unconditionally, but only clears hoveredRowKey when current === rowKey. If somehow current !== rowKey when the timer fires, the parent's affordance state would be cleared while the child still has a different row highlighted. The invariant that clearHoverIdleTimeout is always called before hoveredRowKey changes makes this safe today, but the asymmetry is easy to misread. Guarding the callback with the same condition makes the code self-documenting and future-proof.
| hoverIdleTimeoutRef.current = setTimeout(() => { | |
| if (hoverIdleDeadlineRef.current !== deadline) { | |
| return; | |
| } | |
| if (clearCurrentRow || lastHoveredRowKeyRef.current === rowKey) { | |
| lastHoveredRowKeyRef.current = null; | |
| setHoveredRowKey(null); | |
| onActiveAddNoteAffordanceChange?.(null); | |
| } | |
| hoverIdleDeadlineRef.current = null; | |
| setHoveredRowKey((current) => (current === rowKey ? null : current)); | |
| onActiveAddNoteAffordanceChange?.(null); | |
| hoverIdleTimeoutRef.current = null; | |
| }, ADD_NOTE_IDLE_HIDE_DELAY_MS); | |
| hoverIdleTimeoutRef.current = setTimeout(() => { | |
| setHoveredRowKey((current) => { | |
| if (current === rowKey) { | |
| onActiveAddNoteAffordanceChange?.(null); | |
| return null; | |
| } | |
| return current; | |
| }); | |
| hoverIdleTimeoutRef.current = null; | |
| }, ADD_NOTE_IDLE_HIDE_DELAY_MS); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/PierreDiffView.tsx
Line: 119-123
Comment:
The idle-timeout callback calls `onActiveAddNoteAffordanceChange?.(null)` unconditionally, but only clears `hoveredRowKey` when `current === rowKey`. If somehow `current !== rowKey` when the timer fires, the parent's affordance state would be cleared while the child still has a different row highlighted. The invariant that `clearHoverIdleTimeout` is always called before `hoveredRowKey` changes makes this safe today, but the asymmetry is easy to misread. Guarding the callback with the same condition makes the code self-documenting and future-proof.
```suggestion
hoverIdleTimeoutRef.current = setTimeout(() => {
setHoveredRowKey((current) => {
if (current === rowKey) {
onActiveAddNoteAffordanceChange?.(null);
return null;
}
return current;
});
hoverIdleTimeoutRef.current = null;
}, ADD_NOTE_IDLE_HIDE_DELAY_MS);
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
[+]affordance whenever review content scrolls under the pointeronMouseOverfrom activating[+]; onlyonMouseMovenow activates it[+]hidden after wheel, keyboard, or programmatic scroll until the pointer actually moves againTests