feat(desktop): persistent v2 sidebar hover card#3813
Conversation
Replace the per-row Radix HoverCard wrappers with a single sidebar-level Popover anchored via virtualRef to whichever workspace row is hovered. Moving between rows now slides the panel rather than unmounting and remounting, and the content updates in place. The slide is gated by a data-positioned flag so the initial open doesn't animate from Radix's off-screen measuring transform. - Introduces DashboardSidebarHoverProvider (open/close timers, anchor + payload state, context-menu suppression, live workspace sync via syncIfHovered). - Introduces DashboardSidebarHoverCardOverlay with the single Popover + PopoverAnchor and lifts useDiffStats(hoveredId) up so the row's per- item hook can stay focused on its own rendering. - Strips the HoverCard layer out of DashboardSidebarWorkspaceContextMenu and lets the context menu report its open state through the provider. - Cloud workspaces get hover cards too; only the PR-refresh callback remains gated to local-device hosts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe sidebar's hover card system is refactored from decentralized per-item management to centralized provider-based management. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Move the popover-wrapper transition rule out of globals.css into a sibling CSS file imported by DashboardSidebarHoverCardOverlay so the style stays scoped to the component that needs it.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx:97">
P2: `requestClose` clears pending opens without checking which row owns the timer, so rapid cross-row hover can cancel the next row’s scheduled open.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const requestClose = useCallback<HoverContextValue["requestClose"]>( | ||
| (id) => { | ||
| if (openTimerRef.current && stateRef.current.hoveredId === null) { |
There was a problem hiding this comment.
P2: requestClose clears pending opens without checking which row owns the timer, so rapid cross-row hover can cancel the next row’s scheduled open.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx, line 97:
<comment>`requestClose` clears pending opens without checking which row owns the timer, so rapid cross-row hover can cancel the next row’s scheduled open.</comment>
<file context>
@@ -0,0 +1,185 @@
+
+ const requestClose = useCallback<HoverContextValue["requestClose"]>(
+ (id) => {
+ if (openTimerRef.current && stateRef.current.hoveredId === null) {
+ // Pending open for this id — cancel it.
+ clearOpenTimer();
</file context>
Greptile SummaryThis PR replaces per-row Radix Confidence Score: 5/5Safe to merge; the one finding is a minor edge-case visual glitch on unmount-while-hovered that is self-correcting on mouse move. All remaining findings are P2. The hover state machine, timer logic, virtualRef approach, and CSS :has() transition guard are all well-implemented. The only issue (no unmount cleanup) produces a transient visual artifact in a rare scenario and doesn't risk data loss or crashes. DashboardSidebarWorkspaceItem.tsx — missing useEffect cleanup to call requestClose on unmount.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx | New context provider managing hover state, open/close timers, and context-menu suppression. Timer logic is correct: immediate switch when a card is already open, 400 ms delay for first open, 100 ms close grace period. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsx | New overlay rendering a single shared Popover with a virtualRef anchor; double-rAF guards the initial positioning jump from the CSS transition cleanly. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx | Correctly replaces per-row HoverCard wrappers with context hooks; missing unmount cleanup means hover state leaks when a workspace item disappears while its card is open. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx | Simplified by removing HoverCard wrappers; now delegates context-menu-open state to the provider via setContextMenuOpen — correct and cleaner. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx | Wraps existing tree with DashboardSidebarHoverProvider and DashboardSidebarHoverCardOverlay; structure is straightforward and correct. |
| apps/desktop/src/renderer/globals.css | Adds :has() selector targeting the Radix popper wrapper only after data-dashboard-sidebar-hover-card="ready" is set, correctly gating the slide transition to avoid animating the initial off-screen measurement jump. |
Sequence Diagram
sequenceDiagram
participant Row as WorkspaceItem
participant Prov as HoverProvider
participant Overlay as HoverCardOverlay
participant Radix as Radix Popover
Row->>Prov: requestOpen(id, anchorEl, payload)
note over Prov: hoveredId=null, start 400ms timer
Prov-->>Prov: timer fires, setState hoveredId+anchorElement+payload
Prov-->>Overlay: context update open=true
Overlay->>Radix: open=true, virtualRef=anchorElement
Radix-->>Overlay: positioned at anchor
Overlay-->>Overlay: double rAF fires, data-attr=ready, CSS slide enabled
Row->>Prov: requestClose(id)
note over Prov: start 100ms close timer
Overlay->>Prov: onPointerEnter, cancelClose
note over Prov: close timer cleared
Row->>Prov: requestOpen(id2, anchorEl2, payload2)
note over Prov: hoveredId!=null, immediate setState
Prov-->>Overlay: anchorElement updated
Radix-->>Overlay: slides to new row position
Row->>Prov: right-click, setContextMenuOpen true
Prov-->>Overlay: contextMenuOpen=true, open=false
Radix-->>Overlay: popover hidden
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
Line: 114-121
Comment:
**No unmount cleanup for hover state**
If a workspace item unmounts while its hover card is open (e.g., the workspace is closed/deleted remotely while the card is showing), `hoveredId` stays set in the context and the popover remains visible with a detached anchor. `getBoundingClientRect()` on a detached element returns all-zeros, so the card jumps to the top-left of the screen and stays there until the user moves their mouse.
Adding a cleanup effect that calls `hoverRequestClose(id)` on unmount prevents this:
```ts
useEffect(() => {
return () => {
hoverRequestClose(id);
};
}, [hoverRequestClose, id]);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "refactor(desktop): co-locate hover card ..." | Re-trigger Greptile
| const isHovered = hoverHoveredId === id; | ||
| useEffect(() => { | ||
| if (isHovered && hostType === "local-device") onHoverCardOpen?.(); | ||
| }, [isHovered, hostType, onHoverCardOpen]); | ||
| useEffect(() => { | ||
| if (!isHovered) return; | ||
| hoverSyncIfHovered(id, hoverPayload); | ||
| }, [isHovered, hoverSyncIfHovered, id, hoverPayload]); |
There was a problem hiding this comment.
No unmount cleanup for hover state
If a workspace item unmounts while its hover card is open (e.g., the workspace is closed/deleted remotely while the card is showing), hoveredId stays set in the context and the popover remains visible with a detached anchor. getBoundingClientRect() on a detached element returns all-zeros, so the card jumps to the top-left of the screen and stays there until the user moves their mouse.
Adding a cleanup effect that calls hoverRequestClose(id) on unmount prevents this:
useEffect(() => {
return () => {
hoverRequestClose(id);
};
}, [hoverRequestClose, id]);Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
Line: 114-121
Comment:
**No unmount cleanup for hover state**
If a workspace item unmounts while its hover card is open (e.g., the workspace is closed/deleted remotely while the card is showing), `hoveredId` stays set in the context and the popover remains visible with a detached anchor. `getBoundingClientRect()` on a detached element returns all-zeros, so the card jumps to the top-left of the screen and stays there until the user moves their mouse.
Adding a cleanup effect that calls `hoverRequestClose(id)` on unmount prevents this:
```ts
useEffect(() => {
return () => {
hoverRequestClose(id);
};
}, [hoverRequestClose, id]);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx`:
- Around line 58-60: The stateRef.current is being updated inside useEffect
which runs after paint and can be one event late for rapid pointer transitions;
change the sync to run synchronously by replacing useEffect with useLayoutEffect
(or alternatively assign stateRef.current = state immediately wherever state is
set) so that stateRef reflects the latest hoveredId during event handlers like
requestOpen and requestClose in DashboardSidebarHoverProvider.
- Around line 97-100: The requestClose logic currently clears any pending open
via openTimerRef when stateRef.current.hoveredId === null, which can cancel a
pending open for a different row; fix by scoping pending-open cancellation to
the specific row id: store the id associated with a pending open (e.g., change
openTimerRef to hold {timer, id} or add pendingOpenIdRef) when scheduling an
open in the function that sets the timer, and update requestClose (and
clearOpenTimer) to only clear the timer if the stored pending id matches the id
being closed (or matches stateRef.current.hoveredId) so you don't cancel timers
for other rows; update references in DashboardSidebarHoverProvider around
requestClose, clearOpenTimer, openTimerRef and any timer-creation code
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23bcd9ae-3b98-4dc0-9874-770c49b41a92
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/src/renderer/globals.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/DashboardSidebarHoverCardOverlay.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHoverCardOverlay/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/index.ts
| useEffect(() => { | ||
| stateRef.current = state; | ||
| }, [state]); |
There was a problem hiding this comment.
stateRef sync is one effect late for pointer-driven state transitions.
On Line [58], syncing stateRef in useEffect can lag behind rapid mouseenter/mouseleave sequences and make requestOpen/requestClose evaluate stale hoveredId. Use useLayoutEffect (or update the ref synchronously where state is set) to keep event-time checks in sync.
Suggested fix
import {
createContext,
useCallback,
useContext,
useEffect,
+ useLayoutEffect,
useMemo,
useRef,
useState,
} from "react";
@@
- useEffect(() => {
+ useLayoutEffect(() => {
stateRef.current = state;
}, [state]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| stateRef.current = state; | |
| }, [state]); | |
| import { | |
| createContext, | |
| useCallback, | |
| useContext, | |
| useEffect, | |
| useLayoutEffect, | |
| useMemo, | |
| useRef, | |
| useState, | |
| } from "react"; | |
| // ... (other code) | |
| useLayoutEffect(() => { | |
| stateRef.current = state; | |
| }, [state]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx`
around lines 58 - 60, The stateRef.current is being updated inside useEffect
which runs after paint and can be one event late for rapid pointer transitions;
change the sync to run synchronously by replacing useEffect with useLayoutEffect
(or alternatively assign stateRef.current = state immediately wherever state is
set) so that stateRef reflects the latest hoveredId during event handlers like
requestOpen and requestClose in DashboardSidebarHoverProvider.
| if (openTimerRef.current && stateRef.current.hoveredId === null) { | ||
| // Pending open for this id — cancel it. | ||
| clearOpenTimer(); | ||
| return; |
There was a problem hiding this comment.
Pending-open cancellation is not scoped to the hovered row id.
On Line [97], requestClose clears any pending open when hoveredId is null. If another row already scheduled a new open timer, this can cancel the wrong open and cause missed hover-card opens.
Suggested fix
const openTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const closeTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const pendingOpenIdRef = useRef<string | null>(null);
const clearOpenTimer = useCallback(() => {
if (openTimerRef.current) {
clearTimeout(openTimerRef.current);
openTimerRef.current = null;
+ pendingOpenIdRef.current = null;
}
}, []);
@@
clearOpenTimer();
+ pendingOpenIdRef.current = id;
openTimerRef.current = setTimeout(() => {
setState({ hoveredId: id, anchorElement: anchor, payload });
openTimerRef.current = null;
+ pendingOpenIdRef.current = null;
}, OPEN_DELAY_MS);
@@
if (openTimerRef.current && stateRef.current.hoveredId === null) {
- // Pending open for this id — cancel it.
+ // Cancel only if this id owns the pending open.
+ if (pendingOpenIdRef.current !== id) return;
clearOpenTimer();
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarHoverProvider/DashboardSidebarHoverProvider.tsx`
around lines 97 - 100, The requestClose logic currently clears any pending open
via openTimerRef when stateRef.current.hoveredId === null, which can cancel a
pending open for a different row; fix by scoping pending-open cancellation to
the specific row id: store the id associated with a pending open (e.g., change
openTimerRef to hold {timer, id} or add pendingOpenIdRef) when scheduling an
open in the function that sets the timer, and update requestClose (and
clearOpenTimer) to only clear the timer if the stored pending id matches the id
being closed (or matches stateRef.current.hoveredId) so you don't cancel timers
for other rows; update references in DashboardSidebarHoverProvider around
requestClose, clearOpenTimer, openTimerRef and any timer-creation code
accordingly.
* fix(desktop): collapse v2 sidebar count + actions into one slot In the project row and section header, the workspace count sat next to the title and the action button (new-workspace "+" / section-actions ellipsis) lived as a separate end-aligned control. The actions were hidden until hover, but the count was always visible — so the right edge had two competing elements at different times. Put count and action in the same slot at the row's end: count visible at rest, action overlays it on hover/focus. Drops the parens around the count and shrinks the project-row count from text-xs to text-[10px] to match the section header. Builds on the v2 sidebar work in #3813 (cc @saddlepaddle). * fix(desktop): indent v2 sidebar group header to match workspace rows Sections sat at pl-0.5 while workspaces under them were at pl-5 (from #3835). The section header now reads as if it's at the same level as the project row instead of grouping the workspaces beneath it. Bump the header to pl-5 so groups align with their workspaces under the project. * fix(desktop): ensure v2 sidebar group count is visible at rest The count span sat in DOM order before the absolute action wrapper, so the wrapper rendered on top of it. Even with the action button at opacity-0, the wrapper could swallow hover/focus that should belong to the count. Render the action wrapper first, the count after with relative positioning so it's the top layer, and add pointer-events-none so the action's hover target stays clean. Also pin text-muted-foreground explicitly to match the project row. * fix(desktop): nudge group header indent below workspace level Section was matching workspace at pl-5, which read as 'sibling' instead of 'parent'. Drop to pl-4 so the group sits one notch shallower than its workspaces. * fix(desktop): align v2 sidebar group header with project row spacing Bring the section header to pl-3 (matches project row) and add mr-2 between the chevron/grip slot and the title so the icon-to-text gap mirrors the project row's gap-2. * fix(desktop): align v2 sidebar group header indent with workspace rows Move section header from pl-3 to pl-5 so its chevron/grip aligns with the workspace icon column underneath it. * fix(desktop): indent grouped workspaces deeper than ungrouped ones Workspaces inside a section share the same pl-5 indent as ungrouped workspaces, so the section nesting reads flat. Plumb the existing isInSection prop through to the row and bump grouped workspaces from pl-5 to pl-7. * fix(desktop): address bot review on v2 sidebar count/action slot Three valid points from PR review: - Project row "+" button: Enter/Space keydown bubbles to the row's collapse handler before the synthesized click fires. Add onKeyDown stopPropagation so keyboard activation doesn't toggle the row. - Project row "+" overlay: opacity-0 doesn't disable pointer events, so clicking the count area at rest can accidentally create a workspace. Add pointer-events-none at rest, re-enable on hover / focus-within / focus-visible. - Section header count: the fade-on-hover ran unconditionally, but the action overlay only renders when actions is truthy. When actions is absent the slot would go blank on hover. Gate the fade classes on actions. * fix(desktop): swap count/action via display instead of stacked opacity Replaced the opacity layering (count opacity-0 + action opacity-100 on hover, with pointer-events-none gating to keep the count clickable) with a true display swap. Only one of the two is rendered at a time, so the count slot is never a transparent ghost over the button. For the section header, the dropdown trigger has data-state=open when its menu is open, which can persist after the row loses hover/focus. The action wrapper now stays visible via has-[[data-state=open]]:flex, and the count hides via peer-has-[[data-state=open]]:hidden so the open state still wins over the count. * fix(desktop): swap thumbnail/chevron via display for project row consistency The thumbnail-at-rest / chevron-on-hover swap at the start of the project row was still using stacked opacities while the count/action slot at the row's end now uses display swap. Convert the icon slot to match — thumbnail is hidden on group-hover, chevron is hidden by default and shown on group-hover. One pattern across the row. * fix(desktop): keep v2 sidebar count visible when row is self-focused Project and section rows are role=button tabIndex=0, so clicking them (e.g., to collapse a section) gives the row itself :focus. group-focus-within matches that self-focus, which made the count hide and stay hidden — most visibly when collapsing a section that contains the active workspace: the user clicks the header, focus stays on it, and the count never returns until they click somewhere else. Switch to group-has-[:focus]: it only matches when a *descendant* has focus (e.g., the rename input or the dropdown trigger), not when the row itself is focused. Same swap on the action wrapper / + button so the action only appears when the user actually focuses into the row's interactive bits.
Summary
DashboardSidebarHoverProviderowns the hover state (open/close timers, anchor element, payload, context-menu suppression). NewDashboardSidebarHoverCardOverlayrenders onePopover+PopoverAnchor virtualRefand reuses the existingDashboardSidebarWorkspaceHoverCardContent.data-positionedflag suppresses the slide transition during the initial open so we don't animate from Radix's off-screen measuring transform.onHoverCardOpen) is still gated to local-device hosts.v1 sidebar (
WorkspaceHoverCardunderscreens/main/...) is untouched — it's still the default path for users who haven't opted into v2.Test plan
Summary by cubic
Make the v2 dashboard sidebar use one persistent hover card that follows the hovered row and slides between items. This removes the per-row 400ms delay and flicker, and restores hover cards for cloud workspaces.
HoverCardwith a single sidebar-levelPopoveranchored via virtualRef that slides between rows.DashboardSidebarHoverProviderto manage open/close timers, anchor element, payload, and context menu suppression; includessyncIfHovered.DashboardSidebarHoverCardOverlayto render the sharedPopover+PopoverAnchorand reuse existingDashboardSidebarWorkspaceHoverCardContent;useDiffStatslifted to the overlay.DashboardSidebarHoverCardOverlay.cssand gate the slide until positioned to avoid animating from the off-screen measuring transform; initial open does not animate.Written for commit 39d49a4. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Refactor
Style