refactor(settings/service): split into URL-routed sub-routes with side-nav layout #829

Merged

charles merged 2 commits from refactor/settings-service-subroutes into main 2026-05-04 19:00:38 +00:00

Conversation 1 Commits 2 Files changed 13 +1205 -964

claude-desktop commented 2026-05-04 18:11:21 +00:00

Collaborator

Copy link

Summary

Picks up the "rethink relation between settings page and its subpages" feedback from PR #827. Implements specs/settings-service-subroutes.md end-to-end.

Operator complaints addressed:

• Inputs sprawled across the full viewport — page now caps at max-w-220 (matches sister settings pages: labels / repos / secrets).
• Service-config page felt orphaned — /settings/ hub already had a card; subtitle updated to drop the dead "auth, node flows" copy left over from the pre-PR-A tab list.
• In-page tab bar broke deep-links + browser back — each section now lives at its own URL (/settings/service/{container,watchdogs,design}) routed by TanStack Router file-based routing. Bookmarkable, browser back navigates between sections.

New layout

/settings/service/* — AppShell
  ↳ header (← Settings · title · subtitle)
  ↳ scope chips (Builtin / Global) + "Reset all to builtin"
  ↳ side-nav (rail on ≥sm, scroll-snap top tabs on <sm)
  ↳ <Outlet> — section-specific form

The layout owns the shared service-config-settings query, the save / reset / delete-all mutations, the destructive-fields confirm dialog (today: only container_image_default), and the scope chips. Section forms read from useServiceConfig() so they don't need to thread props through.

File map

File	Role
apps/web/src/lib/service-config-context.tsx	New — ServiceConfigProvider + useServiceConfig() hook + ScopeId type
apps/web/src/components/settings-side-nav.tsx	New generic side-nav primitive (route-aware via TanStack <Link> activeProps)
apps/web/src/features/service-config/primitives.tsx	New — INPUT_CLS, <Field>, <Toolbar> extracted
apps/web/src/features/service-config/{container,watchdogs,design}-section.tsx	New — one section per file
apps/web/src/routes/settings.service.tsx	Layout: header + scope chips + side-nav + <Outlet> + ServiceConfigProvider
apps/web/src/routes/settings.service.index.tsx	New beforeLoad redirect → /settings/service/container
apps/web/src/routes/settings.service.{container,watchdogs,design}.tsx	New thin route shells
apps/web/src/routes/settings.index.tsx	Service-config card subtitle refresh
apps/web/src/routeTree.gen.ts	Auto-regen by TanStackRouterVite

Test plan

• just qa — 3135/3135 server tests + web typecheck green
• just restart — service running, SPA bundle rebuilt
• Operator smoke — sign in, hit /app/settings/service → redirects to /container. Click each side-nav item → URL changes + content swaps. Browser back navigates between sections.
• Deep-link smoke — load /app/settings/service/design directly → renders Design section without going through the redirect.
• Mobile smoke — narrow viewport → side-nav collapses to horizontal scroll-snap tabs.

Follow-up

PR D (specs/service-config-url-consolidation.md) — surface per-forge OAuth client/secret editors. The Forge sub-route comes back as settings.service.forge.tsx, drops into the side-nav, hosts three forge cards (Forgejo / GitLab / GitHub) with URL + client_id + client_secret editor each.

🤖 Generated with Claude Code

## Summary Picks up the "rethink relation between settings page and its subpages" feedback from PR #827. Implements `specs/settings-service-subroutes.md` end-to-end. Operator complaints addressed: - **Inputs sprawled across the full viewport** — page now caps at `max-w-220` (matches sister settings pages: labels / repos / secrets). - **Service-config page felt orphaned** — `/settings/` hub already had a card; subtitle updated to drop the dead "auth, node flows" copy left over from the pre-PR-A tab list. - **In-page tab bar broke deep-links + browser back** — each section now lives at its own URL (`/settings/service/{container,watchdogs,design}`) routed by TanStack Router file-based routing. Bookmarkable, browser back navigates between sections. ## New layout ``` /settings/service/* — AppShell ↳ header (← Settings · title · subtitle) ↳ scope chips (Builtin / Global) + "Reset all to builtin" ↳ side-nav (rail on ≥sm, scroll-snap top tabs on <sm) ↳ <Outlet> — section-specific form ``` The layout owns the shared `service-config-settings` query, the save / reset / delete-all mutations, the destructive-fields confirm dialog (today: only `container_image_default`), and the scope chips. Section forms read from `useServiceConfig()` so they don't need to thread props through. ## File map | File | Role | |---|---| | `apps/web/src/lib/service-config-context.tsx` | **New** — `ServiceConfigProvider` + `useServiceConfig()` hook + `ScopeId` type | | `apps/web/src/components/settings-side-nav.tsx` | **New** generic side-nav primitive (route-aware via TanStack `<Link>` `activeProps`) | | `apps/web/src/features/service-config/primitives.tsx` | **New** — `INPUT_CLS`, `<Field>`, `<Toolbar>` extracted | | `apps/web/src/features/service-config/{container,watchdogs,design}-section.tsx` | **New** — one section per file | | `apps/web/src/routes/settings.service.tsx` | Layout: header + scope chips + side-nav + `<Outlet>` + ServiceConfigProvider | | `apps/web/src/routes/settings.service.index.tsx` | **New** `beforeLoad` redirect → `/settings/service/container` | | `apps/web/src/routes/settings.service.{container,watchdogs,design}.tsx` | **New** thin route shells | | `apps/web/src/routes/settings.index.tsx` | Service-config card subtitle refresh | | `apps/web/src/routeTree.gen.ts` | Auto-regen by TanStackRouterVite | ## Test plan - [x] `just qa` — 3135/3135 server tests + web typecheck green - [x] `just restart` — service running, SPA bundle rebuilt - [ ] Operator smoke — sign in, hit `/app/settings/service` → redirects to `/container`. Click each side-nav item → URL changes + content swaps. Browser back navigates between sections. - [ ] Deep-link smoke — load `/app/settings/service/design` directly → renders Design section without going through the redirect. - [ ] Mobile smoke — narrow viewport → side-nav collapses to horizontal scroll-snap tabs. ## Follow-up PR D (`specs/service-config-url-consolidation.md`) — surface per-forge OAuth client/secret editors. The Forge sub-route comes back as `settings.service.forge.tsx`, drops into the side-nav, hosts three forge cards (Forgejo / GitLab / GitHub) with URL + client_id + client_secret editor each. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-desktop added 1 commit 2026-05-04 18:11:21 +00:00

refactor(settings/service): split into URL-routed sub-routes with side-nav layout ... e1fa5b1829

Picks up the "rethink relation between settings page and its subpages"
feedback from PR #827. Implements `specs/settings-service-subroutes.md`
end-to-end.

Operator complaints addressed:
- Inputs sprawled across the full viewport — page now caps at
  `max-w-220` (matches sister settings pages: labels / repos / secrets).
- Service-config page felt orphaned — `/settings/` hub already had a
  card; subtitle updated to drop the dead "auth, node flows" copy.
- In-page tab bar broke deep-links + browser back. Each section now
  lives at its own URL (`/settings/service/{container,watchdogs,design}`)
  routed by TanStack Router file-based routing. Bookmarkable, browser
  back navigates between sections.

New layout:

    /settings/service/* — AppShell
      ↳ header (← Settings · title · subtitle)
      ↳ scope chips (Builtin / Global) + "Reset all to builtin"
      ↳ side-nav (rail on ≥sm, scroll-snap top tabs on <sm)
      ↳ <Outlet> — section-specific form

The layout owns the shared `service-config-settings` query, the save /
reset / delete-all mutations, the destructive-fields confirm dialog
(today: only `container_image_default`), and the scope chips. Section
forms read from `useServiceConfig()` so they don't need to thread props
through.

Files:

- `apps/web/src/lib/service-config-context.tsx` — `ServiceConfigProvider`
  + `useServiceConfig()` hook + `ScopeId` type. Children consume via
  context instead of `SectionProps` brick.
- `apps/web/src/components/settings-side-nav.tsx` — generic side-nav
  primitive. Vertical rail at `≥sm`, horizontal scroll-snap tabs below.
  TanStack `<Link>` `activeProps` drives the highlighted state from the
  URL. Re-usable for other deep settings pages later.
- `apps/web/src/features/service-config/primitives.tsx` — `INPUT_CLS`,
  `<Field>`, `<Toolbar>` extracted so each sub-route imports them.
- `apps/web/src/features/service-config/{container,watchdogs,design}-section.tsx`
  — one section per file. Move includes the `WatchdogsShape` defaults
  table and the Penpot `TeamByRepoEditor` / `PenpotTokenEditor`
  helpers (kept colocated with their consumer).
- `apps/web/src/routes/settings.service.tsx` — flips role from
  "page with tabs" to "layout with header + side-nav + Outlet". Owns
  the query + mutations + confirm dialog + ServiceConfigProvider.
- `apps/web/src/routes/settings.service.index.tsx` — `beforeLoad` →
  `redirect({ to: "/settings/service/container" })`. Old bookmarks at
  `/settings/service` land somewhere reasonable.
- `apps/web/src/routes/settings.service.{container,watchdogs,design}.tsx`
  — thin route shells; just register the URL and render the matching
  section component.
- `apps/web/src/routes/settings.index.tsx` — refresh the Service-config
  card subtitle (drop dead "auth, node flows" copy left over from the
  pre-PR-A tab list).
- `apps/web/src/routeTree.gen.ts` — auto-regenerated by TanStackRouterVite.

`just qa` green (3135/3135 server tests + web typecheck). Service
restarted; SPA bundle rebuilt.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

charles requested review from reviewer 2026-05-04 18:19:51 +00:00

reviewer requested changes 2026-05-04 18:22:13 +00:00

reviewer left a comment

Copy link

• behavior (Field label association): htmlFor={id} in apps/web/src/features/service-config/primitives.tsx points at the wrapper <div id={id}>, not the <input> inside it. Clicking the label does not focus the input; screen readers announce the wrong target. Fix: pass id as a prop from Field down to the child input (or expose it via a render-prop / cloneElement), or drop the generated id and require callers to wire their own id+htmlFor. The <input> elements already have aria-label so a11y is not broken, but the broken htmlFor is still misleading and non-compliant.
• perf (ServiceConfigProvider memo): useMemo in apps/web/src/lib/service-config-context.tsx depends on [value] but the call site in settings.service.tsx passes an inline object literal, so value is always a new reference every render and the memo never short-circuits. Not behavior-breaking, but the abstraction is broken. Fix: destructure the individual fields in the dependency array, or stabilise the object with useMemo at the call site.

- behavior (`Field` label association): `htmlFor={id}` in `apps/web/src/features/service-config/primitives.tsx` points at the wrapper `<div id={id}>`, not the `<input>` inside it. Clicking the label does not focus the input; screen readers announce the wrong target. Fix: pass `id` as a prop from `Field` down to the child input (or expose it via a render-prop / `cloneElement`), or drop the generated id and require callers to wire their own `id`+`htmlFor`. The `<input>` elements already have `aria-label` so a11y is not broken, but the broken `htmlFor` is still misleading and non-compliant. - perf (`ServiceConfigProvider` memo): `useMemo` in `apps/web/src/lib/service-config-context.tsx` depends on `[value]` but the call site in `settings.service.tsx` passes an inline object literal, so `value` is always a new reference every render and the memo never short-circuits. Not behavior-breaking, but the abstraction is broken. Fix: destructure the individual fields in the dependency array, or stabilise the object with `useMemo` at the call site.

claude-desktop referenced this pull request 2026-05-04 18:35:48 +00:00

feat(service-config): per-forge OAuth credentials editor (PR D of A/B/C/D) #830

charles referenced this pull request from a commit 2026-05-04 18:43:52 +00:00

fix(service-config): address #829 review — Field htmlFor + ServiceConfigProvider memo

charles added 1 commit 2026-05-04 18:43:52 +00:00

fix(service-config): address #829 review — Field htmlFor + ServiceConfigProvider memo ... 04ea54bbd9

Reviewer flagged two issues:

1. **`Field` `htmlFor={id}` pointed at the wrapper `<div>`, not the
   input.** Click-on-label didn't focus the control; screen readers
   announced the wrong target. Switched `Field` to a render-prop:
   `children` is now `(id: string) => ReactNode` and every caller
   applies the id to the actual `<input>`. The wrapper `<div id={id}>`
   is gone, so the `<label htmlFor>` now targets the real control.
   Side-effect: dropped the now-redundant `aria-label` on each input
   (the `<label>` does the labelling).

2. **`ServiceConfigProvider` `useMemo([value])` busted every render.**
   The deps array referenced an inline-object literal, so the memo
   never short-circuited. Refactored:
   - Provider now takes individual props (spread at the call site)
     instead of a single `value` object.
   - `useMemo` deps list the underlying primitives so the memo only
     fires when one of them actually changes.
   - `requestSave` / `requestReset` wrapped in `useCallback` in the
     layout so their identities are stable across renders (mutation
     objects are stable, so empty-dep callbacks are safe).

Affected files:
- `apps/web/src/features/service-config/primitives.tsx` — `Field`
  becomes render-prop.
- `apps/web/src/features/service-config/{container,watchdogs,design}-section.tsx`
  — every `<Field>{<input>}</Field>` rewritten as
  `<Field>{(id) => <input id={id} />}</Field>`.
- `apps/web/src/lib/service-config-context.tsx` — provider props +
  proper memo deps.
- `apps/web/src/routes/settings.service.tsx` — request callbacks via
  `useCallback`; provider invocation switches from `value={…}` to
  spread.

`just qa` green (3135/3135 server tests + web typecheck).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

charles merged commit eff41c620d into main 2026-05-04 19:00:38 +00:00

charles referenced this pull request from a commit 2026-05-04 19:00:39 +00:00

refactor(settings/service): split into URL-routed sub-routes with side-nav layout (#829)

charles deleted branch refactor/settings-service-subroutes 2026-05-04 19:00:39 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!829

No description provided.