improvement(settings): one header action order across detail pages

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.

Author: waleedlatif1

.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 `<SaveDiscardChips>` 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 `<Chip>`; 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.

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,
           },

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,
                 },

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),
                 },

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,
                   },

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,
                       },

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)
+  })
+})

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(
+      <SettingsHeaderProvider>
+        <SettingsHeaderShell>
+          <SettingsPanel title='Thing' actions={actions}>
+            <div />
+          </SettingsPanel>
+        </SettingsHeaderShell>
+      </SettingsHeaderProvider>
+    )
+  })
+}
+
+/** 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()
+  })
+})