docs(specs): object form robustness — data router, dirty guard, partial-failure, validation (#46)
This commit is contained in:
@@ -0,0 +1,164 @@
|
||||
# Object Form Robustness — Design
|
||||
|
||||
**Date:** 2026-06-07
|
||||
**Status:** Approved (brainstorming) — ready for implementation planning.
|
||||
**Issue:** #46.
|
||||
|
||||
## Context
|
||||
|
||||
The object create/edit form has data-integrity gaps that compound in long daily cataloguing sessions
|
||||
(High severity):
|
||||
- **Submit never disabled during save.** `object-form.tsx` submit button has no `disabled`/pending.
|
||||
Both create (`object-new-page.tsx`) and edit (`object-edit-form.tsx`) do **two sequential awaited
|
||||
mutations** (create/update, then `setFields`) with no feedback → a second click can duplicate-create
|
||||
or race the two-step write. `publish-control.tsx` is the model (disables on `isPending`).
|
||||
- **No unsaved-changes guard.** No `isDirty`/`useBlocker`/`beforeunload`; Cancel/nav loses work
|
||||
silently.
|
||||
- **Two-phase write recovers inconsistently.** Create teleports to the edit page on `setFields`
|
||||
failure; edit stays put with a banner. The partial write (core saved, field rejected) reads as
|
||||
nothing happened.
|
||||
- **Thin validation.** Core errors always show `form.required`; the server `code`
|
||||
(`type_mismatch`/`unresolved`/`unknown`) on `FieldRejection` is discarded; no `number_of_objects >= 1`.
|
||||
- No "Save & create another" / keyboard submit for batch entry.
|
||||
|
||||
**Facts established:** the form uses **react-hook-form** (so `formState.isDirty` + `isSubmitting` are
|
||||
available; `isSubmitting` stays true across both awaited mutations — the unified pending signal).
|
||||
`ObjectForm` props: `mode`, `defaults`, `onSubmit`, `onCancel`, `formError`, `fieldErrorKey`; the
|
||||
422→highlight path uses `form.setError`. `FieldRejection` (`queries.ts`) carries `field` + `code`.
|
||||
The router is `<BrowserRouter>` + `<Routes>` (component router) in `app.tsx`; **`useBlocker` requires
|
||||
a data router** (`createBrowserRouter` + `RouterProvider`). `renderApp` wraps `ui` in `<MemoryRouter>`
|
||||
with no `<Routes>` (tests supply their own when they need params).
|
||||
|
||||
### Decisions (from brainstorming)
|
||||
1. **Migrate to a data router** (the long-term-correct foundation; unblocks `useBlocker` and React
|
||||
Router v7 data APIs). Done via `createRoutesFromElements` to keep the exact route tree (minimal
|
||||
diff), isolated as the first task, with the test harness moved to `createMemoryRouter`.
|
||||
2. **Unify create partial-failure** to the edit route with a distinct "Object created, but a field was
|
||||
rejected" banner (single recovery surface).
|
||||
3. **Include batch entry** ("Save & create another" + Cmd/Ctrl+Enter).
|
||||
|
||||
## Components
|
||||
|
||||
### 0. Data-router migration (foundation — isolated)
|
||||
- **`app.tsx`:** replace `<BrowserRouter><Routes>…</Routes></BrowserRouter>` with a module-level
|
||||
`const router = createBrowserRouter(createRoutesFromElements(<>…the existing <Route> tree…</>))`
|
||||
and `export function App() { return <RouterProvider router={router} />; }`. The route JSX (login,
|
||||
`RequireAuth`→`AppShell` nest with all nested routes, lazy `Suspense` wrappers, splat) moves
|
||||
**verbatim** into `createRoutesFromElements`. No behavior/path change.
|
||||
- **`main.tsx`:** unchanged provider stack (QueryClient → Config → Toast) now wraps `<App/>` which is
|
||||
`<RouterProvider/>`.
|
||||
- **`test/render.tsx`:** `renderApp(ui, { route })` switches to
|
||||
`createMemoryRouter([{ path: "*", element: ui }], { initialEntries: [route] })` rendered via
|
||||
`<RouterProvider/>` (inside `QueryClientProvider`). Behavior-preserving: `ui` renders at `route`,
|
||||
and tests that supply their own `<Routes>` still nest correctly; now a data-router context exists so
|
||||
`useBlocker` works. **Acceptance: the entire existing suite stays green** before any feature is built
|
||||
on top.
|
||||
|
||||
### 1. Submit pending + keyboard + batch (`object-form.tsx`)
|
||||
- Submit `<Button type="submit" disabled={form.formState.isSubmitting}>` showing
|
||||
`isSubmitting ? t("form.saving") : (mode === "create" ? t("form.create") : t("form.save"))`. Disable
|
||||
Cancel while submitting. (`isSubmitting` is true across both awaited mutations because RHF awaits the
|
||||
async `onSubmit`.)
|
||||
- **Cmd/Ctrl+Enter** submits: a keydown handler on the form (`(e.metaKey||e.ctrlKey) && e.key==="Enter"`
|
||||
→ `form.handleSubmit(...)`), or rely on the native submit + a small handler. Implementer picks the
|
||||
clean form; must not double-submit.
|
||||
- **"Save & create another"** (create mode only): a secondary submit button. `ObjectForm` gains an
|
||||
optional `onSubmitAndNew?: (values) => Promise<void>` (or a `submitAction` flag passed to `onSubmit`)
|
||||
so the create page knows which button fired. On success it `form.reset(blankDefaults)` and focuses the
|
||||
first field instead of navigating. Edit mode does not show this button.
|
||||
|
||||
### 2. Validation messages
|
||||
- **Server `code` echo:** when a `FieldRejection` is shown, pick the message by `code` —
|
||||
`form.fieldError.type_mismatch` / `form.fieldError.unresolved` / `form.fieldError.unknown` (fallback
|
||||
to the existing `form.fieldRejected`). The create/edit pages already extract `e.field`; also pass
|
||||
`e.code` through to `ObjectForm` (extend `fieldErrorKey` handling to carry a code, e.g. a
|
||||
`fieldError?: { key: string; code?: string }` prop, or a second `fieldErrorCode?` prop). The
|
||||
highlight `form.setError` message uses the code-specific string.
|
||||
- **Core-field messages:** render the RHF error by `type` — `errors.core[key]?.type === "min"` →
|
||||
`form.min` (with the limit), else `form.required`. (Today it's always `form.required`.)
|
||||
- **`number_of_objects >= 1`:** register with `min: { value: 1, message: t("form.minCount") }` (RHF,
|
||||
client-side, before the round-trip). Keep `required`.
|
||||
|
||||
### 3. Unsaved-changes guard (`lib/use-unsaved-changes.tsx`)
|
||||
- `useUnsavedChanges(isDirty: boolean)`:
|
||||
- adds a `beforeunload` listener while `isDirty` (covers reload / tab-close / browser back),
|
||||
- calls `useBlocker(({currentLocation,nextLocation}) => isDirty && currentLocation.pathname !== nextLocation.pathname)` to block in-app navigation while dirty,
|
||||
- returns the `blocker` so the caller renders an `UnsavedChangesDialog` when `blocker.state === "blocked"` (Stay → `blocker.reset()`, Leave → `blocker.proceed()`).
|
||||
- **`UnsavedChangesDialog`** built on `ui/alert-dialog` (title/body/Stay/Leave). New i18n
|
||||
`form.unsaved.{title,body,stay,leave}`.
|
||||
- **Wire into `ObjectForm`:** `const isDirty = form.formState.isDirty;` → `useUnsavedChanges(isDirty)`;
|
||||
render the dialog. The **Cancel** button: if dirty, open the same confirm dialog (Leave → `onCancel`);
|
||||
if clean, `onCancel` directly.
|
||||
- **Suppress on successful save:** after a successful submit the form navigates; `isDirty` must be false
|
||||
by then (RHF `reset` on success, or the blocker condition excludes the programmatic post-save nav).
|
||||
The create→edit partial-failure nav is programmatic and must not be blocked — reset dirty (or set a
|
||||
"saving" ref that bypasses the blocker) around the save so the post-save/teleport navigation isn't
|
||||
caught. Implementer ensures save-driven navigation never triggers the guard.
|
||||
|
||||
### 4. Partial-failure unification
|
||||
- **Create** (`object-new-page.tsx`): on `setFields` failure after the core create succeeds, navigate
|
||||
to `/objects/${id}/edit` with `state: { created: true, fieldErrorKey: e.field, fieldErrorCode: e.code }`
|
||||
(today it passes `fieldsError`/`fieldErrorKey`; add `created` + `code`). The core-create already
|
||||
succeeded, so this is correct (the object exists; editing it is the right URL).
|
||||
- **Edit** (`object-edit-form.tsx`): read `location.state`. If `created`, show the banner
|
||||
`form.createdButFieldRejected` ("Object created, but a field was rejected — fix it below"); otherwise
|
||||
the existing rejection banner. Both highlight the field (via `fieldErrorKey` + code). Edit's own
|
||||
in-place partial-failure (update ok, setFields fails) keeps staying-put with its banner.
|
||||
- Result: a single recovery surface (the edit form) for both flows, with create messaged as "created."
|
||||
|
||||
## Data flow
|
||||
RHF manages form state → `isSubmitting` disables submit across both mutations → `isDirty` drives the
|
||||
guard (beforeunload + useBlocker + dialog) → on save success the form is clean and navigates → on
|
||||
create partial-failure it navigates to the edit route with `created` state → the edit form shows the
|
||||
"created" banner + field highlight (code-specific message).
|
||||
|
||||
## Error handling / edges
|
||||
- `isSubmitting` covers the whole two-phase write (RHF awaits `onSubmit`); a second click is a no-op
|
||||
(button disabled).
|
||||
- The guard must not fire on save-driven navigation (post-success or create→edit teleport) — reset
|
||||
dirty / bypass ref around saves.
|
||||
- `beforeunload` only prompts while dirty (don't attach unconditionally).
|
||||
- `useBlocker` now works (data router). Tests render under `createMemoryRouter` (harness change) so the
|
||||
blocker is exercisable.
|
||||
- A `FieldRejection` with an unknown `code` falls back to the generic `form.fieldRejected`.
|
||||
- `number_of_objects` min is client-side UX; the server remains source of truth.
|
||||
|
||||
## Testing
|
||||
- **Migration:** the full existing suite passes under the new `createMemoryRouter` harness (no
|
||||
behavior change) — this is the gate for Task 1.
|
||||
- **Submit disable:** during an in-flight save the submit button is `disabled` and reads "Saving…";
|
||||
a double-click fires the create mutation once (assert call count).
|
||||
- **Keyboard:** Cmd/Ctrl+Enter submits.
|
||||
- **Save & create another:** create mode → success resets the form (fields cleared, stays on /new),
|
||||
does not navigate to detail.
|
||||
- **Validation:** a `FieldRejection` with `code: "type_mismatch"` shows the code-specific message;
|
||||
`number_of_objects = 0` shows `form.minCount` without hitting the server.
|
||||
- **Dirty guard:** with a dirty form, attempting in-app nav shows the dialog (Stay keeps you, Leave
|
||||
proceeds); Cancel-when-dirty confirms; a clean form navigates freely; `beforeunload` listener added
|
||||
only when dirty (assert via the registered handler / a spy).
|
||||
- **Partial-failure:** create with a `setFields` 422 → lands on `/objects/:id/edit` with the "Object
|
||||
created, but a field was rejected" banner + field highlight; edit's own setFields 422 → stays with
|
||||
its banner. (Extend `object-new-page.test.tsx` / `object-edit-form.test.tsx`.)
|
||||
- Gate: `typecheck`/`lint`/`test`/`build`/`check:size`/`check:colors`; en/sv parity for all new keys;
|
||||
no codename.
|
||||
|
||||
## Acceptance criteria
|
||||
1. The app uses a data router (`createBrowserRouter` + `RouterProvider`) with the route tree unchanged;
|
||||
the test harness uses `createMemoryRouter`; the full suite is green.
|
||||
2. The submit button is disabled and shows "Saving…" while either mutation is pending; no double-submit
|
||||
/ duplicate-create is possible from the UI.
|
||||
3. A dirty form guards navigation: `useBlocker` dialog on in-app nav, `beforeunload` on reload/close,
|
||||
and Cancel confirms when dirty; saving navigates without a false prompt.
|
||||
4. Create and edit share one partial-failure recovery surface (the edit form); the create case is
|
||||
messaged "Object created, but a field was rejected" and highlights the field.
|
||||
5. Field-rejection messages reflect the server `code`; core errors show type-specific messages;
|
||||
`number_of_objects >= 1` is validated client-side.
|
||||
6. Create mode offers "Save & create another" (resets the form) and Cmd/Ctrl+Enter submit.
|
||||
7. `typecheck`/`lint`/`test`/`build`/`check:size`/`check:colors` green; en/sv parity; no codename; no
|
||||
new npm dependency.
|
||||
|
||||
## Out of scope → follow-ups
|
||||
- Flexible-field grouping/ordering (#45 / detail), per-field server validation rules (#11).
|
||||
- Turning core-field 422s into per-field `FieldRejection`s server-side (the core mutations still throw
|
||||
generic errors; only `setFields` yields field-level codes).
|
||||
- Autosave / draft persistence.
|
||||
Reference in New Issue
Block a user