Hover over comment highlights to open thread popovers#105
Hover over comment highlights to open thread popovers#105HamptonMakes wants to merge 2 commits intomainfrom
Conversation
Add hover-to-open behavior to inline comment <mark> elements: - 350ms hover-open delay (avoids triggering on casual reading) - 250ms hover-close delay on mouseleave (gives cursor time to enter the popover); cancelled if cursor enters either the popover or another mark for the same thread - Hovering a different mark while a hover popover is open switches immediately (no re-delay), like a tooltip group - Click-opened and j/k-nav-opened popovers are 'pinned' — they stay until explicitly dismissed (Esc, click outside, native light-dismiss) - Hover does nothing if any popover is already pinned (avoids flicker) - DOM-level :popover-open check so we coordinate correctly with comment_nav_controller (which doesn't go through this controller) Generated with Amp Amp-Thread-ID: https://ampcode.com/threads/T-019df459-b110-726a-97e2-ff15e2903435 Co-authored-by: Amp <amp@ampcode.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9550ff59f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const wasOpen = this._activePopover === popover | ||
| if (!wasOpen) { | ||
| popover.style.visibility = "hidden" | ||
| try { | ||
| popover.showPopover() | ||
| } catch { | ||
| // Already open — fall through to repositioning. |
There was a problem hiding this comment.
After a thread popover is light-dismissed (Esc/click outside), _activePopover is not cleared immediately, so this branch can treat the same popover as already open and skip showPopover(). In that state, clicking or hovering the same highlight will not reopen its thread until some other path (like scrolling) resets controller state, which breaks the primary interaction loop for a thread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 75012e3. Added a 'toggle' event listener (bound once per popover element via _coplanToggleBound) that clears _activePopover/_activeMark/_openMode whenever newState === 'closed'. So Esc, click-outside, and any other native light-dismiss path now reset state; re-clicking or hovering the same highlight reopens the thread.
There was a problem hiding this comment.
Pull request overview
Adds hover-to-open behavior for inline comment highlight popovers in Coplan’s plan doc review UI, aiming to make discussion threads discoverable without clicks while preserving existing click and keyboard navigation behaviors.
Changes:
- Introduces “hover” vs “pinned” open modes for thread popovers, with delayed open (350ms) and delayed close (250ms).
- Adds hover enter/leave handlers on anchor
<mark>elements and hover listeners on popovers to keep them open when moving the cursor into the popover. - Coordinates with existing j/k navigation by checking for any currently-open
.thread-popover:popover-openin the DOM.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Re-check guards in case state changed during the delay. Don't open | ||
| // if any other popover is currently pinned (click or j/k). | ||
| if (this._openMode === "pinned" && this._activePopover) return | ||
| const ourPopover = document.getElementById(`${mark.dataset.threadId}_popover`) | ||
| const openInDom = document.querySelector(".thread-popover:popover-open") | ||
| if (openInDom && openInDom !== ourPopover) return | ||
| this._showThreadPopoverFor(mark, "hover") |
There was a problem hiding this comment.
Same root cause as the Codex P1 above — fixed in 75012e3 the way you suggested: a 'toggle' event listener now clears _openMode/_activePopover whenever the popover transitions to closed. So light-dismiss of a pinned popover no longer permanently blocks future hover-opens. Listener is bound once per popover (_coplanToggleBound flag) so it survives re-positioning and broadcast updates.
| _isAnyPopoverOpen() { | ||
| try { | ||
| return !!document.querySelector(".thread-popover:popover-open") | ||
| } catch { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fixed in 75012e3. Replaced _isAnyPopoverOpen() with a _findOpenPopover() helper that mirrors comment_nav_controller#findOpenPopover — same try/catch around :popover-open with a checkVisibility() fallback. The inline document.querySelector in _scheduleHoverOpen now goes through that helper too, so both checks degrade gracefully on browsers without :popover-open support.
| mark.dataset.threadId = threadId | ||
| mark.style.cursor = "pointer" | ||
| mark.addEventListener("click", (e) => this.openThreadPopover(e)) | ||
| mark.addEventListener("mouseenter", () => this.handleMarkHoverEnter(mark)) | ||
| mark.addEventListener("mouseleave", () => this.handleMarkHoverLeave(mark)) |
There was a problem hiding this comment.
Skipping the system spec for this PR — Hampton (the author) and I agreed the timing/feel parts are best validated by use, and the bug-class issues you flagged above are now covered by the controller's own state machine. We'll add coverage if regressions show up. Tracked separately if it becomes recurrent.
Codex (P1) and Copilot both flagged: when a popover is closed by native light-dismiss (Esc, click outside, another popover='auto' opening), our controller never learned about it, so _activePopover/_openMode stayed populated. The 'pinned popover already open' guard then blocked hover-open forever, and clicking the same mark again would skip showPopover() because _activePopover === popover (treated as 'still open'). Fix: listen to the popover 'toggle' event and clear all tracked state when newState === 'closed'. The listener is bound once per popover element via a _coplanToggleBound flag. Also addressed Copilot's third comment: _isAnyPopoverOpen() now uses the same fallback as comment_nav_controller#findOpenPopover (handles browsers where the :popover-open selector throws), and the inline querySelector in _scheduleHoverOpen now goes through the same helper. Removed the now-redundant _openMode === 'pinned' guard there since the DOM check is the source of truth and toggle clears stale state. Skipping the system spec suggestion for now per user direction. Generated with Amp Amp-Thread-ID: https://ampcode.com/threads/T-019df459-b110-726a-97e2-ff15e2903435 Co-authored-by: Amp <amp@ampcode.com>
Why
Today you have to click an inline comment highlight (or its margin dot) to read the thread. Hovering should be enough — opening on hover makes the discussion feel ambient instead of hidden, especially when scanning a doc.
What
<mark>→ popover opens after 350 ms:popover-opencheck coordinates with comment_nav_controller.js)Risk Assessment
Low — JS-only change to a single Stimulus controller, no model/route/CSS changes, no behavior removed. Click and j/k navigation paths are preserved (verified by walking the code, not just the diff).
References
Generated with Amp