improvement(settings): consolidate resource UI onto shared primitives - #6202
improvement(settings): consolidate resource UI onto shared primitives#6202waleedlatif1 wants to merge 4 commits into
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 and delete behavior shifts for several resources: detail pages open via whole-row click + chevron instead of a Integrations/skills/passwords lists, credential headings, and related tests are updated to match the new row DOM and Reviewed by Cursor Bugbot for commit 80a01f0. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 80a01f0. Configure here.
Greptile SummaryThe PR consolidates settings resource lists and detail surfaces onto shared row, tile, section, field, empty-state, and navigation primitives.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the sandbox confirmation is reset synchronously when its selected resource changes, and the current modal confirmation remains bound to that selected resource.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/sandboxes.tsx | The selection-identity guard now disarms the delete confirmation before a newly selected sandbox can become its target, resolving the previously reported wrong-sandbox deletion path. |
| apps/sim/app/workspace/[workspaceId]/settings/components/settings-resource-row/settings-resource-row.tsx | Introduces the shared resource-row interaction and presentation primitive used throughout the migrated settings lists. |
| apps/sim/app/workspace/[workspaceId]/settings/components/sandboxes/utils.ts | Adds confirmation copy that names the selected sandbox and explains the permanent deletion consequence. |
Reviews (2): Last reviewed commit: "fix(settings): wire flush, drop the lone..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…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 |
1 participant
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