fix(ui): hide add-note affordance while scrolling#328
Conversation
Greptile SummaryThis PR suppresses the hover-only
Confidence Score: 3/5Safe to review further; the core scroll-hiding behavior works correctly for the common single-row case, but multi-row hover during the suppression window desynchronizes the parent affordance from the still-visible badge. The new idle-timeout logic in the suppressed branch only creates a single timeout anchored to the first hovered row. If the mouse crosses additional rows within the 160 ms suppression window, that first timeout fires 2 s later and unconditionally calls src/ui/diff/PierreDiffView.tsx — the suppressed branch of Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DiffPane
participant DiffSection
participant PierreDiffView
User->>DiffPane: wheel scroll
DiffPane->>DiffPane: "suppressAddNoteHoverForMouseScroll()<br/>(sets addNoteHoverSuppressed=true, starts 160ms timer)"
DiffPane->>DiffSection: "hoverSuppressed=true"
DiffSection->>PierreDiffView: "hoverSuppressed=true"
User->>PierreDiffView: onHoverRow (during suppression)
PierreDiffView->>PierreDiffView: "activateHoveredRow()<br/>updates hoveredRowKey + affordance,<br/>starts idle timeout if no deadline set"
Note over DiffPane: 160ms elapses
DiffPane->>DiffPane: setAddNoteHoverSuppressed(false)
DiffPane->>DiffSection: "hoverSuppressed=false"
DiffSection->>PierreDiffView: "hoverSuppressed=false"
PierreDiffView->>PierreDiffView: "useEffect: restore hoveredRowKey<br/>if idle deadline still active"
Note over PierreDiffView: badge shows again
Note over PierreDiffView: 2000ms idle timeout fires
PierreDiffView->>PierreDiffView: clear hoveredRowKey + affordance
Note over PierreDiffView: badge hides
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/ui/diff/PierreDiffView.tsx:127-135
**Stale affordance after multi-row hover during suppression**
When the mouse moves over multiple rows during the 160 ms suppression window, only the first timeout is created (the `hoverIdleDeadlineRef` guard skips subsequent calls). That first timeout captures `rowKey` (row 1) in its closure. When it fires 2 s later, it skips `setHoveredRowKey` (since `current` has moved to row 2) and skips `lastHoveredRowKeyRef` cleanup — but still calls `onActiveAddNoteAffordanceChange?.(null)` unconditionally. The parent therefore loses the active affordance for row 2 while the visual badge still shows, so any keyboard/shortcut action that relies on the affordance would silently target nothing.
### Issue 2 of 2
src/ui/components/panes/DiffSection.tsx:148-150
`onMouseScroll` is not included in the custom memo comparator. Since `suppressAddNoteHoverForMouseScroll` is always a stable `useCallback([])` reference this is harmless today, but the omission is inconsistent with the rest of the comparator and could silently misbehave if the prop's stability assumption ever changes.
```suggestion
previous.hoverActive === next.hoverActive &&
previous.hoverSuppressed === next.hoverSuppressed &&
previous.onMouseScroll === next.onMouseScroll &&
previous.theme === next.theme &&
```
Reviews (1): Last reviewed commit: "fix(ui): hide add-note affordance while ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| hoverIdleTimeoutRef.current = setTimeout(() => { | ||
| setHoveredRowKey((current) => (current === rowKey ? null : current)); | ||
| if (lastHoveredRowKeyRef.current === rowKey) { | ||
| lastHoveredRowKeyRef.current = null; | ||
| } | ||
| hoverIdleDeadlineRef.current = null; | ||
| onActiveAddNoteAffordanceChange?.(null); | ||
| hoverIdleTimeoutRef.current = null; | ||
| }, ADD_NOTE_IDLE_HIDE_DELAY_MS); |
There was a problem hiding this comment.
When the mouse moves over multiple rows during the 160 ms suppression window, only the first timeout is created (the hoverIdleDeadlineRef guard skips subsequent calls). That first timeout captures rowKey (row 1) in its closure. When it fires 2 s later, it skips setHoveredRowKey (since current has moved to row 2) and skips lastHoveredRowKeyRef cleanup — but still calls onActiveAddNoteAffordanceChange?.(null) unconditionally. The parent therefore loses the active affordance for row 2 while the visual badge still shows, so any keyboard/shortcut action that relies on the affordance would silently target nothing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/diff/PierreDiffView.tsx
Line: 127-135
Comment:
**Stale affordance after multi-row hover during suppression**
When the mouse moves over multiple rows during the 160 ms suppression window, only the first timeout is created (the `hoverIdleDeadlineRef` guard skips subsequent calls). That first timeout captures `rowKey` (row 1) in its closure. When it fires 2 s later, it skips `setHoveredRowKey` (since `current` has moved to row 2) and skips `lastHoveredRowKeyRef` cleanup — but still calls `onActiveAddNoteAffordanceChange?.(null)` unconditionally. The parent therefore loses the active affordance for row 2 while the visual badge still shows, so any keyboard/shortcut action that relies on the affordance would silently target nothing.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. I changed the hover idle timeout to use one shared scheduler that can clear the current hovered row at the preserved idle deadline during scroll suppression, instead of clearing the parent affordance from a stale row closure.
Responded by pi using GPT-5.
| previous.hoverActive === next.hoverActive && | ||
| previous.hoverSuppressed === next.hoverSuppressed && | ||
| previous.theme === next.theme && |
There was a problem hiding this comment.
onMouseScroll is not included in the custom memo comparator. Since suppressAddNoteHoverForMouseScroll is always a stable useCallback([]) reference this is harmless today, but the omission is inconsistent with the rest of the comparator and could silently misbehave if the prop's stability assumption ever changes.
| previous.hoverActive === next.hoverActive && | |
| previous.hoverSuppressed === next.hoverSuppressed && | |
| previous.theme === next.theme && | |
| previous.hoverActive === next.hoverActive && | |
| previous.hoverSuppressed === next.hoverSuppressed && | |
| previous.onMouseScroll === next.onMouseScroll && | |
| previous.theme === next.theme && |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/components/panes/DiffSection.tsx
Line: 148-150
Comment:
`onMouseScroll` is not included in the custom memo comparator. Since `suppressAddNoteHoverForMouseScroll` is always a stable `useCallback([])` reference this is harmless today, but the omission is inconsistent with the rest of the comparator and could silently misbehave if the prop's stability assumption ever changes.
```suggestion
previous.hoverActive === next.hoverActive &&
previous.hoverSuppressed === next.hoverSuppressed &&
previous.onMouseScroll === next.onMouseScroll &&
previous.theme === next.theme &&
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Added onMouseScroll to the DiffSection memo comparator so the custom comparison covers the new callback prop too.
Responded by pi using GPT-5.
benvinegar
force-pushed
the
fix/hide-add-note-on-scroll
branch
from
28ee2d6 to
fd4df21
Compare
1 participant
Summary
[+]add-note badge during mouse wheel scrollingTests