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 3 commits 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 makes
Confidence Score: 5/5Safe to merge — the refactor is mechanically correct across all 70 consumers, mutation guards are fully restored, and the remaining findings are visual regressions scoped to two UI elements. The refactor's core correctness is high: cancelDisabled is wired properly for every footer that previously disabled Cancel during a mutation, handleFormSubmit's optional-e signature change is correct for both onClick and onSubmit call sites, and the access-control unsaved-changes dialog rearrangement preserves all three outcomes (discard / go-back / save). The two open items are purely visual: an accidentally dropped className on the inbox trash Chip that removed its muted/red-hover styling, and the notifications 'Add' button losing its + icon because ChipModalFooterAction has no leftIcon field. Neither touches data flow or user-facing logic. inbox-settings-tab.tsx (accidental className removal from sender-list trash Chip) and notifications.tsx (lost leftIcon on the Add button) are the two files worth a quick visual check before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Consumer as Modal Consumer
participant Footer as ChipModalFooter
participant Cancel as Cancel Chip
participant Primary as Primary Chip
participant Secondary as Secondary Chip (opt.)
Consumer->>Footer: onCancel, primaryAction, secondaryAction?, cancelDisabled?
Footer->>Secondary: variant ?? 'filled', disabled?, onClick (left-docked)
Footer->>Cancel: "variant='filled', disabled=cancelDisabled"
Footer->>Primary: variant ?? 'primary', disabled?, onClick
Note over Cancel: Always rendered, always labelled 'Cancel'
Note over Secondary: Left-docked (justify-between) only when provided
Note over Primary: Right-anchored alongside Cancel
Reviews (2): Last reviewed commit: "fix(emcn): restore Cancel disable guard ..." | Re-trigger Greptile |
| 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> | |
| ) | |
| } |
Add `cancelDisabled` to ChipModalFooter and thread the pre-migration in-flight guards back into all 45 footers that had them, so destructive flows can no longer be dismissed mid-mutation.
|
Addressed the Cancel-during-mutation regression in
Not changed (by design):
Verified: @greptile review |
…l-migration # Conflicts: # apps/sim/app/workspace/[workspaceId]/w/components/sidebar/sidebar.tsx
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