Skip to content

improvement(settings): persistent layout + locked-down header API#5278

Merged
waleedlatif1 merged 6 commits into
stagingfrom
settings-page-layout
Jun 30, 2026
Merged

improvement(settings): persistent layout + locked-down header API#5278
waleedlatif1 merged 6 commits into
stagingfrom
settings-page-layout

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

What

Mirror the Resource compound component's locked invariant for settings, per Emir's review ("ideally a Next.js layout … nobody can vibe-code a padding change").

  • Persistent chrome via a Next.js layout. All page chrome — header bar, title/description, scroll region, centered max-w-[48rem] column, spacing — moves into settings/[section]/layout.tsx (SettingsHeaderShell). It stays mounted across section navigation, so the header/description/spacing never re-render or re-lay-out. SettingsPanel becomes 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).
  • Locked-down 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. Adds a left back slot for detail sub-views and an aside escape hatch for the rare non-chip widget.
  • SaveDiscardActions component → saveDiscardActions() helper returning the dirty-gated SettingsAction[].
  • Migrated 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 — none hand-roll their own shell anymore.
  • Emir's chip rules: sentence-case labels (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 own CredentialDetailLayout (it lives outside [section]).

Verification

tsc clean, biome clean, check:api-validation passed. 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-page skill).

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.
@vercel

vercel Bot commented Jun 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 30, 2026 4:05am

Request Review

@cursor

cursor Bot commented Jun 30, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Large UI refactor across many settings surfaces; behavior should be equivalent but header registration and navigation transitions are easy to get subtly wrong.

Overview
Moves settings page chrome into settings/[section]/layout.tsx via SettingsHeaderShell and SettingsHeaderProvider, so the header bar, title/description, scroll region, and centered column stay mounted when switching sections. SettingsPanel no longer renders that shell—it registers header data through context (useSettingsHeader) and only renders section body.

actions changes from arbitrary JSX to SettingsAction[] (data-only chips with optional tooltip / onPrefetch). Detail views use back instead of hand-rolled shells. SaveDiscardActions becomes saveDiscardActions(), returning actions to spread into actions.

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. settings.tsx drops the extra wrapper div around section switches. Docs/rules updated; secrets/[credentialId] still uses its own layout outside [section].

Reviewed by Cursor Bugbot for commit f174569. Configure here.

Comment thread apps/sim/ee/access-control/components/group-detail.tsx Outdated
@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves settings pages onto persistent shared chrome. The main changes are:

  • Adds a section-level settings layout with a shared header shell.
  • Converts SettingsPanel into a header registrar.
  • Replaces JSX header actions with data-driven settings actions.
  • Migrates settings list and detail views onto the shared panel contract.
  • Updates save/discard actions to return reusable action data.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/settings/[section]/layout.tsx Adds the section-level provider and shell wrapper for persistent settings chrome.
apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx Adds the shared header shell, context registration, and data-driven action rendering.
apps/sim/app/workspace/[workspaceId]/settings/components/settings-panel/settings-panel.tsx Changes the panel into a registrar that feeds header data to the shared shell.
apps/sim/ee/access-control/components/group-detail.tsx Moves the group detail view into the shared settings panel and keeps its tabs pinned in the scroll region.
apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx Migrates workflow MCP server detail chrome and actions to the shared settings panel contract.

Reviews (4): Last reviewed commit: "refactor(settings): final audit pass — p..." | Re-trigger Greptile

Comment thread apps/sim/app/workspace/[workspaceId]/settings/[section]/layout.tsx
Comment thread apps/sim/ee/access-control/components/group-detail.tsx Outdated
…, 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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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).
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/workspace/[workspaceId]/settings/[section]/layout.tsx
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@waleedlatif1 waleedlatif1 merged commit 948a5cb into staging Jun 30, 2026
16 checks passed
@waleedlatif1 waleedlatif1 deleted the settings-page-layout branch June 30, 2026 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant