manage/.pi-tmp/svc-hub-slice4-review.md

# Review — Slice 4: services-as-hub-ia (frontend shell)

**Scope:** unstaged frontend changes — top-nav generation, service-page tab skeleton + instance switcher, ServiceTypePage resolver, empty-state CTAs, dashboards API/hook, stubs.
**Base:** `main` (NOT `mobile-responsive-parity`); absence of SheetForm/useIsMobile is expected and not flagged.

## Verdict: fix-then-commit

One blocker (secret-editing behavior loss) must be fixed before commit. One confirmed issue (missing legacy-404 test promised by the slice) should be added. Everything else is sound.

---

## Verification results (commands run)

| Command | Result |
|---|---|
| `cd frontend && npm run lint` | PASS — 0 errors (2 pre-existing warnings in `UsersPage.impl.tsx`, deleted in slice 8) |
| `cd frontend && npm run build` | PASS — built in 1.19s (tsc + vite) |
| `cd frontend && npm run test` | PASS — 25 files / 83 tests |
| `git diff --cached --stat` | empty — no staged files |

---

## Blocker

### B1 — Secret editing is broken (behavior loss) — `frontend/src/pages/ServicePage.tsx`

The Config-body lift orphaned the secret-draft state. The old `ServiceConnectionCard` saved secrets by filtering its local `draftSecrets` to non-empty values and sending them on its own "Update connection" button. The new `ConfigBody` still owns `draftSecrets` (line ~`const [draftSecrets, setDraftSecrets] = useState<...>({})`), but the merged Save button calls the parent's `onSave` → `save()` → `buildInput()`, which hard-codes **`secrets: {}`**:

```ts
function buildInput(): ServiceInstanceInput {
  return {
    id: instance!.id,
    service_type: instance!.service_type,
    name,
    config: draftConfig,
    secrets: {},          // <-- typed secret values are never collected
    enabled,
  };
}
```

So typing a value into any secret field and clicking Save sends an empty secrets object — the secret is discarded. This violates **R2.3** ("Config tabs unchanged … secrets editors") and **R10.1** ("ServicePage config/secrets editing continue to work"), and directly contradicts review verification point #2 ("preserve … config/secrets editing verbatim, no behavior loss").

**Fix:** lift `draftSecrets` to the parent (alongside `name`/`enabled`/`draftConfig`), or have `ConfigBody` expose its draft secrets to the save path. Cleanest: move `draftSecrets` into `ServicePage` state and build secrets in `buildInput()`:

```ts
const onlyChanged = Object.fromEntries(
  Object.entries(draftSecrets).filter(([, v]) => v !== ""),
);
// ... secrets: onlyChanged ...
```

and reset `draftSecrets` after a successful save. Add a ServicePage test that types a secret and asserts the mutate payload includes it (the current test never exercises secret save).

---

## Confirmed issues (should-fix before commit)

### C1 — Missing legacy-route 404 test (slice deliverable gap)

Slice 4.5 / AC5 / R4.7 explicitly call for **"404-on-legacy-routes tests."** The *implementation* is correct — all legacy routes (`/media`, `/files`, `/actions`, `/users`, `/observability`, `/backups`, `/monitoring`, `/applications`) were removed and `<Route path="*" element={<NotFoundPage />} />` catches them (`App.tsx`). But there is **no test** asserting any of these resolve to the NotFound catch-all. No `App.test.tsx` exists; `grep` for `notfound/404/legacy` across `*.test.*` finds nothing relevant.

**Fix:** add a small `App`-level (or router-level) test rendering `<AppInner>` (or the route subtree) with `MemoryRouter initialEntries=["/media"]` etc. and asserting the "Not found" text renders for each legacy path. The behavior is right; only the test is missing.

---

## Suggestions (non-blocking)

### S1 — Instance switcher trigger counts all siblings, not enabled-only (R3.1)

`frontend/src/pages/ServicePage.tsx`:

```ts
const siblings = services.filter((s) => s.service_type === serviceType);
const showSwitcher = siblings.length > 1;
```

R3.1 specifies the switcher appears when "**more than one enabled instance**" exists. Today two instances where one is disabled still show the switcher, and the dropdown lists disabled instances too. Minor edge case (the common path — two enabled — works and is tested). Suggest `services.filter((s) => s.service_type === serviceType && s.enabled)` for the trigger condition. Whether to also navigate to disabled instances in the dropdown is a product call, but the *trigger* should key off enabled count per spec.

### S2 — No nav loading skeleton (design deviation, graceful but not as specified)

Design §"Top nav generation" / risk list: *"Show a skeleton nav until settled; do not block the route render."* `useNavItems` defaults both queries to `[]` while loading, so during load the nav renders only the core entries (Dashboard / Services / Settings) and conditional + dashboard entries pop in once data arrives. This is graceful (no crash, core always visible) but is not a skeleton and allows a nav "flash." Acceptable for the shell slice; consider an `isLoading`-gated skeleton later. `R1.4` is satisfied in spirit.

### S3 — `/d/:slug` route is absent (staging, not a defect)

`useNavItems` emits `/d/:slug` entries for named dashboards, but `App.tsx` has no `/d/:slug` route, so clicking one would currently hit the catch-all NotFound. This is fine for slice 4 because **no named dashboards exist yet** (Main Dashboard lives at `/`; named-dashboard CRUD/landing is slice 10), so the entries are empty in practice. Flagging only so the parent knows slice 10 must add the route — not a slice-4 blocker.

### S4 — Composed nav order is unit-tested only partially

`navEntries.test.ts` thoroughly covers `configuredNavEntries` (filtering, ssh_tasks double-entry, nextcloud-none, declaration order). The *composed* `useNavItems` order (Dashboard first, then dashboards, then service entries, then Services, then Settings) is not asserted by a test. Behavior is correct by inspection; a tiny composed-order assertion would lock AC1. Optional.

---

## Confirmed correct (with evidence)

- **Nav order (R1.1/AC1):** `useNavItems` (`App.tsx`) returns `[Dashboard, ...dashboardEntries, ...serviceEntries, Services, Settings]`. ✓
- **Conditional filtering (R1.2):** `configuredTypes` is built from `services.filter((s) => s.enabled)`; `configuredNavEntries` filters the static map. ssh_tasks correctly contributes Files+Actions (two entries); nextcloud has no entries in the static map (asserted by test). ✓
- **Tab skeleton (R2.1/R2.4):** `serviceContentTabs` (`service-tabs/index.ts`) switch returns exactly: jellyfin→Media+Requests, ssh_tasks→Files+Actions, backups→Jobs, authentik→Users+Messaging, alertmanager→Alerts, grafana→Links, prometheus→Metrics, default(nextcloud)→[]. ServicePage renders `[Overview, ...content, Widgets, Config]`. ✓
- **Stubs are stubs:** `service-tabs/stubs.tsx` — every tab is a "coming soon" `<Alert>`; no half-implemented content. ✓
- **Widgets tab preserved:** widget-list rendering lifted verbatim into `widgetsContent` (kind/name/description/badge + "add from dashboard edit dialog"). ✓
- **Instance switcher (R3):** renders a Radix `Select` only when `siblings.length > 1`; absent for single instance; selecting navigates to `/services/:type/:id`. Tested (show/hide). ✓ (modulo S1 enabled-count nuance)
- **Routing (R4):** legacy routes removed; `*` catch-all → `NotFoundPage`; `/services/:serviceType` → `ServiceTypePage` (resolves first-enabled → `<Navigate>` redirect, empty-state if none); `/services/:serviceType/:serviceId` → `ServicePage`; `/`, `/settings`, `/services` unchanged. Two route blocks (desktop + mobile drawer) kept in sync. ✓
- **Empty state (R9):** Dashboard renders "Welcome to Manage / Add a service" CTA when `services.length === 0` (`Dashboard.tsx`); `Dashboard.test.tsx` mocks the new `useServiceInstances`. ServicesPage strong empty state already pre-exists (`ServicesPage.tsx:298`). ✓
- **Rules of Hooks:** `useNavItems`, `ServicePage`, `ServiceTypePage` all call hooks unconditionally at top level — no conditional hooks. `useServiceInstances`/`useDashboards` accept optional/undefined args cleanly. ✓
- **Diff size ~530 lines:** structural, not scope creep. Bulk is `ServicePage.tsx` (260 changed — ConfigBody lift + tab skeleton + switcher) and the new `service-tabs/` + `navEntries` + `dashboards` API/hook, all in scope for slice 4. `useDashboards`/`api/dashboards.ts` belong here because the design wires `useDashboards()` into nav generation. No real content migrated. ✓
- **`./shared` import in `api/dashboards.ts`:** resolves to the existing `api/shared.ts` (get/post/put/del with auth headers). ✓
- **Test quality:** `navEntries.test.ts` asserts real filtering/order behavior; `ServicePage.test.tsx` asserts per-type tab presence (jellyfin vs ssh_tasks) and switcher conditional. Good — aside from the missing legacy-404 and secret-save cases above. ✓

---

## acceptance-report

```acceptance-report
{
  "criteriaSatisfied": [
    {
      "id": "criterion-1",
      "status": "partial",
      "evidence": "Scope is bounded to slice 4 (nav generation, service-page skeleton, stubs, resolver, empty states, dashboards API/hook). No content migration leaked. However one in-scope behavior (secret editing, R2.3/R10.1) regressed and must be fixed; one promised test (legacy-404) is missing."
    },
    {
      "id": "criterion-2",
      "status": "satisfied",
      "evidence": "Cited file:line evidence for each finding; ran lint/build/test; verified git staging state."
    }
  ],
  "changedFiles": [
    "frontend/src/App.tsx",
    "frontend/src/pages/Dashboard.tsx",
    "frontend/src/pages/ServicePage.tsx",
    "frontend/src/pages/__tests__/Dashboard.test.tsx",
    "frontend/src/integrations/navEntries.ts",
    "frontend/src/integrations/__tests__/navEntries.test.ts",
    "frontend/src/api/dashboards.ts",
    "frontend/src/hooks/useDashboards.ts",
    "frontend/src/pages/service-tabs/stubs.tsx",
    "frontend/src/pages/service-tabs/index.ts",
    "frontend/src/pages/ServiceTypePage.tsx",
    "frontend/src/pages/__tests__/ServicePage.test.tsx"
  ],
  "testsAddedOrUpdated": [
    "frontend/src/integrations/__tests__/navEntries.test.ts",
    "frontend/src/pages/__tests__/ServicePage.test.tsx",
    "frontend/src/pages/__tests__/Dashboard.test.tsx"
  ],
  "commandsRun": [
    {
      "command": "cd frontend && npm run lint",
      "result": "passed",
      "summary": "0 errors; 2 pre-existing warnings in UsersPage.impl.tsx (deleted in slice 8)"
    },
    {
      "command": "cd frontend && npm run build",
      "result": "passed",
      "summary": "tsc + vite build succeeded in 1.19s"
    },
    {
      "command": "cd frontend && npm run test",
      "result": "passed",
      "summary": "25 files / 83 tests passed"
    },
    {
      "command": "git diff --cached --stat",
      "result": "passed",
      "summary": "empty — no staged files"
    }
  ],
  "validationOutput": [
    "lint: 0 errors",
    "build: success",
    "test: 83/83 passed",
    "no staged files"
  ],
  "residualRisks": [
    "B1 (blocker): secret editing sends secrets:{} — fix before commit",
    "C1: no legacy-route 404 test though behavior is implemented",
    "S1: switcher trigger keys off total siblings not enabled-only (R3.1 nuance)",
    "S3: /d/:slug route absent — fine now (no named dashboards exist), must land in slice 10"
  ],
  "noStagedFiles": true,
  "diffSummary": "~530 lines: App.tsx data-driven nav (useNavItems from services+dashboards) + legacy-route removal + NotFound catch-all; ServicePage refactored to tab skeleton [Overview,...content,Widgets,Config] with instance switcher and ConfigBody lift; new navEntries map/filter, service-tabs stubs, ServiceTypePage resolver, Dashboard empty-state CTA, dashboards API+hook. Structural overrun, not scope creep.",
  "reviewFindings": [
    "blocker: frontend/src/pages/ServicePage.tsx buildInput() returns secrets:{} — typed secret drafts in ConfigBody are never sent; secret editing regressed (R2.3/R10.1). Fix by lifting draftSecrets and sending onlyChanged.",
    "confirmed-issue: no test asserts legacy routes (/media,/files,/actions,/users,/observability,/backups) hit the NotFound catch-all — slice 4.5/AC5 promised it; behavior implemented but untested.",
    "suggestion: ServicePage.tsx switcher trigger counts all siblings, not enabled-only (R3.1).",
    "suggestion: no nav loading skeleton (design called for one); partial-nav-during-load is graceful but flashes.",
    "suggestion: /d/:slug route absent; acceptable staging, lands in slice 10."
  ],
  "manualNotes": "Verdict: fix-then-commit. Fix B1 (secret save) and add C1 (legacy-404 test), then commit slice 4. S1–S4 are non-blocking follow-ups. Confirmed the base is main (no SheetForm/useIsMobile) per instructions; mobile reconciliation is deferred."
}
```