From 2cdda6ce505bafbd9cacaf1552fa1f4a2561a85a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 3 Aug 2026 10:12:25 -0700 Subject: [PATCH] improvement(settings): one header action order across detail pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detail headers disagreed on where Delete sits. The skills page reads `Share → Delete → Discard → Save`, but every SettingsPanel page spread saveDiscardActions() first and appended Delete, rendering it to the RIGHT of the primary chip: sandboxes, custom tools, custom blocks, permission groups and data retention all did this. Fixed in the shell rather than at nine callsites. orderHeaderActions() ranks actions — secondary, then `id:'discard'`, then `variant:'primary'` — stably within each band, so writing the array the natural way now produces the right header and a new detail page cannot get it wrong. This generalizes past Save: workflow MCP servers' `Add workflows` primary is now right-most with Delete before it, instead of the reverse. The ranking has to survive one trap. The shell routes onSelect through configRef.current.actions[index] to avoid stale closures, so reordering the render without preserving the source index would bind every chip to the wrong handler — clicking Delete would Save. orderHeaderActions carries {action,index} pairs; settings-header-shell.test.tsx pins that at the render level, including the conditional-Discard case where a missing action shifts every index. Delete is also now a plain chip on the nine resource-detail headers, matching skills, each with a stable `id:'delete'` (three lacked one, so the chip remounted when its label flipped to Deleting...). `variant:'destructive'` is kept for actions destructive at scale — Delete all passwords, Clear all browsing data, Sign out all members — which the confirm modal does not cover the way it covers removing the single resource you are looking at. --- .claude/rules/sim-settings-pages.md | 44 +++++- .../password-detail/password-detail.tsx | 2 +- .../custom-tool-detail/custom-tool-detail.tsx | 1 - .../settings/components/mcp/mcp.tsx | 1 - .../components/sandboxes/sandboxes.tsx | 1 - .../workflow-mcp-servers.tsx | 1 - .../settings/settings-header-order.test.ts | 129 ++++++++++++++++++ .../settings/settings-header-shell.test.tsx | 116 ++++++++++++++++ .../components/settings/settings-header.tsx | 34 ++++- .../components/group-detail.tsx | 2 +- .../components/custom-block-detail.tsx | 2 +- .../components/data-drain-detail.tsx | 2 +- .../components/data-retention-settings.tsx | 2 +- 13 files changed, 321 insertions(+), 16 deletions(-) create mode 100644 apps/sim/components/settings/settings-header-order.test.ts create mode 100644 apps/sim/components/settings/settings-header-shell.test.tsx diff --git a/.claude/rules/sim-settings-pages.md b/.claude/rules/sim-settings-pages.md index 114bc1f30a8..7438f5aaf5e 100644 --- a/.claude/rules/sim-settings-pages.md +++ b/.claude/rules/sim-settings-pages.md @@ -247,12 +247,46 @@ the email is the primary content; `components/permissions/member-row.tsx` render a 36px `getUserColor`-hashed avatar for member *management* rows that carry a name, an email, and a role control. Same shape, different job — do not merge them. +## Header action order + +Every detail header reads left→right: + +``` +← Back [secondary actions] → Delete → Discard → Save +``` + +You do not have to get the array order right — `orderHeaderActions()` ranks them +(secondary → `id:'discard'` → `variant:'primary'`), order-stable within each +band, so spreading `saveDiscardActions()` first still renders Save last. Both +action stacks apply it: `SettingsHeaderShell` and `SettingsActionChips`. +Covered by `settings-header-order.test.ts` and `settings-header-shell.test.tsx` +— the latter pins that a reordered chip still routes to its own handler. + +Two consequences worth knowing: + +- **The primary chip is always right-most**, and it is not always Save — on a + page with no save state it is whatever the primary action is (`Add workflows`, + `Import`). Delete still precedes it. +- `CredentialDetailLayout` takes a `ReactNode`, so the chips you write directly + are in your order — only what you route through `SettingsActionChips` / + `SaveDiscardChips` is ranked. Put `` last (skills, secrets, + connected credentials already do). + ## Deleting a resource -Delete lives in the **detail header**, as -`{ text: 'Delete', variant: 'destructive', onSelect: … }` behind a -`ChipConfirmModal` — never `textTone: 'error'`, never a bare `Chip`, and never -unconfirmed. A list row does not carry Delete when the resource has a detail page. +Delete lives in the **detail header**, as `{ id: 'delete', text: 'Delete', +onSelect: … }` behind a `ChipConfirmModal` — a **plain chip**, never +`textTone: 'error'`, and never unconfirmed. In a `SettingsPanel` header it is +action *data*, never a hand-rolled ``; only `CredentialDetailLayout` +surfaces, which take a `ReactNode`, render one directly. Always set `id: 'delete'`; without a +stable id the chip remounts when the label flips to `Deleting...`. + +`variant: 'destructive'` is reserved for actions that are destructive at +**scale** — `Delete all` passwords, `Clear all` browsing data, `Sign out all +members`. Removing the single resource you are already looking at is confirmed +by the modal, so it does not also need a red chip. + +A list row does not carry Delete when the resource has a detail page. ## Save / Discard + unsaved-changes guard @@ -344,5 +378,5 @@ A settings page is design-system-clean when: - [ ] Rows that open a detail page use `navigable` + `clickLabel`; flat records use `RowActionsMenu`. Not both. - [ ] Decorative trailing content is in `badge`, not `trailing`. - [ ] Labeled sections use `SettingsSection`; read-only fields use `SettingsField`; empty/loading/error use `SettingsEmptyState`. -- [ ] Delete is a `destructive` header action behind a `ChipConfirmModal`. +- [ ] Delete is a plain `id:'delete'` header action behind a `ChipConfirmModal`; `destructive` is reserved for bulk actions. - [ ] `tsc`, `biome`, and the page's tests pass. diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/browser/components/password-detail/password-detail.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/browser/components/password-detail/password-detail.tsx index 90091fb5f00..7a805605904 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/browser/components/password-detail/password-detail.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/browser/components/password-detail/password-detail.tsx @@ -133,8 +133,8 @@ export function PasswordDetail({ credential, onBack, onForgotten }: PasswordDeta description='Saved on this device, encrypted. Chat can never read, choose, or type it.' actions={[ { + id: 'delete', text: 'Forget', - variant: 'destructive' as const, onSelect: () => setConfirmingForget(true), disabled: busy, }, diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx index 653bc066b94..809feeeb408 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx @@ -221,7 +221,6 @@ export function CustomToolDetail({ { id: 'delete', text: deleteTool.isPending ? 'Deleting...' : 'Delete', - variant: 'destructive' as const, onSelect: () => setShowDeleteConfirm(true), disabled: deleteTool.isPending, }, diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 0c886ebf438..04158338a79 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -448,7 +448,6 @@ export function MCP() { { id: 'delete', text: deletingServers.has(server.id) ? 'Deleting...' : 'Delete', - variant: 'destructive' as const, onSelect: () => handleRemoveServer(server.id), disabled: deletingServers.has(server.id), }, diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx index ed393de09a6..c8ab16b44ff 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx @@ -217,7 +217,6 @@ export function Sandboxes() { { id: 'delete', text: deleteSandbox.isPending ? 'Deleting...' : 'Delete', - variant: 'destructive' as const, onSelect: () => setShowDeleteConfirm(true), disabled: deleteSandbox.isPending, }, diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx index 1a7accd3634..53e4a100ec0 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx @@ -411,7 +411,6 @@ function ServerDetailView({ { id: 'delete', text: isDeleting ? 'Deleting...' : 'Delete', - variant: 'destructive' as const, onSelect: onDelete, disabled: isDeleting, }, diff --git a/apps/sim/components/settings/settings-header-order.test.ts b/apps/sim/components/settings/settings-header-order.test.ts new file mode 100644 index 00000000000..a648c32d42a --- /dev/null +++ b/apps/sim/components/settings/settings-header-order.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, it } from 'vitest' +import { saveDiscardActions } from '@/components/settings/save-discard-actions' +import type { SettingsAction } from '@/components/settings/settings-header' +import { orderHeaderActions } from '@/components/settings/settings-header' + +const noop = () => {} + +/** Labels in the order the header renders them. */ +function rendered(actions: SettingsAction[]): string[] { + return orderHeaderActions(actions).map(({ action }) => action.text) +} + +const save = (dirty: boolean) => + saveDiscardActions({ dirty, saving: false, onSave: noop, onDiscard: noop }) + +describe('orderHeaderActions', () => { + it('puts Delete before Discard and Save no matter how the caller ordered them', () => { + // The natural way to write this array — Save/Discard first, then Delete — + // is what every settings detail page did, and it rendered Delete to the + // right of the primary chip. + const actions: SettingsAction[] = [ + ...save(true), + { id: 'delete', text: 'Delete', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Delete', 'Discard', 'Save']) + }) + + it('matches the skills detail header: secondary actions, then Delete, then Save', () => { + const actions: SettingsAction[] = [ + { text: 'Share', onSelect: noop }, + { id: 'delete', text: 'Delete', onSelect: noop }, + ...save(true), + ] + + expect(rendered(actions)).toEqual(['Share', 'Delete', 'Discard', 'Save']) + }) + + it('keeps Save right-most when there is nothing to discard', () => { + const actions: SettingsAction[] = [ + ...save(false), + { id: 'delete', text: 'Delete', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Delete', 'Save']) + }) + + it('sends any primary action to the end, not just Save', () => { + // Workflow MCP servers: Add workflows is the primary, Delete must precede it. + const actions: SettingsAction[] = [ + { text: 'Edit server', onSelect: noop }, + { text: 'Add workflows', variant: 'primary', onSelect: noop }, + { id: 'delete', text: 'Delete', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Edit server', 'Delete', 'Add workflows']) + }) + + it('preserves caller order within a band', () => { + const actions: SettingsAction[] = [ + { text: 'Refresh', onSelect: noop }, + { text: 'Edit', onSelect: noop }, + { id: 'delete', text: 'Delete', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Refresh', 'Edit', 'Delete']) + }) + + it('leaves a destructive bulk action left of the primary', () => { + // Passwords: `Delete all` is destructive but must not outrank `Import`. + // This is what keeps a red chip from becoming the right-most control. + const actions: SettingsAction[] = [ + { text: 'Delete all', variant: 'destructive', onSelect: noop }, + { text: 'Import', variant: 'primary', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Delete all', 'Import']) + }) + + it('ranks a destructive action alongside secondary ones, not after Discard', () => { + const actions: SettingsAction[] = [ + ...save(true), + { text: 'Sign out all members', variant: 'destructive', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Sign out all members', 'Discard', 'Save']) + }) + + it('treats primary as the stronger signal when an action is both', () => { + const actions: SettingsAction[] = [ + { text: 'Other', onSelect: noop }, + { id: 'discard', text: 'Odd', variant: 'primary', onSelect: noop }, + ] + + expect(rendered(actions)).toEqual(['Other', 'Odd']) + }) + + it('carries each action original index so ref-routed handlers stay bound', () => { + const actions: SettingsAction[] = [ + ...save(true), // indices 0 (Discard), 1 (Save) + { id: 'delete', text: 'Delete', onSelect: noop }, // index 2 + ] + + expect(orderHeaderActions(actions).map(({ action, index }) => [action.text, index])).toEqual([ + ['Delete', 2], + ['Discard', 0], + ['Save', 1], + ]) + }) + + it('tolerates an absent or empty action list', () => { + expect(orderHeaderActions(undefined)).toEqual([]) + expect(orderHeaderActions([])).toEqual([]) + }) + + it('does not mutate the caller array', () => { + // The shell sorts a prop read off a live ref; reordering it in place would + // renumber the indices the handlers are routed through. + const actions: SettingsAction[] = [ + ...save(true), + { id: 'delete', text: 'Delete', onSelect: noop }, + ] + const before = actions.map((a) => a.text) + + orderHeaderActions(actions) + + expect(actions.map((a) => a.text)).toEqual(before) + }) +}) diff --git a/apps/sim/components/settings/settings-header-shell.test.tsx b/apps/sim/components/settings/settings-header-shell.test.tsx new file mode 100644 index 00000000000..bca2da37694 --- /dev/null +++ b/apps/sim/components/settings/settings-header-shell.test.tsx @@ -0,0 +1,116 @@ +/** + * @vitest-environment jsdom + * + * The shell renders header actions in ranked order but routes every handler + * through `configRef.current.actions[index]` to dodge stale closures. Those two + * facts fight each other: if the reordered render ever renumbered the indices, + * clicking Delete would invoke Save. These tests pin the pairing at the render + * level, which the pure-function tests cannot reach. + */ +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { saveDiscardActions } from '@/components/settings/save-discard-actions' +import type { SettingsAction } from '@/components/settings/settings-header' +import { SettingsHeaderProvider, SettingsHeaderShell } from '@/components/settings/settings-header' +import { SettingsPanel } from '@/components/settings/settings-panel' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +let container: HTMLDivElement +let root: Root + +beforeEach(() => { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() + vi.clearAllMocks() +}) + +function renderHeader(actions: SettingsAction[]) { + act(() => { + root.render( + + + +
+ + + + ) + }) +} + +/** Header chips in rendered (left→right) order. */ +function chipLabels(): string[] { + return [...container.querySelectorAll('header button, div button')] + .map((node) => node.textContent?.trim() ?? '') + .filter(Boolean) +} + +function clickChip(label: string) { + const chip = [...container.querySelectorAll('button')].find( + (node) => node.textContent?.trim() === label + ) + if (!chip) throw new Error(`no chip labelled "${label}" (have: ${chipLabels().join(', ')})`) + act(() => { + chip.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) +} + +describe('SettingsHeaderShell action routing', () => { + it('renders Delete before Discard and Save even though the array lists it last', () => { + const actions: SettingsAction[] = [ + ...saveDiscardActions({ dirty: true, saving: false, onSave: vi.fn(), onDiscard: vi.fn() }), + { id: 'delete', text: 'Delete', onSelect: vi.fn() }, + ] + + renderHeader(actions) + + const labels = chipLabels() + expect(labels.indexOf('Delete')).toBeLessThan(labels.indexOf('Discard')) + expect(labels.indexOf('Discard')).toBeLessThan(labels.indexOf('Save')) + }) + + it('invokes the action that was clicked, not the one at that render position', () => { + const onSave = vi.fn() + const onDiscard = vi.fn() + const onDelete = vi.fn() + + renderHeader([ + ...saveDiscardActions({ dirty: true, saving: false, onSave, onDiscard }), + { id: 'delete', text: 'Delete', onSelect: onDelete }, + ]) + + // Delete renders first but lives at source index 2. + clickChip('Delete') + expect(onDelete).toHaveBeenCalledTimes(1) + expect(onSave).not.toHaveBeenCalled() + expect(onDiscard).not.toHaveBeenCalled() + + clickChip('Save') + expect(onSave).toHaveBeenCalledTimes(1) + expect(onDelete).toHaveBeenCalledTimes(1) + }) + + it('stays correctly bound when a conditional action shifts every index', () => { + // Sandboxes: Discard only exists while dirty, so Delete moves 2 -> 1. + const onSave = vi.fn() + const onDelete = vi.fn() + + renderHeader([ + ...saveDiscardActions({ dirty: false, saving: false, onSave, onDiscard: vi.fn() }), + { id: 'delete', text: 'Delete', onSelect: onDelete }, + ]) + + clickChip('Delete') + + expect(onDelete).toHaveBeenCalledTimes(1) + expect(onSave).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/components/settings/settings-header.tsx b/apps/sim/components/settings/settings-header.tsx index 9093b4d70b4..c194d435b23 100644 --- a/apps/sim/components/settings/settings-header.tsx +++ b/apps/sim/components/settings/settings-header.tsx @@ -73,6 +73,9 @@ function computeSignature(config: SettingsHeaderConfig): string { back: config.back ? [config.back.text, config.back.icon ? 1 : 0] : null, actions: config.actions?.map((action) => [ action.text, + // `id` participates in ordering, so a config that changes only the id + // must still re-render — the sort key cannot be wider than the signature. + action.id ?? '', action.textTone ?? '', action.variant ?? '', action.active ?? false, @@ -180,13 +183,40 @@ export function SettingsActionChip({ export function SettingsActionChips({ actions }: { actions: SettingsAction[] }) { return ( <> - {actions.map((action) => ( + {orderHeaderActions(actions).map(({ action }) => ( ))} ) } +/** + * Every detail header reads left→right as + * `[secondary actions] → [Delete] → [Discard] → [Save]`. + * + * The shell enforces it rather than trusting callsites, because the natural way + * to write the array — spreading {@link saveDiscardActions} first, then adding a + * Delete — produces the opposite order and puts a destructive chip to the right + * of the primary one. Ranking is stable, so an action's position within its own + * band is still the caller's to choose. + * + * Pairs each action with its ORIGINAL index: the shell dereferences + * `actions[index]` on a live ref to dodge stale closures, so a reordered render + * must not renumber them. + */ +export function orderHeaderActions( + actions: SettingsAction[] | undefined +): { action: SettingsAction; index: number }[] { + const rank = (action: SettingsAction) => { + if (action.variant === 'primary') return 2 + if (action.id === 'discard') return 1 + return 0 + } + return (actions ?? []) + .map((action, index) => ({ action, index })) + .sort((a, b) => rank(a.action) - rank(b.action)) +} + export function SettingsHeaderShell({ children }: { children: ReactNode }) { const read = useContext(ReadContext) const configRef = read?.configRef @@ -209,7 +239,7 @@ export function SettingsHeaderShell({ children }: { children: ReactNode }) { Docs )} - {actions?.map((action, index) => ( + {orderHeaderActions(actions).map(({ action, index }) => ( setShowDeleteConfirm(true), disabled: deletePermissionGroup.isPending, }, diff --git a/apps/sim/ee/custom-blocks/components/custom-block-detail.tsx b/apps/sim/ee/custom-blocks/components/custom-block-detail.tsx index 94bd6bc3b73..d4d22822858 100644 --- a/apps/sim/ee/custom-blocks/components/custom-block-detail.tsx +++ b/apps/sim/ee/custom-blocks/components/custom-block-detail.tsx @@ -439,8 +439,8 @@ export function CustomBlockDetail({ blockId, workspaceId, onBack }: CustomBlockD ...(existing && canManageBlock ? [ { + id: 'delete', text: remove.isPending ? 'Deleting...' : 'Delete', - variant: 'destructive' as const, onSelect: () => { setShowDelete(true) // The warning must reflect the org's CURRENT usage, not a diff --git a/apps/sim/ee/data-drains/components/data-drain-detail.tsx b/apps/sim/ee/data-drains/components/data-drain-detail.tsx index dc7c65c291d..e8d5bcf68e0 100644 --- a/apps/sim/ee/data-drains/components/data-drain-detail.tsx +++ b/apps/sim/ee/data-drains/components/data-drain-detail.tsx @@ -150,8 +150,8 @@ export function DataDrainDetail({ organizationId, drain, onBack }: DataDrainDeta }, { text: 'Test connection', onSelect: handleTest, disabled: testDrain.isPending }, { + id: 'delete', text: 'Delete', - variant: 'destructive', onSelect: () => setShowDeleteConfirm(true), disabled: deleteDrain.isPending, }, diff --git a/apps/sim/ee/data-retention/components/data-retention-settings.tsx b/apps/sim/ee/data-retention/components/data-retention-settings.tsx index 8076adbd14b..81e49ceb12e 100644 --- a/apps/sim/ee/data-retention/components/data-retention-settings.tsx +++ b/apps/sim/ee/data-retention/components/data-retention-settings.tsx @@ -516,8 +516,8 @@ function PolicyDetail({ ...(canRemove ? [ { + id: 'delete', text: 'Remove override', - variant: 'destructive', onSelect: () => setShowRemoveConfirm(true), disabled: isSaving, } satisfies SettingsAction,