Skip to content

feat(settings): adopt set-grid dark/mono shell with all sections wired#38

Merged
hoootan merged 2 commits into
mainfrom
feat/settings-page-redesign
Apr 28, 2026
Merged

feat(settings): adopt set-grid dark/mono shell with all sections wired#38
hoootan merged 2 commits into
mainfrom
feat/settings-page-redesign

Conversation

@hoootan
Copy link
Copy Markdown
Owner

@hoootan hoootan commented Apr 28, 2026

Summary

Brings the Settings page in line with the rest of the redesigned dashboard
shipped in #37. That merge only updated Settings' theme hook because the
page wasn't part of the redesign commit — this completes it.

  • Restores the set-* CSS primitives (set-grid, set-nav, set-card,
    set-row, set-input, .toggle, .key-row).
  • Rewrites settings/page.tsx as the set-grid shell with left nav
    grouped under Workspace / Runtime / Integrations / Notifications. Only
    the active section renders.
  • Every section is wired to a real API:
    • ConcurrencyGET/PATCH /api/v1/tenant/concurrency (PATCH on
      blur for numbers, click for the idempotency toggle).
    • NotificationsGET/PATCH /api/v1/tenant/notifications + Test
      buttons hitting POST /tenant/notifications/test. All 7 fields
      surfaced (Slack URL/channel, PD enabled+key, two trigger toggles,
      email digest).
    • Danger zone — Pause all / Transfer ownership / Delete workspace,
      each behind a confirmation Dialog (typed-slug for delete).
    • Members / Billing / Audit / API keys / Model providers / Secrets
      host the existing sub-components inside set-section blocks (no
      outer set-card to avoid card-in-card).
  • General workspace identity stays localStorage-only with a help-line
    noting the backend PATCH /tenant endpoint hasn't shipped yet.
  • Drops concurrency-tab.tsx, notifications-tab.tsx, danger-zone.tsx
    — content is inlined in the page now.

Test plan

  • pnpm build clean (only the pre-existing auth-store SSR
    rehydration warning, not introduced here)
  • /settings renders with the dark/mono shell, left nav highlights
    the active section, only one section visible at a time
  • Concurrency: change a number → PATCH fires on blur, returns and
    reflects in the field
  • Notifications: paste a hooks.slack.com URL → "Test" → toast
    shows "Sent." (or the server's rejection message)
  • Danger zone: pause-all toast count is correct; transfer dialog
    lists workspace users; delete dialog rejects mismatched slug

Brings the Settings page in line with the rest of the redesigned
dashboard. The earlier merge (#37) only updated Settings' theme hook
because the page wasn't part of the redesign commit; this completes the
job.

CSS:
- Restore the set-* primitives in globals.css: set-grid (sticky left
  nav + content), set-nav (grouped headings + is-on highlight), set-card
  (with .danger-card variant), set-row (200/1fr dashed-divider rows),
  set-input, set-help, .toggle, .key-row.

Settings page (rewritten):
- Replace shadcn Tabs/Card with the set-grid shell. Left nav grouped
  under Workspace / Runtime / Integrations / Notifications. Only the
  active section renders.
- Sections wired to real APIs:
  * Concurrency — GET/PATCH /api/v1/tenant/concurrency (PATCH on blur
    for numbers, click for the idempotency toggle).
  * Notifications — GET/PATCH /api/v1/tenant/notifications + per-channel
    Test buttons hitting POST /tenant/notifications/test. All 7 fields
    surfaced (Slack URL/channel, PD enabled+key, two trigger toggles,
    email digest).
  * Danger zone — Pause all (POST /tenant/pause-all), Transfer ownership
    (POST /tenant/transfer-ownership with user-picker), Delete workspace
    (DELETE /tenant with typed-slug confirmation). Each behind a
    shadcn Dialog.
  * Members & access, Billing, Audit, API keys, Model providers, Secrets
    sections host the existing sub-components inside set-section blocks
    without an outer set-card wrapper (the sub-components carry their
    own Card chrome — avoids the card-in-card we hit earlier).
- General workspace identity stays localStorage-only with a help-line
  noting the backend PATCH /tenant endpoint hasn't shipped yet.

Cleanup:
- Drop concurrency-tab.tsx, notifications-tab.tsx, danger-zone.tsx —
  their content is inlined in the page now and no other consumers exist.
Copilot AI review requested due to automatic review settings April 28, 2026 22:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Completes the Settings page redesign to match the new dark/mono dashboard shell by replacing the old tabbed layout with a set-grid left-nav + single-active-section layout and inlining previously separate sections.

Changes:

  • Rewrites settings/page.tsx into a set-grid shell with grouped left navigation and section-by-section rendering.
  • Inlines Concurrency / Notifications / Danger Zone logic into the page and removes the old tab components.
  • Restores the set-* CSS primitives (set-grid, set-nav, set-card, set-row, set-input, .toggle, .key-row) in globals.css.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
dashboard/src/app/(dashboard)/settings/page.tsx Replaces Tabs-based settings UI with set-grid shell, inlines Concurrency/Notifications/Danger Zone, and hosts existing sub-components in set-section blocks.
dashboard/src/app/globals.css Adds set-* settings primitives and .toggle/.key-row styles to support the redesigned settings layout.
dashboard/src/components/settings/concurrency-tab.tsx Deleted (logic moved into settings/page.tsx).
dashboard/src/components/settings/notifications-tab.tsx Deleted (logic moved into settings/page.tsx).
dashboard/src/components/settings/danger-zone.tsx Deleted (logic moved into settings/page.tsx).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +308 to +317
<a
key={it.id}
className={active === it.id ? "is-on" : ""}
onClick={(e) => {
e.preventDefault();
setActive(it.id);
}}
>
{it.label}
</a>
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The sidebar nav items are rendered as elements without an href. Anchors without href are not keyboard-focusable and won’t be announced as interactive controls by screen readers. Use (or add a real href + proper routing) and set aria-current for the active item.

Suggested change
<a
key={it.id}
className={active === it.id ? "is-on" : ""}
onClick={(e) => {
e.preventDefault();
setActive(it.id);
}}
>
{it.label}
</a>
<button
key={it.id}
type="button"
className={active === it.id ? "is-on" : ""}
aria-current={active === it.id ? "page" : undefined}
onClick={() => {
setActive(it.id);
}}
>
{it.label}
</button>

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +93
{ id: "notifications", label: "Alerts" },
{ id: "danger", label: "Danger zone" },
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

Notifications/Danger zone are described as “admin-only” in the UI copy, but the nav does not mark them as adminOnly and the sections render for non-admin users. This exposes controls that will likely fail server-side and is risky for destructive actions; gate these items/sections behind isAdmin (and consider gating the initial API fetches too).

Suggested change
{ id: "notifications", label: "Alerts" },
{ id: "danger", label: "Danger zone" },
{ id: "notifications", label: "Alerts", adminOnly: true },
{ id: "danger", label: "Danger zone", adminOnly: true },

Copilot uses AI. Check for mistakes.
Comment on lines +165 to +169
api.getUsers().then((res) => {
if (res?.users) {
setTransferUsers(res.users.map((u) => ({ id: u.id, email: u.email })));
}
});
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The transfer-ownership dropdown is populated with all users, including the current user. Transferring ownership to yourself is at best a no-op and at worst an API error. Filter out the current user (e.g., via useAuthStore().user.id or api.getCurrentUser()) before setting transferUsers.

Suggested change
api.getUsers().then((res) => {
if (res?.users) {
setTransferUsers(res.users.map((u) => ({ id: u.id, email: u.email })));
}
});
Promise.all([api.getUsers(), api.getCurrentUser()]).then(
([res, currentUser]) => {
if (res?.users) {
const currentUserId = currentUser?.id;
setTransferUsers(
res.users
.filter((u) => u.id !== currentUserId)
.map((u) => ({ id: u.id, email: u.email })),
);
}
},
);

Copilot uses AI. Check for mistakes.
Comment on lines +879 to +887
<button
className="btn"
style={{
color: "var(--danger)",
borderColor: "color-mix(in oklab, var(--danger) 40%, var(--line))",
}}
onClick={handleDeleteWorkspace}
disabled={deleting || confirmSlug.trim() !== tenantSlug}
>
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The delete-workspace confirm button can become enabled when tenantSlug is still empty (initial state), because confirmSlug.trim() === tenantSlug (both ""). That allows calling deleteWorkspace("") without a loaded slug. Disable the destructive action until tenantSlug is non-empty and add an explicit guard in handleDeleteWorkspace for !tenantSlug.

Copilot uses AI. Check for mistakes.
Comment on lines +489 to 498
<button
type="button"
aria-label="Toggle idempotency keys"
className={`toggle ${conc.use_event_id_idempotency ? "on" : ""}`}
onClick={() =>
persistConcurrency({
use_event_id_idempotency: !conc.use_event_id_idempotency,
})
}
/>
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The custom toggle control lacks ARIA state. Add role="switch" with aria-checked (or aria-pressed) bound to conc.use_event_id_idempotency so assistive tech can understand the on/off state; apply the same pattern to the other .toggle buttons in this page.

Copilot uses AI. Check for mistakes.
<h1>
Settings <em>· workspace.</em>
</h1>
<p>flowforge · workspace settings</p>
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

The page header text is hard-coded to "flowforge · workspace settings", so it won’t reflect the current workspace name/slug (or even the locally edited General values). Consider deriving this from tenant info or the General state so the header stays consistent with the rest of the page.

Suggested change
<p>flowforge · workspace settings</p>
<p>{`${tenantSlug || "workspace"} · workspace settings`}</p>

Copilot uses AI. Check for mistakes.
- (#1 a11y) Sidebar nav items rendered as <a> without href — convert to
  <button type="button"> with aria-current="page" on the active item.
  globals.css updated so .set-nav { a, button } selectors share styles.
- (#2 access) Notifications and Danger zone are admin-only on the server
  (GET /tenant/notifications returns secrets, danger actions are
  destructive). Mark both NAV items adminOnly so members no longer see
  them, and skip the notifications GET fetch entirely for non-admins to
  avoid a noisy 403 on mount.
- (#3 transfer) Filter the current user out of the transfer-ownership
  dropdown — the server explicitly rejects self-transfer.
- (#4 destructive) Delete-workspace button could enable when tenantSlug
  was still empty (initial state, both strings ""). Disable the
  destructive action and add an explicit guard in handleDeleteWorkspace.
- (#5 a11y) Add role="switch" + aria-checked to all six .toggle buttons
  so assistive tech can read the on/off state.
- (#6 copy) Page subtitle was hard-coded to "flowforge · workspace
  settings". Derive from tenantSlug (with general.workspaceSlug
  fallback) so the header tracks the actual workspace.
@hoootan hoootan merged commit 207d449 into main Apr 28, 2026
4 checks passed
@hoootan hoootan deleted the feat/settings-page-redesign branch April 28, 2026 22:47
hoootan added a commit that referenced this pull request Apr 28, 2026
* fix(dashboard): cap getRuns page_size at 100 to match server limit

After the dashboard redesign (#37/#38), three pages requested
`getRuns({ page_size: 200 })`. The server caps `page_size` at 100
(`server/src/flowforge_server/api/routes/runs.py:76` —
`Query(50, ge=1, le=100)`), so every one of those calls returns:

  422 Unprocessable Entity
  {"validation_errors":[{"field":"query.page_size",
   "message":"Input should be less than or equal to 100"}]}

The runs list, dashboard home, and functions page all render empty
because the primary fetch errors. Reproduce by visiting any of the
three pages with at least one run in the system and watching the
network tab — `GET /api/v1/runs?page_size=200` 422s.

Fix: drop the three callsites to 100 (the server max). A future
follow-up should paginate properly when there are >100 runs, but
this restores visibility immediately.

Files changed:
  - dashboard/src/app/(dashboard)/page.tsx
  - dashboard/src/app/(dashboard)/functions/page.tsx
  - dashboard/src/app/(dashboard)/runs/page.tsx

* fix(dashboard): rename remaining inline-style var(--accent) → var(--brand)

Copilot caught 4 stray var(--accent) / var(--accent-ink) references
in TSX inline styles that the globals.css rename missed:

- (auth)/login/page.tsx — logo background + 2FA Shield icon
- (dashboard)/runs/page.tsx — bar-chart OK fraction
- (dashboard)/runs/[id]/page.tsx — Output section kicker

All four were brand-intent (the redesign was using --accent to mean
brand). They would have rendered as the new neutral surface (var(--bg-3))
after this PR's earlier --accent → --brand rename, losing the green.
Switching them to --brand restores the intent.
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.

2 participants