improvement(settings): consolidate resource UI onto shared primitives - #6202
Conversation
Sandboxes, MCP tools, and MCP servers each modeled their list rows and detail headers differently. Collapse them — and the surfaces they drifted from — onto one set of primitives. SettingsResourceRow now owns the row entirely: tile, title/subtitle tokens, padding and bleed, hover band, hit area, focus ring, and the one navigation chevron. Adds onClick/href (a stretched overlay, so interactive trailing controls keep their clicks), navigable, iconVariant='custom', and a badge slot for decoration that must not swallow row clicks. Rows that open a detail page get a chevron and a whole-row click; flat records keep the "..." menu. Delete moves to the detail header as a destructive chip behind a confirm modal — sandboxes previously deleted with no confirmation at all. Also folds in SettingsField (new), SettingsEmptyState tone='error', RESOURCE_LIST_STACK/GRID, RESOURCE_TILE_PLAIN, and a shared MemberAvatar; deletes DetailIconTile (byte-identical to ResourceTile); and standardizes on the emcn ArrowRight, which is a visibly different glyph from lucide's.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Navigation pattern shifts for several resources: detail pages use whole-row click + chevron instead of a Docs/skills ( Reviewed by Cursor Bugbot for commit df4263b. Configure here. |
Greptile SummaryThis PR consolidates resource lists and detail surfaces onto shared settings primitives while standardizing navigation, deletion confirmation, spacing, typography, and empty states.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/settings/components/settings-resource-row/settings-resource-row.tsx | Expands the shared resource-row primitive to provide consistent layout, navigation overlays, accessibility, trailing controls, and visual treatment. |
| apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx | Migrates sandbox rows and detail deletion to the shared interaction model; the identity guard correctly clears confirmation state when selection changes. |
| apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/components/sandbox-editor.tsx | Adds confirmed deletion to the sandbox detail header and displays the selected sandbox in confirmation copy. |
| apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx | Converts MCP resources from menu-only detail access to whole-row navigation using the shared primitive. |
| apps/sim/app/workspace/[workspaceId]/components/credential-detail/components/credential-detail-heading.tsx | Reuses the static shared resource row for consistent credential-detail heading layout and typography. |
| .claude/rules/sim-settings-pages.md | Documents the consolidated resource-row, deletion, field, section, tile, and list-container contracts. |
Reviews (3): Last reviewed commit: "fix(settings): wire flush, drop the lone..." | Re-trigger Greptile
…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.
Consolidating the Teammates/Organization avatar onto the credential member row's was a redesign, not a deduplication. The two encode different things: the roster is a dense list keyed on email where the avatar is a 14px neutral marker, while a member management row carries a name, an email, and a role control and earns a 36px hashed avatar. Merging them made those rows ~70% taller, gave every workspace in a permission group a colour-hashed "avatar" seeded on its name, and cut the Add Members picker from ~7 visible rows to ~5 inside its fixed-height box. Restores both avatars and the containers that owned their bleed, and records in the rule why they stay separate. Keeps only the unrelated fix in that area: the picker row was the one settings row rounded at `sm`.
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.
|
All three review findings are fixed and the threads are resolved:
Three commits landed since the last review, including a revert of the Teammates/Organization avatar change (that was a redesign, not a deduplication) and a round of fixes from a further audit: a dead CI is green on |
|
Greptile has re-reviewed bugbot run |
|
@greptile review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit df4263b. Configure here.
Summary
SettingsResourceRownow owns the whole row — tile, title/subtitle tokens, padding + bleed, hover band, hit area, focus ring, and the one navigation chevron. NewonClick/href(stretched overlay, so interactive trailing controls keep their clicks),navigable,iconVariant='custom', and abadgeslot for decoration that must not swallow row clicks....menu. MCP tools/servers previously had...→ "Details" as the only way into their detail page.destructivechip behind aChipConfirmModal. Sandboxes previously deleted on a single click with no confirmation.SettingsField;SettingsEmptyStategainedtone='error'; addedRESOURCE_LIST_STACK/RESOURCE_LIST_GRID,RESOURCE_TILE_PLAIN, and a sharedMemberAvatar. DeletedDetailIconTile(byte-identical toResourceTile).CredentialDetailHeading,DetailSection, andIntegrationSectionare now thin aliases over the shared primitives.ArrowRight— it is a visibly different glyph from lucide's (different viewBox and stroke width), and both were in use on adjacent settings lists.text-xsis 11px in this repo (not 12px),text-[14px]≠text-sm(different line-height), the BYOK skeleton didn't match the row it stands in for, BYOK's list ran at 4× the standard row gap, inbox rows were indented 8px against their own search field, and custom-tool detail printed the tool name twice..claude/rules/sim-settings-pages.mdwith the resource-row contract, the chevron-vs-...rule, the delete rule, and six new checklist items; removed the section that taught hand-rolling row title/subtitle spans.Type of Change
Testing
tscandbiomeclean. 630 tests across 52 files pass in every touched area, including two test files updated for the new row DOM (verified they still fail when row content breaks). Not yet exercised in a browser.Checklist