improvement(settings): persistent layout + locked-down header API#5278
Conversation
Mirror the Resource compound component's locked invariant for settings. - Move ALL page chrome (header bar, title/description, scroll region, centered max-w-[48rem] column, spacing) into the Next.js settings/[section]/layout.tsx via SettingsHeaderShell, so it persists across section navigation and never re-renders / re-lays-out. SettingsPanel becomes a thin registrar that feeds the shell its header data via a React context slot. - Lock the header API: actions is now SettingsAction[] (data only: text/icon/variant/active/onSelect/disabled, rendered as Chips) — no JSX, no <div>, no className, so a padding change is structurally impossible. Add a left 'back' slot for detail sub-views + an 'aside' escape hatch for the rare non-chip widget. - SaveDiscardActions component -> saveDiscardActions() helper returning the dirty-gated SettingsAction[]. - Migrate every panel page + the 5 in-section detail views (access-control group-detail, data-retention PolicyDetail, mcp, credential-sets, workflow-mcp) onto the layout/back-slot; they no longer hand-roll their own shells. - Chip labels are sentence case (Add override, Create server, ...); the action gap is owned once by the shell; no action <div>s. Documents the new contract in the settings rule + add-settings-page skill.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Migrated list pages and in-section detail views (MCP, credential sets, workflow MCP servers, access-control group detail, data-retention policy editor, secrets manager save flow, etc.) to the new API, with sentence-case chip labels. Reviewed by Cursor Bugbot for commit f174569. Configure here. |
Greptile SummaryThis PR moves settings pages onto persistent shared chrome. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "refactor(settings): final audit pass — p..." | Re-trigger Greptile |
…, parity fixes
Address /cleanup + /simplify + per-page parity audit findings:
- Foundation: shell dereferences action/back/search handlers from the config ref
at call time, so a section re-render that changes a closure (e.g. a dirty-form
onSave) can never leave the shell holding a stale handler.
- Add SettingsAction.tooltip (shell renders the hover tooltip, incl. for disabled
actions). Migrate the secrets-manager + workflow-mcp 'aside' tooltips to data
actions — eliminates the per-page <div>-wrapped tooltip chips (Emir's no-div).
- Parity: detail-view description no longer falls back to the section meta blurb
(empty-description groups render blank again); secrets Save keeps no variant.
- Sentence-case: group-detail back 'Access control'; empty-state prose
'Create API key' / 'Add tool' / 'Add server'.
- Drop redundant array-level 'satisfies SettingsAction[]'; make SettingsPanel
children optional (loading detail states drop the {null}).
…store tooltips - Remove the SettingsHeaderConfig 'aside' escape hatch (the last stale-prone, div-admitting path). Add SettingsAction.onPrefetch so the teammates Invite chip (hover-prefetch) is pure data — nothing renders header JSX now, so the signature-vs-ref staleness cursor flagged is gone. - useIsomorphicLayoutEffect for header registration: section switches flush the new header before paint (no stale/blank header frame). - group-detail: pin the config tabs (sticky top-0) so they stay visible while the detail body scrolls. - Restore the team-management Invite disabled-reason tooltip via the new SettingsAction.tooltip field.
|
@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 d56c0a2. Configure here.
…consistency
From a line-by-line audit (+ React-docs validation of the click-time ref deref and
isomorphic layout-effect patterns):
- workflow-mcp detail: restore origin action order (Edit server, then the primary
Add workflows) so the primary CTA stays rightmost.
- Sentence-case the detail back-chip labels to match nav ('MCP tools', 'MCP
servers'); fix the workflow-mcp list empty-state copy ('Add server').
- data-retention: import ArrowLeft from '@sim/emcn/icons' (canonical back-chip icon
source, matching the other detail views); group-detail: import SettingsPanel from
the package index, not the deep file path.
- Document that scrollContainerRef is intentionally excluded from the header
signature (refs are identity-stable).
|
@greptile review |
|
@cursor review |
|
@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 f174569. Configure here.
What
Mirror the
Resourcecompound component's locked invariant for settings, per Emir's review ("ideally a Next.js layout … nobody can vibe-code a padding change").max-w-[48rem]column, spacing — moves intosettings/[section]/layout.tsx(SettingsHeaderShell). It stays mounted across section navigation, so the header/description/spacing never re-render or re-lay-out.SettingsPanelbecomes a thin registrar that feeds the shell its header data through a React context slot (stable setter + signature ref → no render loop, no flash, no stale handlers).actionsis nowSettingsAction[](data only:text / icon / variant / active / onSelect / disabled, rendered asChips) — no JSX, no<div>, noclassName, so a padding change is structurally impossible. Adds a leftbackslot for detail sub-views and anasideescape hatch for the rare non-chip widget.SaveDiscardActionscomponent →saveDiscardActions()helper returning the dirty-gatedSettingsAction[].PolicyDetail, mcp, credential-sets, workflow-mcp) onto the layout/back-slot — none hand-roll their own shell anymore.Add override,Create server, …); the action gap is owned once by the shell; zero action<div>s.The route-based credential detail (
settings/secrets/[credentialId]) keeps its ownCredentialDetailLayout(it lives outside[section]).Verification
tscclean,biomeclean,check:api-validationpassed. Foundation built by hand; the ~15 section migrations were fanned out across parallel subagents and verified together. Docs updated (sim-settings-pages.md+add-settings-pageskill).