Skip to content

Commit f381712

Browse files
committed
fix(settings): correct row bleed, delete-confirm binding, and avatar seeding
Follow-up from review of the consolidation. The sandbox delete confirmation was boolean-only state. Browser Back unmounts the modal without closing it, so opening another sandbox re-opened it already confirmed — against the wrong sandbox. Reset it where the draft guard already handles the same history hazard. RESOURCE_LIST_GRID kept an 8px column gap after the bleed moved onto the row, so neighbouring cards overlapped by exactly the gutter and the right-hand card's stretched hit area won it: clicking between two cards opened the wrong one. Budget the gap for the bleed instead. Also: restore the `group` the template-icon hover outlines depend on; add a `flush` row for headings and overflow boxes; seed MemberAvatar identically on every surface; keep the MCP transport label visible in all row states; give the Delete chip a stable id so it doesn't remount mid-delete; and stop an empty subtitle rendering a phantom line. Docs: widen the rule's path globs to the surfaces it now governs, drop the `text-[14px]` example that contradicted the token rule, and add a Mode C for migrating rows onto the primitive.
1 parent 80a01f0 commit f381712

11 files changed

Lines changed: 95 additions & 26 deletions

File tree

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

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
---
22
paths:
33
- "apps/sim/app/workspace/*/settings/**"
4+
- "apps/sim/app/workspace/*/{integrations,skills,upgrade}/**"
5+
- "apps/sim/app/workspace/*/components/{resource-tile,credential-detail}/**"
6+
- "apps/sim/components/{settings,permissions}/**"
47
- "apps/sim/ee/**/components/**"
58
---
69

@@ -173,8 +176,10 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
173176
`iconFill` lets an uploaded image reach the tile edge.
174177
- `onClick` / `href` — makes the **whole row** activatable via a stretched
175178
overlay. Prefer `href` when the destination is a route, so the row keeps
176-
prefetch, middle-click, and open-in-new-tab. `clickLabel` is the accessible
177-
name and is **required** alongside either: the overlay has no text of its own.
179+
prefetch, middle-click, and open-in-new-tab. Always pass `clickLabel` with
180+
either — the overlay holds no text, so it is the control's *only* accessible
181+
name. The prop is optional in the type (nothing enforces it), so omitting it
182+
ships a nameless button rather than failing the build.
178183
- `navigable` — appends the one canonical chevron. Set it on rows that open a
179184
detail page; leave it off when `onClick` acts in place (revealing a folder).
180185
Never import an arrow yourself: `lucide-react` and `@sim/emcn/icons` ship
@@ -187,9 +192,13 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
187192
containers. A `SettingsResourceRow` carries its own `-mx-2` bleed and padding,
188193
so a container holding one only sets rhythm: never add a second `-mx-2` (they
189194
stack into a 16px bleed) and never a different gap. A list of hand-rolled rows
190-
is the opposite — there the container owns the bleed. Note `-mx-2` inside a
191-
fixed-height `overflow-y-auto` box forces a horizontal scrollbar; drop the
192-
bleed there.
195+
is the opposite — there the container owns the bleed. A row inside a
196+
fixed-height `overflow-y-auto` box, or a heading that must line up with the
197+
section labels under it, passes `flush` to drop the bleed and padding.
198+
`RESOURCE_LIST_GRID` budgets its column gap for the bleed (24px of track gap
199+
minus 16px of bleed = an 8px gutter); narrowing that gap makes neighbouring
200+
rows — and their stretched hit areas — overlap, so a click in the gutter opens
201+
the wrong card.
193202

194203
**Three-dots vs. chevron** is not a taste call:
195204

@@ -221,7 +230,8 @@ navigation chevron. Never hand-roll any of it, and never wrap the row in your ow
221230
Conditional items become array spreads: `...(canManage ? [{…}] : [])`. Never
222231
hand-roll the `<DropdownMenu>` + `<MoreHorizontal>` trigger per page.
223232
- **`RESOURCE_TILE_BASE`** + one of `RESOURCE_TILE_FILL` / `RESOURCE_TILE_PLAIN`
224-
(`…/components/resource-tile`) — the 36px tile chrome, for the rare tile
233+
(`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
225235
outside a row (a detail heading). `ResourceTile` wraps the filled pairing.
226236
- **`MemberAvatar`** (`@/components/permissions/member-avatar`) — the one avatar
227237
for any member row.
@@ -316,7 +326,7 @@ A settings page is design-system-clean when:
316326
- [ ] Its `NavigationItem` has an accurate, consistent-length `description`.
317327
- [ ] Detail sub-views and entitlement/loading gates keep their own chrome (intentional).
318328
- [ ] 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.
319-
- [ ] No business logic, handlers, or conditional rendering changed by the migration.
329+
- [ ] 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).
320330
- [ ] No literal `text-[Npx]` classes — named scale tokens only (see "Text-scale tokens" above).
321331
- [ ] 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).
322332
- [ ] Rows that open a detail page use `navigable` + `clickLabel`; flat records use `RowActionsMenu`. Not both.

.claude/rules/sim-styling.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,13 @@ paths:
88

99
## Tailwind
1010

11-
1. **No inline styles** - Use Tailwind classes
11+
1. **No inline styles** - Use Tailwind classes. Exception: a genuinely dynamic
12+
value (a hashed avatar colour, a brand tile background) that cannot be a class.
1213
2. **No duplicate dark classes** - Skip `dark:` when value matches light mode
13-
3. **Exact values** - `text-[14px]`, `h-[26px]`
14+
3. **Exact values over approximations** - `h-[26px]`, not `h-6`. But **type size is
15+
always a named token** (`text-sm`, `text-caption`) — never `text-[14px]`, which
16+
sets font-size only and inherits a different line-height. See
17+
`sim-settings-pages.md` for the scale.
1418
4. **Transitions** - `transition-colors` for interactive states
1519

1620
## Conditional Classes

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ 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 -n "text-\[1[0-8]px\]" -- 'apps/sim/**/settings/' 'apps/sim/ee/'`
56+
`git grep -nE "text-\[[0-9]+px\]" -- 'apps/sim/**/settings/' 'apps/sim/ee/'`
5757
4. Confirm each page imports `SettingsPanel` and that its `NavigationItem` has an
5858
accurate `description` of consistent length with its peers.
5959
- Editable pages: confirm Save/Discard go through `SaveDiscardActions` and
@@ -77,3 +77,31 @@ For each page component, confirm the checklist in `.claude/rules/sim-settings-pa
7777
8. **Verify the whole sweep:** `tsc --noEmit`, `biome check` on every touched
7878
file, and run the affected pages' tests. Diff each file against the base and
7979
confirm the change is purely structural before shipping.
80+
81+
## Mode C — Migrate list rows to `SettingsResourceRow`
82+
83+
Read "The resource row" in `.claude/rules/sim-settings-pages.md` first — it is the
84+
contract. Then, per page:
85+
86+
1. Find hand-rolled rows:
87+
`git grep -n "truncate text-\[var(--text-body)\] text-sm" -- 'apps/sim/app/workspace/' 'apps/sim/ee/'`
88+
Every match outside `settings-resource-row.tsx` is either a row to migrate or a
89+
genuinely different shape (multi-line body, tabular columns, a grid) that stays
90+
bespoke — decide which, and say so.
91+
2. Replace the row *and* its wrapper: a `<button>`/`<Link>` around the row becomes
92+
`onClick`/`href` on the row itself. Wrapping the row is what the primitive
93+
exists to stop — it is also invalid HTML once `trailing` holds a control.
94+
3. Sort the trailing content: interactive → `trailing`, decorative → `badge`.
95+
Getting this backwards makes the row's right edge a dead zone.
96+
4. Add `navigable` only if the row opens a detail page, and `clickLabel` always.
97+
5. Drop the container's `-mx-2` — the row now owns the bleed. Use
98+
`RESOURCE_LIST_STACK` / `RESOURCE_LIST_GRID`; do not hand-write the gap.
99+
6. Unlike Mode B, this migration **may** change conditional rendering: a
100+
`<button disabled={!can}>` becomes `onClick={can ? … : undefined}` +
101+
`navigable={can}`, which renders a plain non-interactive row. Verify the gated
102+
state has no clickable affordance left.
103+
7. Check what the old row rendered *beside* the title (a badge, a timestamp, a
104+
transport label). The row's title truncates as one unit, so anything folded
105+
into it can be ellipsised away — move it to `description` or `badge`.
106+
8. Verify: `tsc --noEmit`, `biome check`, the page's tests, and a diff read of
107+
every converted block for lost props, conditions, and `key` placement.

apps/sim/app/workspace/[workspaceId]/settings/components/custom-tools/components/custom-tool-detail/custom-tool-detail.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ export function CustomToolDetail({
219219
...(tool && !readOnly
220220
? [
221221
{
222+
id: 'delete',
222223
text: deleteTool.isPending ? 'Deleting...' : 'Delete',
223224
variant: 'destructive' as const,
224225
onSelect: () => setShowDeleteConfirm(true),

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,15 +117,16 @@ function ServerListItem({
117117
const serverName = server.name || 'Unnamed server'
118118
// Transport rides on the description rather than beside the name — inside the
119119
// row's truncating title a long name would clip it away entirely.
120-
const statusLabel = isConnecting
120+
const statusText = isConnecting
121121
? 'Waiting for authorization...'
122122
: isRefreshing
123123
? 'Refreshing...'
124124
: isLoadingTools && tools.length === 0
125125
? 'Loading...'
126126
: showDiscoveryError
127127
? discoveryError
128-
: `${transportLabel} · ${toolsLabel}`
128+
: toolsLabel
129+
const statusLabel = `${transportLabel} · ${statusText}`
129130

130131
return (
131132
<SettingsResourceRow
@@ -444,6 +445,7 @@ export function MCP() {
444445
onSelect: () => setEditingServerId(server.id),
445446
},
446447
{
448+
id: 'delete',
447449
text: deletingServers.has(server.id) ? 'Deleting...' : 'Delete',
448450
variant: 'destructive' as const,
449451
onSelect: () => handleRemoveServer(server.id),

apps/sim/app/workspace/[workspaceId]/settings/components/member-list/member-list.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ interface MemberRowProps {
3030
export function MemberRow({ name, email, image, status, roleControl, menu }: MemberRowProps) {
3131
return (
3232
<div className={ROW_CLASSES}>
33-
<MemberAvatar name={name} image={image} />
33+
<MemberAvatar name={name} image={image} colorSeed={email || name} />
3434
<span className={ROW_EMAIL_CLASSES}>{email}</span>
3535
<span className={ROW_STATUS_CLASSES}>{status}</span>
3636
{roleControl}

apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ export function Sandboxes() {
8080
setDraft(null)
8181
setIssues([])
8282
}
83+
// The confirmation belongs to the sandbox that opened it. Browser Back unmounts
84+
// the modal without closing it, so leaving this set would re-open it against
85+
// whichever sandbox is selected next — and delete that one instead.
86+
setShowDeleteConfirm(false)
8387
// Creating and having one open are mutually exclusive, and history can land on
8488
// a sandbox while create mode is still set — Forward after starting a new one.
8589
// Leaving both on renders an empty "New sandbox" form whose Delete still points
@@ -211,6 +215,7 @@ export function Sandboxes() {
211215
...(selected && canAdmin
212216
? [
213217
{
218+
id: 'delete',
214219
text: deleteSandbox.isPending ? 'Deleting...' : 'Delete',
215220
variant: 'destructive' as const,
216221
onSelect: () => setShowDeleteConfirm(true),

apps/sim/app/workspace/[workspaceId]/settings/components/settings-resource-row/settings-resource-row.tsx

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import Link from 'next/link'
55
import {
66
RESOURCE_TILE_BASE,
77
RESOURCE_TILE_FILL,
8+
RESOURCE_TILE_PLAIN,
89
} from '@/app/workspace/[workspaceId]/components/resource-tile'
910

1011
/**
@@ -84,6 +85,12 @@ interface SettingsResourceRowProps {
8485
* `lucide-react` and `@sim/emcn/icons` ship visibly different ones.
8586
*/
8687
navigable?: boolean
88+
/**
89+
* Drops the row's `-mx-2` bleed and padding. For a row that is not in a list —
90+
* a detail heading — or one inside a fixed-height `overflow-y-auto` box, where
91+
* the bleed would force a horizontal scrollbar.
92+
*/
93+
flush?: boolean
8794
}
8895

8996
/** The one navigation chevron for every settings resource row. */
@@ -95,9 +102,14 @@ export const RESOURCE_ROW_ARROW_CLASSES = 'size-4 flex-shrink-0 text-[var(--text
95102
*/
96103
export const RESOURCE_LIST_STACK = 'flex flex-col gap-y-0.5'
97104

98-
/** Responsive two-up card grid worn by the skills, integrations, and passwords lists. */
105+
/**
106+
* Responsive two-up card grid worn by the skills, integrations, and passwords
107+
* lists. The column gap budgets for the rows' own `-mx-2`: 24px of track gap
108+
* minus 16px of combined bleed leaves the same 8px gutter a stack row gets.
109+
* Narrowing it makes neighbouring rows — and their hit areas — overlap.
110+
*/
99111
export const RESOURCE_LIST_GRID =
100-
'grid grid-cols-[repeat(auto-fit,minmax(280px,1fr))] gap-x-2 gap-y-0.5'
112+
'grid grid-cols-[repeat(auto-fit,minmax(280px,1fr))] gap-x-6 gap-y-0.5'
101113

102114
const PLAIN_BASE =
103115
'flex size-[14px] flex-shrink-0 items-center justify-center text-[var(--text-icon)] [&_svg]:size-[14px] [&_img]:size-[14px]'
@@ -115,6 +127,7 @@ export function SettingsResourceRow({
115127
href,
116128
clickLabel,
117129
navigable = false,
130+
flush = false,
118131
}: SettingsResourceRowProps) {
119132
const isTile = iconVariant === 'tile'
120133
const cluster = (
@@ -127,7 +140,7 @@ export function SettingsResourceRow({
127140
isTile
128141
? cn(
129142
RESOURCE_TILE_BASE,
130-
iconFilled ? RESOURCE_TILE_FILL : 'bg-[var(--bg)]',
143+
iconFilled ? RESOURCE_TILE_FILL : RESOURCE_TILE_PLAIN,
131144
iconFill ? '[&_img]:size-full' : '[&_img]:size-5'
132145
)
133146
: PLAIN_BASE
@@ -161,7 +174,7 @@ export function SettingsResourceRow({
161174

162175
// Row geometry is identical whether or not the row is activatable, so a list
163176
// mixing clickable and static rows keeps one height and one inset.
164-
const rowClass = '-mx-2 flex items-center justify-between gap-2.5 rounded-lg p-2'
177+
const rowClass = cn('flex items-center justify-between gap-2.5', !flush && '-mx-2 rounded-lg p-2')
165178

166179
if (!onClick && !href) {
167180
return (
@@ -182,7 +195,10 @@ export function SettingsResourceRow({
182195
// list) while `trailing` — which may hold its own buttons — stacks above it.
183196
return (
184197
<div
185-
className={cn(rowClass, 'relative transition-colors hover-hover:bg-[var(--surface-active)]')}
198+
className={cn(
199+
rowClass,
200+
'group relative transition-colors hover-hover:bg-[var(--surface-active)]'
201+
)}
186202
>
187203
{href ? (
188204
<Link href={href} aria-label={clickLabel} className={overlayClass} />

apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,7 @@ function ServerDetailView({
416416
...(onDelete
417417
? [
418418
{
419+
id: 'delete',
419420
text: isDeleting ? 'Deleting...' : 'Delete',
420421
variant: 'destructive' as const,
421422
onSelect: onDelete,

apps/sim/app/workspace/[workspaceId]/skills/skills.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { getErrorMessage } from '@sim/utils/errors'
66
import { Plus } from 'lucide-react'
77
import { useParams, useRouter } from 'next/navigation'
88
import { useQueryState } from 'nuqs'
9-
import { AgentSkillsIcon } from '@/components/icons'
9+
import { SkillTile } from '@/app/workspace/[workspaceId]/components'
1010
import { IntegrationTabsHeader } from '@/app/workspace/[workspaceId]/integrations/components/integration-tabs-header'
1111
import { ShowcaseWithExplore } from '@/app/workspace/[workspaceId]/integrations/components/showcase-with-explore'
1212
import { SettingsEmptyState } from '@/app/workspace/[workspaceId]/settings/components/settings-empty-state'
@@ -111,8 +111,8 @@ export function Skills() {
111111
{filteredSkills.map((s) => (
112112
<SettingsResourceRow
113113
key={s.id}
114-
icon={<AgentSkillsIcon className='text-[var(--text-icon)]' />}
115-
iconFilled
114+
iconVariant='custom'
115+
icon={<SkillTile />}
116116
title={s.name}
117117
description={s.description || undefined}
118118
onClick={() => router.push(`${skillsHref}/${s.id}`)}

0 commit comments

Comments
 (0)