manage/.pi-tmp/slice6-review.md

Slice 6 Review — ServicePage mobile form (mobile-responsive-parity)

Reviewer: fresh adversarial review Scope: unstaged frontend/src/pages/ServicePage.tsx + new frontend/src/pages/__tests__/ServicePage.test.tsx Commands run: npm run lint (0 errors, 2 pre-existing warnings in UsersPage.impl.tsx), npm run build (green), npm run test (110 passed; ServicePage suite 5/5).

Correct (verified with evidence)

• Desktop non-regression — token-identical. Compared git show HEAD:ServicePage.tsx against the new desktop branch. The heading (<h2> + binding.description + Badge), the SectionCard title="General" (Name + Enabled + Save/Delete), configFields (=<ServiceConnectionFields isMobile={false}> → renders <SectionCard title="Connection" description="…">{fields}</SectionCard> with byte-identical field JSX), widgetsCard (identical conditional SectionCard), and confirmDelete (identical ConfirmDialog) all render the same tree. The refactor only extracted inline JSX into configFields/widgetsCard/confirmDelete consts and renamed ServiceConnectionCard→ServiceConnectionFields; desktop output is unchanged. ✓
• Mobile SheetForm wiring. sheetOpen init true (open-on-mount, ServicePage.tsx:79); title = name || instance.name (draft-aware, :166); onSave={save} (:167); onCancel={() => setSheetOpen(false)} (:168); isPending={saveService.isPending} disables Save in SheetForm footer. ✓
• Connection fields render without SectionCard on mobile. ServiceConnectionFields isMobile branch returns <div className="flex flex-col gap-3">{fields}</div> (no card) — the SheetForm is the container. Desktop branch still wraps in SectionCard title="Connection". ✓
• Save semantics preserved. buildInput() (:111-121) returns { id, service_type, name, config: draftConfig, secrets: {}, enabled }; save() calls saveService.mutateAsync(buildInput()). ✓
• Secrets "leave blank to keep" preserved. handleUpdateConnection() filters draftSecrets to non-blank only (filter(([,v]) => v !== "")); General Save still sends secrets: {}. Same dual-save model as desktop. ✓
• Delete flow on both branches. Mobile branch renders {confirmDelete} as a sibling of <SheetForm> (ServicePage.tsx:188), so the ConfirmDialog overlays correctly outside the sheet. Desktop unchanged. ✓
• Rules of Hooks — clean. In ServicePage: useParams, useServiceInstances, useServiceTypes, useSaveServiceInstance, useDeleteServiceInstance, both useMemo, all five useState, useIsMobile, and useState(sheetOpen) are all called unconditionally before the !binding/!instance early returns. In ServiceConnectionFields: useSaveServiceInstance() + useState(draftSecrets) at top, unconditionally. No conditional hooks. The earlier "useIsMobile inside a conditional" risk was correctly avoided. ✓
• Test quality — solid. Desktop test #2 asserts queryByRole("dialog") is null (no SheetForm at ≥768px). Mobile test #2 edits the name, clicks Save, and asserts mutateAsync called once with input.name === "Renamed Grafana" and input.id === "svc-1". Mobile test #3 asserts the base_url config field is editable. All 5 pass. ✓

Confirmed issues (must-fix before commit)

Blocker-1 — R4.5 violation: Sheet does not close on successful save

Location: frontend/src/pages/ServicePage.tsx:117-119 (save()) and :165-170 (SheetForm onSave wiring).

save() is:

async function save() {
    await saveService.mutateAsync(buildInput());
}

It never calls setSheetOpen(false). Spec R4.5 explicitly requires: "The Sheet closes on successful save and on explicit cancel." Cancel closes (onCancel → setSheetOpen(false)), but after a successful Save on mobile the sheet stays open. useSaveServiceInstance only invalidates queries; it does not close the sheet. This is a direct, testable deviation from the requirement that AC8/verify will flag.

Fix: close the sheet on successful resolve, e.g.

async function save() {
    await saveService.mutateAsync(buildInput());
    setSheetOpen(false);
}

(Then also address Blocker-2, since closing the sheet surfaces the empty-page problem.)

Notes / risks (non-blocking but important)

Risk-1 — "Cancel leaves empty page" is a REAL UX bug (not acceptable as-is)

The mobile branch (ServicePage.tsx:161-191) renders only <SheetForm> + {confirmDelete}. There is no list, no back button, no useNavigate. When the sheet closes — via Cancel today, or via Save once Blocker-1 is fixed — the user is stranded on a blank <div className="flex flex-col gap-4"> with no way back except browser history. This is a genuine UX defect, not an acceptable artifact of the sheet pattern: this page is reached via /services/:serviceType/:serviceId (deep link / row tap from ServicesPage), so closing the editor must return the user somewhere.

Recommendation: on sheet close (both save-success and cancel), navigate back to the services list — e.g. add const navigate = useNavigate(); and onOpenChange={(o) => { setSheetOpen(o); if (!o) navigate("/services"); }}, or render a fallback "Back to services" affordance when !sheetOpen. This should be resolved in this slice, not deferred, because Blocker-1's fix makes it user-visible.

Risk-2 — R4.5 dirty-state outside-click confirm not implemented

R4.5 also says the sheet "does not close on outside-click while the form is dirty (confirm prompt)." SheetForm passes onOpenChange straight through to Radix Sheet with no dirty guard, and ServicePage wires onOpenChange={setSheetOpen} directly. This is likely a cross-slice concern owned by the Slice-1 SheetForm deliverable, but it is currently unmet for this form. Flag for the verify pass / Slice 1 retro.

Suggestion-1 — Strengthen the mobile Save payload assertion

Mobile test #2 (ServicePage.test.tsx) only asserts input.name and input.id. To lock the save semantics claimed by the slice, also assert input.config (equals draftConfig), input.enabled, and input.secrets === {}. Cheap and prevents regressions.

Suggestion-2 — save() async-onClick typing

SheetForm.onSave is typed () => void but receives an async function; the promise is fire-and-forget. isPending correctly gates the button so this is functionally fine, but worth a comment or a .catch if error toast UX is added later.

Verdict

fix-then-commit.

The desktop non-regression, Rules-of-Hooks, secrets/delete semantics, and test scaffolding are all correct and verified. However, Blocker-1 (sheet does not close on save) is a clear, spec-cited (R4.5) deviation, and Risk-1 (empty page after close) is a real UX bug that becomes user-visible the moment Blocker-1 is fixed. Both should be addressed in this slice before commit. Risk-2 and the two suggestions are non-blocking follow-ups.

Acceptance

{
  "criteriaSatisfied": [
    {
      "id": "criterion-1",
      "status": "partially-satisfied",
      "evidence": "Slice 6 implements ServicePage mobile SheetForm without widening scope (only ServicePage.tsx + new test). Desktop output verified token-identical to HEAD; Rules-of-Hooks clean; secrets/delete semantics preserved; lint/build/test green. BUT R4.5 'sheet closes on successful save' is not implemented (save() never calls setSheetOpen(false)) and closing the sheet strands the user on an empty page — must-fix before commit."
    }
  ],
  "changedFiles": [
    "frontend/src/pages/ServicePage.tsx"
  ],
  "testsAddedOrUpdated": [
    "frontend/src/pages/__tests__/ServicePage.test.tsx"
  ],
  "commandsRun": [
    { "command": "cd frontend && npm run lint", "result": "passed", "summary": "0 errors; 2 pre-existing warnings in UsersPage.impl.tsx (unrelated)" },
    { "command": "cd frontend && npm run build", "result": "passed", "summary": "vite build green (chunk-size advisory only)" },
    { "command": "cd frontend && npm run test", "result": "passed", "summary": "110/110 tests pass; ServicePage suite 5/5" },
    { "command": "git show HEAD:frontend/src/pages/ServicePage.tsx", "result": "passed", "summary": "Used to verify desktop branch token-identical to pre-change page" }
  ],
  "validationOutput": [
    "Desktop non-regression: CONFIRMED token-identical (heading, General, Connection, Widgets, ConfirmDialog).",
    "Mobile SheetForm wiring (open-on-mount, title=draft name, onSave=save, onCancel closes, isPending disables Save): CONFIRMED.",
    "Connection fields render without SectionCard inside sheet on mobile: CONFIRMED.",
    "buildInput() + save()→mutateAsync: CONFIRMED.",
    "Secrets leave-blank-to-keep (onlyChanged filter; General secrets:{}): CONFIRMED.",
    "ConfirmDialog rendered OUTSIDE SheetForm on mobile (sibling): CONFIRMED.",
    "Rules of Hooks (all hooks unconditional, before early returns): CONFIRMED clean.",
    "R4.5 'closes on successful save': NOT MET — save() does not call setSheetOpen(false).",
    "Empty page after sheet close (cancel/save): real UX bug, no back navigation."
  ],
  "residualRisks": [
    "Blocker-1: Sheet does not close on successful save (R4.5 violation) — ServicePage.tsx:117-119.",
    "Risk-1: Closing the sheet (cancel, or save once fixed) leaves an empty page with no path back to /services — ServicePage.tsx mobile branch.",
    "Risk-2: R4.5 dirty-state outside-click confirm not implemented at ServicePage/SheetForm level (likely Slice-1 cross-cutting concern)."
  ],
  "noStagedFiles": true,
  "diffSummary": "Adds a mobile (isMobile) branch to ServicePage that renders the edit form inside a SheetForm (open-on-mount, draft-name title, onSave=save, onCancel=close) with Connection fields unwrapped and ConfirmDialog as a sibling; extracts desktop JSX into configFields/widgetsCard/confirmDelete consts and renames ServiceConnectionCard→ServiceConnectionFields (isMobile prop) so the desktop output stays token-identical. Adds 5 Vitest cases (2 desktop, 3 mobile).",
  "reviewFindings": [
    "blocker: ServicePage.tsx:117-119 — save() does not close the sheet on success; violates R4.5.",
    "blocker: ServicePage.tsx:161-191 — mobile branch has no back navigation; closing the sheet strands the user on an empty page (becomes visible once blocker-1 is fixed).",
    "note: R4.5 dirty-state outside-click confirm not implemented (SheetForm passes onOpenChange through).",
    "suggestion: ServicePage.test.tsx mobile Save test should also assert config/enabled/secrets payload, not just name+id."
  ],
  "manualNotes": "Verdict: fix-then-commit. Desktop non-regression, hooks, and core save/delete/secrets semantics are correct and verified. The two blockers are tightly coupled (fixing save-close surfaces the empty-page gap) and should be resolved together in this slice: close sheet on save AND navigate back to /services (or render a fallback) on close."
}