Skip to content

Commit df4263b

Browse files
committed
fix(settings): wire flush, drop the lone skeleton, close review gaps
Final review round. Two fixes the previous commit claimed but did not land: `CredentialDetailHeading` never passed `flush`, so every credential detail heading wore list-row padding, and the empty-subtitle guard was never applied. Both were scripted replacements that silently no-op'd. RESOURCE_LIST_GRID also dropped to one column 32px earlier than the grid it replaced — `auto-fit` measures tracks, not margin boxes, so widening the gap for the rows' bleed moved the breakpoint. Track minimum now budgets for it. Removes the BYOK skeleton rather than maintaining a second copy of the row: it was the only skeleton in settings, and it had already desynced from the row it imitates. Its peers render nothing while loading. Also: unify the glyph-tile treatment across MCP/sandboxes/workflow-MCP with custom tools; move decoration out of `trailing` in verified-domains and recently-deleted; convert the last hand-rolled row and empty states in workflow-MCP, api-keys, copilot and group-detail; give copilot's delete the same `...` affordance as api-keys; announce the row description via aria-describedby, which the stretched overlay had silenced; and let SettingsField render its own value so callers stop restating type tokens. Docs: correct claims that predate this PR — `aside` does not exist, the navigation source of truth is under components/, beforeunload mounts in the layouts, `getSettingsSectionMeta` takes two args — and stop asserting a literal-pixel grep returns zero when display type legitimately uses it.
1 parent 7fd9eba commit df4263b

18 files changed

Lines changed: 425 additions & 415 deletions

File tree

.claude/rules/sim-settings-pages.md

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Do NOT hand-roll any of these in a settings page — they are owned by the layou
2323
shell (fed through `SettingsPanel`):
2424

2525
- `<div className='flex h-full flex-col bg-[var(--bg)]'>` shell
26-
- the header bar (`flex flex-shrink-0 … px-[16px] pt-[8.5px] pb-[8.5px]`)
26+
- the header bar — compose `PAGE_HEADER_BAR` (`@/components/page-header-bar`); never rewrite its padding
2727
- the scroll container (`min-h-0 flex-1 overflow-y-auto px-6 [scrollbar-gutter:stable_both-edges]`)
2828
- the content column (`mx-auto … max-w-[48rem] … gap-7`)
2929
- a title block (`<h1 className='font-medium text-[var(--text-body)] text-lg'>` + `<p className='text-[var(--text-muted)] text-md'>`)
@@ -58,21 +58,23 @@ return (
5858
## `SettingsPanel` props
5959

6060
- `actions?: SettingsAction[]` — right-aligned header chips, **data only**:
61-
`{ text, icon?, variant?: 'primary'|'destructive', active?, onSelect, disabled?, tooltip? }`.
61+
`{ id?, text, textTone?: 'error', icon?, variant?: 'primary'|'destructive', active?,
62+
onSelect, onPrefetch?, disabled?, tooltip? }`.
6263
The shell renders each as a `Chip` — never pass JSX, a `<div>`, or `className`
6364
(the locked contract: it's structurally impossible to vibe-code a padding
6465
change). Multiple/conditional actions are a plain array
6566
(`[...(canManage ? [{…}] : []), …]`). Labels are **sentence case** (`Add override`,
6667
not `Add Override`). A disabled action that needs to explain itself sets
67-
`tooltip` (the shell renders the hover tooltip, disabled chip included) — never
68-
hand-roll a tooltip-wrapped chip in `aside`. Save/Discard pairs come from the
69-
`saveDiscardActions()` helper (spread it into `actions`). Only a widget that
70-
genuinely cannot be a chip (e.g. one needing hover-prefetch) goes in `aside`.
68+
`tooltip` (the shell renders the hover tooltip, disabled chip included). An action
69+
that wants to warm a route on hover sets `onPrefetch`; the shell wires it. A label
70+
that flips while pending (`Delete``Deleting...`) sets a stable `id`, or the chip
71+
remounts mid-action. Save/Discard pairs come from the `saveDiscardActions()`
72+
helper (spread it into `actions`).
7173
- `back?: SettingsBackAction` (`{ text, icon?, onSelect }`) — left-aligned back
7274
chip for a **detail sub-view** (e.g. a selected MCP server, a permission group,
7375
a retention policy). Detail sub-views render through `SettingsPanel` like list
7476
pages — they do NOT hand-roll their own shell.
75-
- `aside?: ReactNode`escape hatch for the rare non-chip header widget. Keep it rare.
77+
- `docsLink?: string`renders the header's `Docs` `ChipLink`.
7678
- `search?: { value; onChange: (value: string) => void; placeholder?; disabled? }`
7779
renders the canonical search field directly below the title. Pass `setSearchTerm`
7880
straight to `onChange`. Use this for a standalone search; if search shares a row
@@ -85,15 +87,15 @@ return (
8587

8688
## Title + description live in navigation metadata
8789

88-
`apps/sim/app/workspace/[workspaceId]/settings/navigation.ts` is the single source
89-
of truth. Every `NavigationItem` carries a one-line `description`; `SettingsPanel`
90-
resolves both via `getSettingsSectionMeta(section)` and the
90+
`apps/sim/components/settings/navigation.ts` is the single source of truth (the
91+
`settings/navigation.ts` in the route tree is only a re-export shim). Every `NavigationItem` carries a one-line `description`; `SettingsPanel`
92+
resolves both via `getSettingsSectionMeta(plane, section)` and the
9193
`SettingsSectionProvider` the settings shell wraps around the active section.
9294

9395
Adding a new settings page:
9496

95-
1. Add the `SettingsSection` id + a `NavigationItem` (with `label` **and**
96-
`description`) in `navigation.ts`. Keep descriptions verb-first, one line,
97+
1. Add the section id to the `UnifiedSettingsSection` union + a `NavigationItem`
98+
(with `label` **and** `description`) in `components/settings/navigation.ts`. Keep descriptions verb-first, one line,
9799
~40–55 chars, in the product voice (see `.claude/rules/constitution.md`).
98100
2. Render the component inside the shell's `effectiveSection` switch in
99101
`settings/[section]/settings.tsx`.
@@ -148,8 +150,8 @@ down the **row title/subtitle** shape above, not every text element on every pag
148150
**`SettingsResourceRow`** (`…/components/settings-resource-row`) is *the* list row
149151
for every settings resource — and for skills, integrations, and the `ee/` surfaces
150152
too. It owns the tile, the title/subtitle tokens, the row padding and bleed
151-
(`-mx-2 … rounded-lg p-2`), the hover band, the hit area, the focus ring, and the
152-
navigation chevron. Never hand-roll any of it, and never wrap the row in your own
153+
(`-mx-2 … rounded-lg p-2`), the hit area, the focus ring, the navigation chevron,
154+
and — on activatable rows only — the hover band. Never hand-roll any of it, and never wrap the row in your own
153155
`<button>` or `<Link>` — that is what `onClick`/`href` are for.
154156

155157
```tsx
@@ -183,7 +185,8 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
183185
- `navigable` — appends the one canonical chevron. Set it on rows that open a
184186
detail page; leave it off when `onClick` acts in place (revealing a folder).
185187
Never import an arrow yourself: `lucide-react` and `@sim/emcn/icons` ship
186-
visibly different glyphs, and the row already picked one.
188+
visibly different glyphs, and the row already picked one. A sanctioned bespoke
189+
row (below) draws it with `RESOURCE_ROW_ARROW_CLASSES` from the same module.
187190
- `trailing` vs `badge``trailing` is for **interactive** controls (a `Chip`, a
188191
`RowActionsMenu`) and sits above the hit area. `badge` is for **decoration** (a
189192
status tag) and is click-through. Putting a badge in `trailing` turns the row's
@@ -211,7 +214,8 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
211214

212215
## Other shared settings primitives (do not re-roll these)
213216

214-
- **`SettingsSection`** (`…/components/settings-section`) — muted label, hairline
217+
- **`SettingsSection`** (`…/components/settings-section/settings-section` — this
218+
directory has no barrel) — muted label, hairline
215219
divider, body. Also carries `headerAccessory` and `action` slots. Never
216220
re-derive the label/divider chrome; `sim-styling.md` owns those tokens.
217221
- **`SettingsField`** (`…/components/settings-field`) — a read-only label/value
@@ -231,8 +235,10 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
231235
hand-roll the `<DropdownMenu>` + `<MoreHorizontal>` trigger per page.
232236
- **`RESOURCE_TILE_BASE`** + one of `RESOURCE_TILE_FILL` / `RESOURCE_TILE_PLAIN`
233237
(`app/workspace/[workspaceId]/components/resource-tile` — note: *not* under
234-
`settings/`, unlike the other `…/` paths on this page) — the 36px tile chrome, for the rare tile
235-
outside a row (a detail heading). `ResourceTile` wraps the filled pairing.
238+
`settings/`, unlike the other `…/` paths on this page) — the 36px tile chrome, for
239+
any tile the row does not draw itself: a detail heading, or a caller-supplied
240+
`iconVariant='custom'` tile. `ResourceTile` wraps the filled pairing. Use
241+
`RESOURCE_TILE_FILL` for a glyph, `RESOURCE_TILE_PLAIN` for a brand logo or favicon.
236242

237243

238244
**Member avatars are deliberately two components, not one.** `member-list.tsx`
@@ -295,8 +301,9 @@ shells. Reach for it before hand-rolling a `Chip`.
295301
(from `@/app/workspace/[workspaceId]/components/credential-detail`). The
296302
in-view header **Discard** chip (via `SaveDiscardActions onDiscard`) is a
297303
*reset to original* — distinct from the back-confirm's discard, which leaves.
298-
- **`useSettingsBeforeUnload`** is mounted **once** in the settings shell
299-
(`settings/[section]/settings.tsx`) — never add a per-page `beforeunload`.
304+
- **`useSettingsBeforeUnload`** is mounted by the settings shells
305+
(`settings/layout.tsx` and `components/settings/standalone-settings-shell.tsx`) —
306+
never add a per-page `beforeunload`.
300307
- **Dirty *computation* stays local** (shapes differ: field-compare vs
301308
normalize+stringify) — only how dirty is *consumed* is shared. Derive it (a
302309
`const`/`useMemo`), never store it in `useState`.
@@ -333,7 +340,7 @@ A settings page is design-system-clean when:
333340
- [ ] If it has editable state: Save/Discard go through `SaveDiscardActions`, dirty is wired via `useSettingsUnsavedGuard` (called before any early-return gate), and there is **no** hand-rolled Save button / `beforeunload` / "Unsaved changes" modal.
334341
- [ ] No business logic, handlers, or conditional rendering changed by the migration — except where the shared primitive makes a gate structural (a permission gate becomes `onClick={can ? … : undefined}` + `navigable={can}`, which renders a plain non-interactive row).
335342
- [ ] No literal `text-[Npx]` classes — named scale tokens only (see "Text-scale tokens" above).
336-
- [ ] Every **resource** list row (a thing with an identity — a tool, a server, a key, a credential) is a `SettingsResourceRow` in a `RESOURCE_LIST_STACK`/`RESOURCE_LIST_GRID` — no wrapper `<button>`/`<Link>`, no hand-passed arrow, no re-derived title/subtitle spans. Rows with a genuinely different shape stay bespoke: multi-line bodies (inbox tasks), tabular columns (billing invoices, credit usage), and grids (secrets).
343+
- [ ] Every **resource** list row (a thing with an identity — a tool, a server, a key, a credential) is a `SettingsResourceRow` in a `RESOURCE_LIST_STACK`/`RESOURCE_LIST_GRID` — no wrapper `<button>`/`<Link>`, no hand-passed arrow, no re-derived title/subtitle spans. Rows with a genuinely different shape stay bespoke, and draw their own arrow with `RESOURCE_ROW_ARROW_CLASSES`: multi-line bodies (inbox tasks), tabular columns (billing invoices, credit usage), grids (secrets), and the member rows (see the avatar note above).
337344
- [ ] Rows that open a detail page use `navigable` + `clickLabel`; flat records use `RowActionsMenu`. Not both.
338345
- [ ] Decorative trailing content is in `badge`, not `trailing`.
339346
- [ ] Labeled sections use `SettingsSection`; read-only fields use `SettingsField`; empty/loading/error use `SettingsEmptyState`.

.claude/skills/add-settings-page/SKILL.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ skill is the procedure.
1212

1313
Key paths:
1414
- Layout primitive: `apps/sim/app/workspace/[workspaceId]/settings/components/settings-panel/settings-panel.tsx`
15-
- Nav metadata (titles + descriptions): `apps/sim/app/workspace/[workspaceId]/settings/navigation.ts`
15+
- Nav metadata (titles + descriptions): `apps/sim/components/settings/navigation.ts`
1616
- Section switch + provider: `apps/sim/app/workspace/[workspaceId]/settings/[section]/settings.tsx`
1717
- Pages: `apps/sim/app/workspace/[workspaceId]/settings/components/<name>/<name>.tsx` and EE pages under `apps/sim/ee/<feature>/components/`
1818

@@ -53,7 +53,9 @@ For each page component, confirm the checklist in `.claude/rules/sim-settings-pa
5353
3. Find literal pixel text sizes (should be 0 — see "Text-scale tokens" in
5454
`.claude/rules/sim-settings-pages.md` for the token map and the row
5555
title/subtitle pairing convention):
56-
`git grep -nE "text-\[[0-9]+px\]" -- 'apps/sim/**/settings/' 'apps/sim/ee/'`
56+
`git grep -nE "text-\[1[0-8]px\]" -- 'apps/sim/**/settings/' 'apps/sim/ee/'` — should
57+
be 0. Display type above the scale (`text-[40px]` hero headings, the `text-[8px]`
58+
member-avatar initial) is deliberate and out of scope.
5759
4. Confirm each page imports `SettingsPanel` and that its `NavigationItem` has an
5860
accurate `description` of consistent length with its peers.
5961
- Editable pages: confirm Save/Discard go through `SaveDiscardActions` and

apps/sim/app/workspace/[workspaceId]/components/credential-detail/components/credential-detail-heading.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@ export function CredentialDetailHeading({
1919
subtitle,
2020
}: CredentialDetailHeadingProps) {
2121
return (
22-
<SettingsResourceRow iconVariant='custom' icon={leading} title={title} description={subtitle} />
22+
<SettingsResourceRow
23+
// A heading is not a list row: no bleed, no row padding.
24+
flush
25+
iconVariant='custom'
26+
icon={leading}
27+
title={title}
28+
// `''` rendered nothing before; the row only skips a nullish description.
29+
description={subtitle || undefined}
30+
/>
2331
)
2432
}

apps/sim/app/workspace/[workspaceId]/settings/components/api-keys/api-keys.tsx

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,14 +202,21 @@ export function ApiKeys({ scope = 'workspace' }: ApiKeysProps) {
202202
{showsWorkspaceKeys && !searchTerm.trim() ? (
203203
<SettingsSection label='Workspace'>
204204
{workspaceKeys.length === 0 ? (
205-
<div className='text-[var(--text-muted)] text-sm'>No workspace API keys yet</div>
205+
<SettingsEmptyState variant='inline'>
206+
No workspace API keys yet
207+
</SettingsEmptyState>
206208
) : (
207209
<div className={RESOURCE_LIST_STACK}>
208210
{workspaceKeys.map((key) => (
209211
<SettingsResourceRow
210212
key={key.id}
211213
title={key.name}
212-
description={`${key.displayKey} · last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
214+
description={key.displayKey}
215+
badge={
216+
<span className='whitespace-nowrap text-[var(--text-muted)] text-caption'>
217+
{`last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
218+
</span>
219+
}
213220
trailing={
214221
<ApiKeyRowMenu
215222
keyName={key.name}
@@ -232,7 +239,12 @@ export function ApiKeys({ scope = 'workspace' }: ApiKeysProps) {
232239
<SettingsResourceRow
233240
key={key.id}
234241
title={key.name}
235-
description={`${key.displayKey} · last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
242+
description={key.displayKey}
243+
badge={
244+
<span className='whitespace-nowrap text-[var(--text-muted)] text-caption'>
245+
{`last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
246+
</span>
247+
}
236248
trailing={
237249
<ApiKeyRowMenu
238250
keyName={key.name}
@@ -258,7 +270,12 @@ export function ApiKeys({ scope = 'workspace' }: ApiKeysProps) {
258270
<div key={key.id} className='flex flex-col'>
259271
<SettingsResourceRow
260272
title={key.name}
261-
description={`${key.displayKey} · last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
273+
description={key.displayKey}
274+
badge={
275+
<span className='whitespace-nowrap text-[var(--text-muted)] text-caption'>
276+
{`last used ${formatLastUsed(key.lastUsed).toLowerCase()}`}
277+
</span>
278+
}
262279
trailing={
263280
<ApiKeyRowMenu
264281
keyName={key.name}

apps/sim/app/workspace/[workspaceId]/settings/components/browser/components/password-detail/password-detail.tsx

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -145,16 +145,14 @@ export function PasswordDetail({ credential, onBack, onForgotten }: PasswordDeta
145145
<SettingsField label='Site'>
146146
<div className='flex items-center gap-2.5'>
147147
<div className={cn(RESOURCE_TILE_BASE, RESOURCE_TILE_PLAIN)}>
148-
<div className='flex size-full items-center justify-center'>
149-
{credential.icon ? (
150-
// A `data:` URL copied from the source browser at import
151-
// time — never a network request, which would disclose
152-
// which sites the user has passwords for.
153-
<img src={credential.icon} alt='' className='size-full object-contain' />
154-
) : (
155-
<Key className='size-5 text-[var(--text-icon)]' />
156-
)}
157-
</div>
148+
{credential.icon ? (
149+
// A `data:` URL copied from the source browser at import
150+
// time — never a network request, which would disclose
151+
// which sites the user has passwords for.
152+
<img src={credential.icon} alt='' className='size-full object-contain' />
153+
) : (
154+
<Key className='size-5 text-[var(--text-icon)]' />
155+
)}
158156
</div>
159157
<ChipCopyInput value={credential.origin} copyLabel='Copy site' />
160158
</div>

apps/sim/app/workspace/[workspaceId]/settings/components/byok/byok-key-manager.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import {
2121
CHIP_FIELD_SHELL,
2222
} from '@/app/workspace/[workspaceId]/components/credential-detail/components/chip-field'
2323
import { BYOKProviderKeysModal } from '@/app/workspace/[workspaceId]/settings/components/byok/byok-provider-keys-modal'
24-
import { BYOKKeySkeleton } from '@/app/workspace/[workspaceId]/settings/components/byok/byok-skeleton'
2524
import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state'
2625
import {
2726
RESOURCE_LIST_STACK,
@@ -324,13 +323,7 @@ export function BYOKKeyManager(props: BYOKKeyManagerProps) {
324323

325324
{description && <p className='text-[var(--text-secondary)] text-sm'>{description}</p>}
326325

327-
{isLoading ? (
328-
<div className={RESOURCE_LIST_STACK}>
329-
{providers.map((p) => (
330-
<BYOKKeySkeleton key={p.id} />
331-
))}
332-
</div>
333-
) : showNoResults ? (
326+
{isLoading ? null : showNoResults ? (
334327
<SettingsEmptyState variant='inline'>
335328
No providers found matching "{searchTerm}"
336329
</SettingsEmptyState>

apps/sim/app/workspace/[workspaceId]/settings/components/byok/byok-skeleton.tsx

Lines changed: 0 additions & 22 deletions
This file was deleted.

0 commit comments

Comments
 (0)