Skip to content

refactor: Popover cleanup#7356

Merged
raunakab merged 7 commits intomainfrom
popover-cleanup
Jan 12, 2026
Merged

refactor: Popover cleanup#7356
raunakab merged 7 commits intomainfrom
popover-cleanup

Conversation

@raunakab
Copy link
Copy Markdown
Contributor

@raunakab raunakab commented Jan 12, 2026

Description

  • Moved popover.tsx to refresh-components/Popover.tsx with compound component API (Popover.Trigger, Popover.Content, etc.)
  • Added JSDoc documentation and exported Popover.Anchor
  • Updated 18+ files to use new import path and API
  • Migrated DefaultDropdown component to new Popover API
  • Deleted old popover files from components/popover/ and components/ui/

This PR was validated through the UI.

Additional Options

  • [Optional] Override Linear Check

Summary by cubic

Consolidated all popover implementations into a single compound component at refresh-components/Popover and migrated usages across the app. This standardizes styling, simplifies imports, and improves positioning (especially inside overflow-hidden containers).

  • Refactors

    • Introduced compound API: Popover.Trigger, Popover.Content, Popover.Anchor, Popover.Close, Popover.Menu, with JSDoc.
    • Replaced shadcn and legacy popover components; removed old files under components/ui/popover and components/popover/.
    • Updated dropdowns, date pickers, admin tables, sidebar menus, actions/file pickers, and LLM popovers to the new API; added “use client” where needed.
    • Added Popover.Content wide option for consistent menu widths; removed unused components (SimplifiedChatInputBar, HorizontalSourceSelector, TagFilter).
  • Migration

    • Import Popover from refresh-components/Popover and use Popover.Trigger asChild and Popover.Content for popover bodies.
    • Use Popover.Anchor when positioning relative to a non-trigger element; use Popover.Menu (also exported as PopoverMenu).
    • Set align/side/sideOffset on Popover.Content; use wide for preset width.
    • Remove legacy props like matchWidth and triggerMaxWidth and update any imports from components/ui/popover or components/popover.

Written for commit 8b73b8b. Summary will update on new commits.

@raunakab raunakab requested a review from a team as a code owner January 12, 2026 10:28
@raunakab raunakab enabled auto-merge January 12, 2026 10:29
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 28 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 12, 2026

Greptile Overview

Greptile Summary

This PR refactors the Popover component from multiple scattered implementations into a unified compound component API at refresh-components/Popover.tsx. The migration successfully updates 18+ files to use the new Popover.Trigger, Popover.Content, Popover.Anchor pattern, adds comprehensive JSDoc documentation, and removes old implementations.

Key Changes:

  • New API: Compound component pattern (Popover.Trigger, Popover.Content, etc.) replacing the old props-based API
  • Import consolidation: All imports now use @/refresh-components/Popover following absolute import standards
  • Documentation: Added extensive JSDoc comments with examples
  • Cleanup: Removed unused imports in LLMSelector.tsx and deleted old popover files
  • Layout integration: Several components now use Section from general-layouts for consistent spacing

Critical Issues Found:

  1. Lost padding customization: The new PopoverContent uses WithoutStyles type and hardcodes p-1 padding, but Calendar components in AdminDateRangeSelector and UsageReports previously used className="p-0" to remove padding. This will add unwanted 4px padding around calendars.

  2. Missing matchWidth feature: The DefaultDropdown previously used matchWidth prop that set the popover width to match the trigger via CSS variables. This functionality is lost, potentially causing dropdown content to not match trigger width.

  3. Width mismatch: SignedUpUserTable changed from w-48 (192px) to wide (280px), a 46% increase that may cause unexpected UI changes.

  4. Reduced padding: LLMPopover padding decreased from p-1.5 to p-1 (minor visual change).

Positive Aspects:

  • Most migrations are clean and follow web standards correctly
  • Good use of absolute imports with @ prefix
  • Proper cleanup of unused imports
  • Added "use client" directives where needed
  • Consistent compound component pattern throughout

Confidence Score: 3/5

  • This PR has multiple visual regressions that will affect the UI, particularly around Calendar popovers and dropdown sizing
  • While the refactoring is architecturally sound with good documentation and follows standards, there are three critical issues that will cause visible UI problems: (1) Calendar popovers will have unwanted padding, (2) dropdowns may not match trigger width, and (3) one popover is 46% wider than intended. These are not breaking runtime errors but will noticeably affect user experience.
  • Pay close attention to web/src/refresh-components/Popover.tsx (needs padding flexibility), web/src/components/Dropdown.tsx (needs matchWidth support), web/src/components/admin/users/SignedUpUserTable.tsx (width mismatch), and calendar popover implementations in AdminDateRangeSelector.tsx and UsageReports.tsx (padding issues)

Important Files Changed

File Analysis

Filename Score Overview
web/src/refresh-components/Popover.tsx 3/5 New compound component API with good JSDoc documentation. However, PopoverContent hardcodes padding and removes className support, breaking previous customizations in Calendar popovers.
web/src/components/Dropdown.tsx 3/5 Successfully migrated to new Popover API, but lost matchWidth functionality which may cause dropdown content to not match trigger width.
web/src/components/admin/users/SignedUpUserTable.tsx 3/5 Migration changes popover width from 192px to 280px (88px wider), which may cause unexpected UI changes.
web/src/refresh-components/popovers/LLMPopover.tsx 4/5 Successfully migrated with Section wrapper for layout. Minor padding reduction from p-1.5 to p-1.
web/src/components/dateRangeSelectors/AdminDateRangeSelector.tsx 3/5 Migration removes custom className with p-0, now hardcoded p-1 adds unwanted padding around Calendar component.
web/src/app/ee/admin/performance/usage/UsageReports.tsx 3/5 Same issue as AdminDateRangeSelector - lost p-0 customization, Calendar now has unwanted padding.

Sequence Diagram

sequenceDiagram
    participant User
    participant Component
    participant PopoverRoot
    participant PopoverTrigger
    participant PopoverContent
    participant RadixUI

    User->>Component: Clicks trigger element
    Component->>PopoverRoot: open=true, onOpenChange
    PopoverRoot->>RadixUI: Initialize Radix Popover state
    
    Component->>PopoverTrigger: Render trigger with asChild
    PopoverTrigger->>RadixUI: Register trigger element
    
    Component->>PopoverContent: Render with align, side, sideOffset
    PopoverContent->>PopoverContent: Apply widthClasses (main/wide)
    PopoverContent->>PopoverContent: Apply fixed styles (p-1, rounded-12, etc.)
    PopoverContent->>RadixUI: Create Portal
    RadixUI->>User: Display popover positioned near trigger
    
    User->>PopoverContent: Clicks inside popover
    PopoverContent->>Component: Event propagates to content
    
    User->>User: Clicks outside popover
    RadixUI->>PopoverRoot: Detect outside click
    PopoverRoot->>Component: onOpenChange(false)
    Component->>PopoverRoot: open=false
    RadixUI->>User: Hide popover with animation
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@Subash-Mohan Subash-Mohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Added a few small nits, but approving the PR.

@raunakab raunakab added this pull request to the merge queue Jan 12, 2026
@Subash-Mohan Subash-Mohan removed this pull request from the merge queue due to a manual request Jan 12, 2026
@raunakab raunakab enabled auto-merge January 12, 2026 11:53
@raunakab raunakab added this pull request to the merge queue Jan 12, 2026
Merged via the queue into main with commit c78fe27 Jan 12, 2026
76 checks passed
@raunakab raunakab deleted the popover-cleanup branch January 12, 2026 12:13
jessicasingh7 pushed a commit that referenced this pull request Jan 12, 2026
jessicasingh7 pushed a commit that referenced this pull request Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants