refactor(frontend): extract shared primitives and remove duplicated ui clones#14042
Open
tarciorodrigues wants to merge 8 commits into
Open
refactor(frontend): extract shared primitives and remove duplicated ui clones#14042tarciorodrigues wants to merge 8 commits into
tarciorodrigues wants to merge 8 commits into
Conversation
…er to GlobalVariableDeleteConfirmation The shared/components/delete-confirmation-modal.tsx sounds generic but is a global-variables-specific wrapper around modals/deleteConfirmationModal. Rename it honestly and relocate it next to the global-variables feature under components/core/globalVariableDeleteConfirmation. No behavior change; both consumers and the jest mock repointed.
The copyToClipboard + isCopied + reset-timeout pattern is reimplemented across several components. Extract it to shared/hooks/use-copy-to-clipboard (canonical copy/isCopied shape, resetDelay defaulting to the current 1s) and adopt it in useMcpServer first; other call sites migrate opportunistically.
Extract the icon + title + description + CTA shape reimplemented by KnowledgeBaseEmptyState, DeploymentsEmptyState and AssistantEmptyState into components/ui/empty-state.tsx. Compound anatomy (Empty/EmptyHeader/ EmptyMedia/EmptyTitle/EmptyDescription/EmptyContent) mirroring upstream shadcn/ui Empty instead of flat props: the three call sites differ in sizes, spacing and button variants, and slots with className passthrough keep each render pixel-identical.
select-custom.tsx cloned select.tsx with three deltas: an unstyled chevron-less trigger, a cursor-pointer indicator-less item, and a SelectContentWithoutPortal variant. Express the first two as variant="plain" on SelectTrigger/SelectItem, move SelectContentWithoutPortal to select.tsx verbatim, compensate the min-w-[11.5rem] base at the repointed SelectContent call sites, and delete the clone. Consumers and test mocks repointed.
dialog-with-no-close.tsx is not a close-button-less clone: dialog.tsx already has hideCloseButton, and the file is a structurally different minimal dialog (self-centered content, overlayShow/contentShow animations, p-3/gap-3 grid, raw portal, no a11y title injection). Only two of its exports are consumed (Dialog, which is identical, and its DialogContent). Move that content verbatim into dialog.tsx as DialogContentPlain (with its private plain overlay), alias it at the two consumers (baseModal, ListSelectionComponent), delete the file. A boolean-prop merge would have changed baseModal rendering app-wide.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesThe frontend replaces custom select and dialog modules with shared plain variants, introduces reusable empty-state primitives, renames the global-variable delete confirmation component, and centralizes clipboard state in a shared hook. Shared UI migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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 |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/frontend/src/components/ui/select.tsx (2)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace empty
SelectPrimitive.Iconwithnullfor clarity.The
plainbranch renders<SelectPrimitive.Icon asChild></SelectPrimitive.Icon>with no children — a no-op since Radix'sSlotreturns nothing without a child. Usingnullis clearer and avoids the non-standardasChild-without-children pattern.♻️ Proposed refactor
{children} - {variant === "plain" ? ( - <SelectPrimitive.Icon asChild></SelectPrimitive.Icon> - ) : ( + {variant === "plain" ? null : ( <SelectPrimitive.Icon asChild> {direction === "up" ? ( <ChevronUp className="h-4 w-4" /> ) : ( <ChevronDown className="h-4 w-4" /> )} </SelectPrimitive.Icon> )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/components/ui/select.tsx` around lines 38 - 39, In the plain variant branch of the select component, replace the empty SelectPrimitive.Icon element with null. Preserve the existing alternate branch and all other SelectPrimitive behavior.
84-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared class constants to reduce duplication between
SelectContentandSelectContentWithoutPortal.Both components share identical animation, side-position, and viewport class strings — differing only in
min-wand the portal wrapper. Extracting shared constants prevents class-string drift and reduces maintenance burden.♻️ Proposed refactor
+const selectContentBaseClass = + "relative z-50 overflow-hidden rounded-md border bg-popover text-popover-foreground shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2"; +const selectContentPopperClass = + "data-[side=bottom]:translate-y-1 data-[side=left]:-translate-x-1 data-[side=right]:translate-x-1 data-[side=top]:-translate-y-1"; +const selectViewportPopperClass = + "h-[var(--radix-select-trigger-height)] w-full min-w-[var(--radix-select-trigger-width)]"; const SelectContent = React.forwardRef<...>(({ className, children, position = "popper", ...props }, ref) => ( <SelectPrimitive.Portal> <SelectPrimitive.Content ref={ref} className={cn( - "relative z-50 min-w-[8rem] overflow-hidden rounded-md border bg-popover text-popover-foreground shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", + selectContentBaseClass, + "min-w-[8rem]", - position === "popper" && - "data-[side=bottom]:translate-y-1 data-[side=left]:-translate-x-1 data-[side=right]:translate-x-1 data-[side=top]:-translate-y-1", + position === "popper" && selectContentPopperClass, className, )} ... > <SelectPrimitive.Viewport className={cn( "p-1", - position === "popper" && - "h-[var(--radix-select-trigger-height)] w-full min-w-[var(--radix-select-trigger-width)]", + position === "popper" && selectViewportPopperClass, )} > {children} </SelectPrimitive.Viewport> </SelectPrimitive.Content> </SelectPrimitive.Portal> )); const SelectContentWithoutPortal = React.forwardRef<...>(({ className, children, position = "popper", ...props }, ref) => ( <SelectPrimitive.Content ref={ref} className={cn( - "relative z-50 min-w-[11.5rem] overflow-hidden rounded-md border bg-popover text-popover-foreground shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", + selectContentBaseClass, + "min-w-[11.5rem]", - position === "popper" && - "data-[side=bottom]:translate-y-1 data-[side=left]:-translate-x-1 data-[side=right]:translate-x-1 data-[side=top]:-translate-y-1", + position === "popper" && selectContentPopperClass, className, )} ... > <SelectPrimitive.Viewport className={cn( "p-1", - position === "popper" && - "h-[var(--radix-select-trigger-height)] w-full min-w-[var(--radix-select-trigger-width)]", + position === "popper" && selectViewportPopperClass, )} > {children} </SelectPrimitive.Viewport> </SelectPrimitive.Content> ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/components/ui/select.tsx` around lines 84 - 111, Extract the identical content animation/position classes and viewport classes into shared constants, then reuse them in both SelectContent and SelectContentWithoutPortal. Preserve each component’s existing differences, including the portal wrapper and min-width behavior, while eliminating duplicated class strings.src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx (1)
115-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the shared Select migration with a real integration test.
Both suites replace
@/components/ui/selectwith inert mocks, so they cannot validate the actual Radix composition or the new plain variants.
src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx#L115-L197: retain only if needed for isolation, and add real Select coverage for menu rendering and selection.src/frontend/src/pages/FlowPage/components/nodeToolbarComponent/__tests__/config-transition.test.tsx#L147-L160: keep configuration-transition isolation, but move Select contract coverage to an integration test using the real shared primitive.As per coding guidelines, frontend tests should avoid excessive mocks and verify meaningful behavior rather than only smoke-testing it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx` around lines 115 - 197, Replace the inert Select mock in sidebarDraggableComponent.test.tsx (lines 115-197) with integration coverage using the real shared Select primitive, verifying menu rendering and item selection; retain the mock only if required for unrelated isolation. In config-transition.test.tsx (lines 147-160), keep Select mocking solely for configuration-transition isolation and move Select contract assertions to the integration test, including behavior of the new plain variants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/shared/hooks/use-copy-to-clipboard.ts`:
- Around line 12-23: Update the use-copy-to-clipboard hook’s copy callback to
store the reset timeout in a useRef, clearing any existing timeout before
scheduling a new one so rapid copies keep isCopied true for the full resetDelay.
Add unmount cleanup to clear the pending timeout, and preserve the existing
clipboard success behavior.
---
Nitpick comments:
In `@src/frontend/src/components/ui/select.tsx`:
- Around line 38-39: In the plain variant branch of the select component,
replace the empty SelectPrimitive.Icon element with null. Preserve the existing
alternate branch and all other SelectPrimitive behavior.
- Around line 84-111: Extract the identical content animation/position classes
and viewport classes into shared constants, then reuse them in both
SelectContent and SelectContentWithoutPortal. Preserve each component’s existing
differences, including the portal wrapper and min-width behavior, while
eliminating duplicated class strings.
In
`@src/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsx`:
- Around line 115-197: Replace the inert Select mock in
sidebarDraggableComponent.test.tsx (lines 115-197) with integration coverage
using the real shared Select primitive, verifying menu rendering and item
selection; retain the mock only if required for unrelated isolation. In
config-transition.test.tsx (lines 147-160), keep Select mocking solely for
configuration-transition isolation and move Select contract assertions to the
integration test, including behavior of the new plain variants.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5698e69-860b-4368-9a93-d0a544a0984c
📒 Files selected for processing (25)
src/frontend/src/CustomNodes/GenericNode/components/ListSelectionComponent/index.tsxsrc/frontend/src/CustomNodes/NoteNode/NoteToolbarComponent/index.tsxsrc/frontend/src/CustomNodes/NoteNode/components/select-items.tsxsrc/frontend/src/components/core/assistantPanel/components/assistant-empty-state.tsxsrc/frontend/src/components/core/folderSidebarComponent/components/sideBarFolderButtons/components/select-options.tsxsrc/frontend/src/components/core/globalVariableDeleteConfirmation/index.tsxsrc/frontend/src/components/core/parameterRenderComponent/components/inputGlobalComponent/__tests__/index.test.tsxsrc/frontend/src/components/core/parameterRenderComponent/components/inputGlobalComponent/index.tsxsrc/frontend/src/components/core/playgroundComponent/chat-view/chat-header/components/session-more-menu.tsxsrc/frontend/src/components/ui/dialog-with-no-close.tsxsrc/frontend/src/components/ui/dialog.tsxsrc/frontend/src/components/ui/empty-state.tsxsrc/frontend/src/components/ui/select-custom.tsxsrc/frontend/src/components/ui/select.tsxsrc/frontend/src/modals/IOModal/components/IOFieldView/components/session-selector.tsxsrc/frontend/src/modals/IOModal/components/chatView/chatInput/components/voice-assistant/components/audio-settings/audio-settings-dialog.tsxsrc/frontend/src/modals/baseModal/index.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/__tests__/sidebarDraggableComponent.test.tsxsrc/frontend/src/pages/FlowPage/components/flowSidebarComponent/components/sidebarDraggableComponent.tsxsrc/frontend/src/pages/FlowPage/components/nodeToolbarComponent/__tests__/config-transition.test.tsxsrc/frontend/src/pages/FlowPage/components/nodeToolbarComponent/index.tsxsrc/frontend/src/pages/MainPage/pages/deploymentsPage/components/deployments-empty-state.tsxsrc/frontend/src/pages/MainPage/pages/homePage/hooks/useMcpServer.tssrc/frontend/src/pages/MainPage/pages/knowledgePage/components/KnowledgeBaseEmptyState.tsxsrc/frontend/src/shared/hooks/use-copy-to-clipboard.ts
💤 Files with no reviewable changes (2)
- src/frontend/src/components/ui/dialog-with-no-close.tsx
- src/frontend/src/components/ui/select-custom.tsx
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
refactor
github-actions
Bot
added
refactor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.11.0 #14042 +/- ##
==================================================
+ Coverage 60.16% 60.31% +0.15%
==================================================
Files 2333 2334 +1
Lines 228353 228835 +482
Branches 32038 34132 +2094
==================================================
+ Hits 137386 138021 +635
+ Misses 89351 89198 -153
Partials 1616 1616
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
LE-1735 Tier A componentization — work package 1. The frontend accumulated exact and near-miss clones of UI primitives and state logic: a full copy of the select primitive, a minimal dialog clone, three hand-rolled empty states, repeated copy-to-clipboard state, and a global-variable delete confirmation living under a misleading generic path. This PR removes that duplication with behavior-preserving moves only — one concern per commit, each trivially revertible.
What
GlobalVariableDeleteConfirmationand relocate it tocomponents/core/globalVariableDeleteConfirmation/; both prod consumers and the jest mock repointed.shared/hooks/use-copy-to-clipboard.tsas the canonicalcopy/isCopiedhook and adopt it inuseMcpServer. The remaining call sites with the same inline pattern are left for a follow-up to keep this PR mechanical.components/ui/empty-state.tsxas a compound component and recompose the three existing empty states on it, keeping their exact classes. Deviation from the ticket letter: compound API for upstream shadcn parity instead of the flat-props shape.select-custom.tsxintoselect.tsx:variant="plain"on SelectTrigger/SelectItem,SelectContentWithoutPortalmoved verbatim, spacing compensated at the 4 call sites, clone deleted; 7 prod consumers and 2 test mocks repointed.dialog-with-no-close.tsxintodialog.tsxasDialogContentPlain. Deviation from the ticket letter: the suggested boolean-prop merge was not possible —dialog.tsxalready shipshideCloseButton, and the clone is a structurally different minimal dialog whose prop-merge would repaintbaseModalapp-wide; moved verbatim instead and aliased at the 2 consumers.Net effect: about 255 LOC of clones deleted; no exported symbol renamed without its consumers repointed.
Tests
make format_frontendandmake test_frontendgreen — 444 suites / 5361 tests.git grepof every removed or moved symbol returns zero hits.SelectContentWithoutPortal, playground session menu, plain dialogs includingbaseModal, MCP Server JSON copy. Console compared base vs branch on the identical click path: the same 6 pre-existing errors, zero new ones.Note
Knowledge-base empty state, Deployments page and Assistant panel are covered by unit tests only — not reachable in a dev session without destroying user data or enabling feature flags.
Refs LE-1735
Screenshots
Global variable delete confirmation
Light
Before
After
Dark
Before
After
Node toolbar select
Light
Before
After
Dark
Before
After
Folder options menu
Light
Before
After
Dark
Before
After
MCP Server JSON copy
Light
Before
After
Dark
Before
After
Summary by CodeRabbit
New Features
Bug Fixes