refactor(emcn): make ChipModal footer/header props-driven and migrate all consumers#4905
refactor(emcn): make ChipModal footer/header props-driven and migrate all consumers#4905emir-karabeg wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Curated prompt handoff to home chat is tightened so integration names chip only when intended. Landing integration template cards rewrite names to Minor follow-ons: mic control gets Reviewed by Cursor Bugbot for commit 94d36b2. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 94d36b2. Configure here.
| disabled: deleteSchedule.isPending, | ||
| variant: 'destructive', | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Header close skips state reset
Low Severity
Required ChipModalHeader close handlers only hide the delete dialog, while footer Cancel also clears the targeted API key or scheduled task. Dismissing via the new header control leaves that selection in state, unlike Cancel and unlike other migrated delete modals that reset both in onClose.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 94d36b2. Configure here.
Greptile SummaryThis PR standardises all ~70
Confidence Score: 3/5The migration is structurally sound but the new ChipModalFooter API drops the ability to disable the Cancel button, which several destructive-action modals depended on to prevent mid-mutation dismissal. The new props-driven API is a genuine improvement in consistency and reduces boilerplate across 70+ call sites. However, removing the Cancel apps/sim/components/emcn/components/chip-modal/chip-modal.tsx (ChipModalFooterProps is missing cancelDisabled), apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/delete-modal/delete-modal.tsx, apps/sim/app/workspace/[workspaceId]/settings/components/team-management/components/remove-member-dialog/remove-member-dialog.tsx, apps/sim/app/workspace/[workspaceId]/settings/components/byok/byok.tsx Important Files Changed
Sequence DiagramsequenceDiagram
participant Consumer as Consumer Modal
participant Footer as ChipModalFooter
participant CancelChip as Cancel Chip
participant PrimaryChip as Primary Chip
participant SecondaryChip as Secondary Chip (optional)
Consumer->>Footer: "props { onCancel, primaryAction, secondaryAction? }"
Footer->>SecondaryChip: render if secondaryAction (far-left, variant default 'filled')
Footer->>CancelChip: "render always (label='Cancel', no disabled support)"
Footer->>PrimaryChip: render always (variant default 'primary', disabled? supported)
Note over CancelChip: Always enabled — even during in-flight mutations
CancelChip-->>Consumer: onClick → onCancel()
PrimaryChip-->>Consumer: onClick → primaryAction.onClick()
SecondaryChip-->>Consumer: onClick → secondaryAction.onClick()
Reviews (1): Last reviewed commit: "refactor(emcn): make ChipModal footer/he..." | Re-trigger Greptile |
| */ | ||
| leading?: React.ReactNode | ||
| secondaryAction?: ChipModalFooterAction | ||
| } | ||
|
|
||
| /** | ||
| * Footer row. Renders the leading inset separator and a tinted action bar. | ||
| * Pass `leading` to left-dock a secondary action (e.g. Delete in edit mode); | ||
| * primary actions always go in `children` and are right-aligned. | ||
| * Footer row with a fixed, declarative shape: an optional far-left | ||
| * `secondaryAction`, then the always-present Cancel and the right-anchored | ||
| * `primaryAction`. Buttons are described via {@link ChipModalFooterAction} and | ||
| * rendered as {@link Chip}s, so no footer can drift from the canonical layout. | ||
| */ | ||
| const ChipModalFooter = React.forwardRef<HTMLDivElement, ChipModalFooterProps>( | ||
| ({ className, leading, children, ...props }, ref) => ( | ||
| function ChipModalFooter({ onCancel, primaryAction, secondaryAction }: ChipModalFooterProps) { | ||
| return ( |
There was a problem hiding this comment.
ChipModalFooterAction exposes a disabled field for the primary and secondary buttons, but there is no equivalent for the always-rendered Cancel button. Across ~9 migrated modals the old Cancel Chip was explicitly disabled while an async operation was in flight: byok.tsx (disabled={upsertKey.isPending}), row-modal.tsx (disabled={isSubmitting}), delete-modal.tsx (disabled={isDeleting}), remove-member-dialog.tsx (disabled={isSubmitting}), transfer-ownership-dialog.tsx (disabled={isSubmitting}), and several more.
The concrete failure: a user clicks Delete (or Remove/Transfer) on a destructive confirmation, sees "Deleting...", then clicks the now-enabled Cancel button. The modal closes and the user assumes the operation was cancelled — but the mutation continues in the background and completes. Data is deleted/removed without the user realising it.
Adding cancelDisabled?: boolean to ChipModalFooterProps and threading it into the Cancel <Chip disabled={cancelDisabled}> would restore the previous safety guard across all consumers.
| onOpenChange={setShowDiscardAlert} | ||
| srTitle='Unsaved Changes' | ||
| > | ||
| <ChipModalHeader showDivider={false}>Unsaved Changes</ChipModalHeader> | ||
| <ChipModalHeader onClose={() => setShowDiscardAlert(false)}> | ||
| Unsaved Changes | ||
| </ChipModalHeader> | ||
| <ChipModalBody> | ||
| <p className='px-2 text-[var(--text-secondary)] text-sm'> | ||
| You have unsaved changes. Are you sure you want to discard them? | ||
| </p> | ||
| </ChipModalBody> | ||
| <ChipModalFooter> | ||
| <Chip variant='filled' flush onClick={() => setShowDiscardAlert(false)}> | ||
| Keep Editing | ||
| </Chip> | ||
| <Chip variant='destructive' flush onClick={handleConfirmDiscard}> | ||
| Discard Changes | ||
| </Chip> | ||
| </ChipModalFooter> | ||
| <ChipModalFooter | ||
| onCancel={() => setShowDiscardAlert(false)} | ||
| primaryAction={{ | ||
| label: 'Discard Changes', | ||
| onClick: handleConfirmDiscard, |
There was a problem hiding this comment.
The old Unsaved Changes dialog used the label "Keep Editing" for the dismiss action; the new hardcoded label is "Cancel". In an unsaved-changes confirmation context, "Cancel" is ambiguous: it could mean "cancel the discard decision" (stay and keep editing) or "cancel the save" (leave without saving). "Keep Editing" was unambiguous. Because the new ChipModalFooterProps explicitly forbids relabeling the dismiss button, this cannot be corrected at the call site — the API design is the constraint.
The same ambiguity appears in the "Unsaved Changes" dialog in ee/access-control/components/access-control.tsx.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function ChipModalFooter({ onCancel, primaryAction, secondaryAction }: ChipModalFooterProps) { | ||
| return ( | ||
| <div className='flex flex-col'> | ||
| <ChipModalSeparator /> | ||
| <div | ||
| ref={ref} | ||
| className={cn( | ||
| 'flex items-center gap-2 bg-[var(--surface-3)] px-4 pt-2 pb-2', | ||
| leading ? 'justify-between' : 'justify-end', | ||
| className | ||
| secondaryAction ? 'justify-between' : 'justify-end' | ||
| )} | ||
| {...props} | ||
| > | ||
| {leading && <div>{leading}</div>} | ||
| <div className='flex gap-2'>{children}</div> | ||
| {secondaryAction ? ( | ||
| <Chip | ||
| variant={secondaryAction.variant ?? 'filled'} | ||
| flush | ||
| onClick={secondaryAction.onClick} | ||
| disabled={secondaryAction.disabled} | ||
| > | ||
| {secondaryAction.label} | ||
| </Chip> | ||
| ) : null} | ||
| <div className='flex gap-2'> | ||
| <Chip variant='filled' flush onClick={onCancel}> | ||
| Cancel | ||
| </Chip> | ||
| <Chip | ||
| variant={primaryAction.variant ?? 'primary'} | ||
| flush | ||
| onClick={primaryAction.onClick} | ||
| disabled={primaryAction.disabled} | ||
| > | ||
| {primaryAction.label} | ||
| </Chip> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
ChipModalFooter was converted from React.forwardRef (which returns React.ForwardRefExoticComponent, a type that includes displayName) to a plain function declaration. Setting .displayName on a plain function-valued variable is valid JavaScript and TypeScript does allow property augmentation via declaration merging (TS 3.1+), so this won't be a hard compile error — but the component can no longer receive a forwarded ref. If any future consumer needs to measure or focus the footer, this will silently fail. Keeping the component as a forwardRef or at minimum explicitly typing it as React.FC<ChipModalFooterProps> would preserve the ref channel and make the displayName intent more conventional.
| function ChipModalFooter({ onCancel, primaryAction, secondaryAction }: ChipModalFooterProps) { | |
| return ( | |
| <div className='flex flex-col'> | |
| <ChipModalSeparator /> | |
| <div | |
| ref={ref} | |
| className={cn( | |
| 'flex items-center gap-2 bg-[var(--surface-3)] px-4 pt-2 pb-2', | |
| leading ? 'justify-between' : 'justify-end', | |
| className | |
| secondaryAction ? 'justify-between' : 'justify-end' | |
| )} | |
| {...props} | |
| > | |
| {leading && <div>{leading}</div>} | |
| <div className='flex gap-2'>{children}</div> | |
| {secondaryAction ? ( | |
| <Chip | |
| variant={secondaryAction.variant ?? 'filled'} | |
| flush | |
| onClick={secondaryAction.onClick} | |
| disabled={secondaryAction.disabled} | |
| > | |
| {secondaryAction.label} | |
| </Chip> | |
| ) : null} | |
| <div className='flex gap-2'> | |
| <Chip variant='filled' flush onClick={onCancel}> | |
| Cancel | |
| </Chip> | |
| <Chip | |
| variant={primaryAction.variant ?? 'primary'} | |
| flush | |
| onClick={primaryAction.onClick} | |
| disabled={primaryAction.disabled} | |
| > | |
| {primaryAction.label} | |
| </Chip> | |
| </div> | |
| </div> | |
| </div> | |
| ) | |
| ) | |
| } | |
| function ChipModalFooter({ onCancel, cancelDisabled, primaryAction, secondaryAction }: ChipModalFooterProps) { | |
| return ( | |
| <div className='flex flex-col'> | |
| <ChipModalSeparator /> | |
| <div | |
| className={cn( | |
| 'flex items-center gap-2 bg-[var(--surface-3)] px-4 pt-2 pb-2', | |
| secondaryAction ? 'justify-between' : 'justify-end' | |
| )} | |
| > | |
| {secondaryAction ? ( | |
| <Chip | |
| variant={secondaryAction.variant ?? 'filled'} | |
| flush | |
| onClick={secondaryAction.onClick} | |
| disabled={secondaryAction.disabled} | |
| > | |
| {secondaryAction.label} | |
| </Chip> | |
| ) : null} | |
| <div className='flex gap-2'> | |
| <Chip variant='filled' flush onClick={onCancel} disabled={cancelDisabled}> | |
| Cancel | |
| </Chip> | |
| <Chip | |
| variant={primaryAction.variant ?? 'primary'} | |
| flush | |
| onClick={primaryAction.onClick} | |
| disabled={primaryAction.disabled} | |
| > | |
| {primaryAction.label} | |
| </Chip> | |
| </div> | |
| </div> | |
| </div> | |
| ) | |
| } |
Summary
ChipModalFooterprops-driven:onCancel,primaryAction, and optionalsecondaryAction(each aChipModalFooterActionwithlabel/onClick/variant/disabled) replace hand-composed footer markup, so the modal owns its chrome as the single source of truthChipModalHeader.onCloserequired and always render the close button + inset divider, dropping the optionalonClose/showDividerbranches for one consistent headerChipModalFooterActionandChipModalFooterPropsfrom the emcn barrelType of Change
Testing
Tested manually. Opened representative modals (create/edit/delete confirmations, invite, deploy, API key, knowledge base) and verified footer actions, cancel, close, destructive-variant flows, and secondary-action layout all render correctly.
bun run lint:check,tsc --noEmit, andbun run check:api-validationclean on touched files.Checklist