Repository navigation
fix(desktop): keep workspace activity popover anchored to its button - #9165
harshithmullapudi wants to merge 1 commit into
Conversation
The floating activity popover anchored to a zero-height span in the top-right pane instead of the trigger. floating-ui's layout-shift observer skips zero-size references, so when the window or split layout resized the popover stayed at its old coordinates. Anchor the popover to the trigger button with PopoverAnchor asChild and remove the span and its store state, which nothing else used. Fixes SUPER-3055
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Replace workspace anchor with trigger anchor apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceActivityMenu/WorkspaceActivityMenu.tsx, .../components/ActivityPanelAside/ActivityPanelAside.tsx, .../constants.ts, .../stores/activityPanelStore/activityPanelStore.ts |
The activity menu popover uses its trigger button as its anchor. The aside no longer renders or registers a separate anchor, and the store no longer maintains per-workspace anchor nodes. The PANEL_TOP_GAP_PX constant and explicit sideOffset={0} prop were removed. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: avipeltz
Merge Risk: 
Minimal · up to 85d73
-
The change has no confirmed merge-blocking risk and is ready to merge with normal checks.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Description check
-
Explanation
The pull request description is complete and relevant. It explains what changed, why the change was needed, implementation details, affected modes, manual QA results, automated checks, and the linked issue. It does not include the repository checklist section, but the required information is otherwise covered.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
[Low impact] The PR appears safe to merge; no actionable issues were found. SummaryThe floating workspace activity popover now anchors to its button instead of an invisible span.
Reviews (1) · Last reviewed commit: "fix(desktop): keep workspace activity po..." · Reviewed by Greptile |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
|
Review: looks good. The change is small and removes code. At Findings:
Full report with before/after screenshots and videos: https://app.superset.sh/page/pr-9165-review-activity-popover-anchor-6jlsgm |
Links
Summary
activityPanelStorestate, which only existed to anchor this popover.Why / Context
In floating mode (center area too narrow to dock the panel), the popover was anchored through a
virtualRefto an invisibleh-0span thatActivityPanelAsideput in the top-right pane, not to the button. floating-ui's layout-shift observer returns early for references with no width or height, and the span never changes size, so its ResizeObserver never fires either. The windowresizeevent did fire, but before the split layout settled. After that nothing repositioned the popover, so it drifted 174px off the button on a 1920→1500 resize and overshot by 162px on the way back (numbers on the page above).How It Works
WorkspaceActivityMenu: the trigger button is wrapped in<PopoverAnchor asChild>, so Radix/floating-ui track the real button.sideOffsetgoes back to the default 4px, because the popover now sits under the button rather than under an offset span.ActivityPanelAside,activityPanelStore,constants: drop the anchor span,anchorNodes/setAnchorNode/clearAnchorNode, andPANEL_TOP_GAP_PX.Docked mode (panel in the pane aside) is untouched.
Manual QA Checklist
Desktop dev build in this workspace, driven over CDP with real mouse input and X11 window resizes. Same script before and after:
console.errorfired.Ctrl+Shift+B(toggle workspace sidebar). Base (parent commit's files, hot-reloaded): button moves to x=1165 and the popover stays at x=1250, 85px off. Fix: offset 0, and it stays 0 when the sidebar is shown again. The window-resize drift depends on timing (it needs the split layout to re-settle after theresizeevent), but any layout change that fires noresizeevent reproduces it every time. The reported recording shows this case: the right pane closes while the popover is open.Design Decisions
h-px) that floating-ui can track. That alternative was not tested.Testing
bun run typecheck(apps/desktop)bunx biome checkon the changed filesbun test .../WorkspaceActivityMenu(14 pass)No unit test added. The bug lives in floating-ui's live layout tracking, which bun's DOM-less tests can't exercise, so the CDP repro is the regression check.
Summary by cubic
Fixes the workspace activity popover drifting up to ~174px when the window or split layout resizes, by anchoring it to the trigger button instead of an invisible zero-height span.
h-0span in the right pane viavirtualRef; floating-ui skips zero-size references, so the popover kept stale coordinates after the first resize.PopoverAnchor asChild, and the popover sits 4px below it via the defaultsideOffset.anchorNodesstore state, andPANEL_TOP_GAP_PX; docked panel mode is unchanged.Written for commit 85d7371. Summary will update on new commits.
Summary by CodeRabbit