refactor(settings): fold verified domains into SSO, move group-detail state to nuqs, design-system cleanup#5950
Conversation
…mitives Verified domains only gates SSO, so managing it on a separate page meant discovering the requirement after filling out the whole IdP form and then navigating away mid-setup. Move it into the SSO page as a section above the provider config and drop the standalone page, its nav entry, and both route branches. Align the surfaces with the shared settings primitives rather than bespoke chrome, matching whitelabeling/custom-blocks/access-control: - SSO's local FormField (muted labels) is replaced by the shared SettingRow, so its fields read like every other settings page. SettingRow gains optional `optional` and `error` props to absorb what FormField did — additive, so existing consumers are untouched. - The domains section is built from SettingsSection, SettingRow, SettingsResourceRow, and SettingsEmptyState instead of hand-rolled cards. Also drop the redundant Upload/Change buttons in whitelabeling: the logo and wordmark thumbnails were already clickable, so the button was a second control for the same action. Remove still appears once an image is set.
…gn-system drift Access control's group detail kept its tab, three search boxes, and three status filters in useState, so a `?group-id=` link always landed on General and a filter was lost on reload — the parent already puts the group id in the URL. The three tabs never render together, so search and status share one param each rather than carrying three mutually-exclusive keys, and switching tabs resets both. Closing the detail clears all three alongside group-id in one batched write, so nothing lingers on the list URL. Design-system fixes from a cleanup pass over the surfaces this branch touched: - Restore accessible names lost when the whitelabeling Upload buttons were removed. The thumbnail is now the only click target, and it contained just an icon, so it announced as an unlabeled button; the icon-only Remove had the same problem. Both now carry aria-labels reflecting their state. - Use Chip, not the legacy Button, for the domain actions — Button is ~26px against the 30px ChipInput beside it, so "Add domain" sat visibly short. - SettingRow now uses the emcn Info component; a bare svg as Tooltip.Trigger was neither focusable nor nameable. It also stops re-specifying Label's own default styling. - Use the new SettingRow error prop for the group name instead of a hand-rolled error paragraph, which is what the prop was added for. - Hoist the block-category lookup out of a sort comparator, size-* over h/w, name the staleTime constants the rules require, and import RowActionsMenu from its barrel.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Access control group detail moves tab, search, and enabled/disabled filters from local state into URL params ( Design-system / UX polish on touched surfaces: SSO drops its local Reviewed by Cursor Bugbot for commit 4a38eee. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis follow-up completes the legacy verified-domain navigation fix while consolidating related settings UI and URL state.
Confidence Score: 5/5The PR appears safe to merge, with no blocking failure remaining in the legacy verified-domain link fix. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/settings/[section]/page.tsx | Adds the workspace-route alias needed to redirect legacy verified-domain links to SSO. |
| apps/sim/components/settings/navigation.ts | Removes the standalone verified-domains navigation entry and aliases legacy organization links to SSO. |
| apps/sim/ee/sso/components/sso-settings.tsx | Integrates verified-domain management into SSO and adopts shared settings components. |
| apps/sim/ee/access-control/components/group-detail.tsx | Moves permission-group detail view state into constrained URL parameters. |
| apps/sim/ee/components/setting-row.tsx | Extends the shared settings row with optional and validation-error presentation. |
Reviews (3): Last reviewed commit: "fix(settings): reset group detail params..." | Re-trigger Greptile
Folding verified domains into the SSO page dropped /settings/domains, so bookmarks and shared links 404'd instead of landing where domains now live. Both alias maps already exist for exactly this (organization/'members', subscription/'billing'); add domains -> sso to each.
|
@cursor 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 4a38eee. Configure here.
1 participant
Summary
useState, so a?group-id=link always landed on General and filters were lost on reload — even though the parent already puts the group id in the URL. Because the tabs never render together, search and status share one param each (group-search,group-status) rather than three mutually-exclusive keys; switching tabs resets both, and closing the detail clears all three alongsidegroup-idin one batched write.Design-system cleanup
A pass over the touched surfaces (url-state, React hygiene, emcn/settings conventions) surfaced these:
aria-labels.Chipover legacyButtonfor the domain actions:Buttonis ~26px against the 30pxChipInputbeside it, so "Add domain" sat visibly short in the same row.SettingRowuses the emcnInfocomponent — a bare<svg>asTooltip.Triggerwas neither focusable nor nameable — and stops re-specifyingLabel's own default styling.FormFieldreplaced by the sharedSettingRow(19 call sites), which is why its labels now match whitelabeling/custom-blocks/access-control.SettingRowgained optionaloptional/errorprops to absorb whatFormFielddid — additive, existing consumers untouched — and group-detail now useserrorinstead of a hand-rolled error paragraph.size-*overh-N w-N, namedstaleTimeconstants per the rules, barrel import forRowActionsMenu.Deliberately not included
useState+ syncing-effect refactor. It's a genuine finding (including a ref read during render), but it's pre-existing, changes render timing, and the audit itself says to verify it against the running UI — which doesn't belong in a PR about something else. Worth its own change.errors-as-derived-state and the Title Case → sentence case label sweep: same reasoning, tracked separately.Type of Change
Testing
162 tests across SSO / whitelabeling / access-control / settings.
tsc0, full-repolint:checkclean,check:api-validationandcheck:utilspass. Not visually verified in a running app.Checklist