diff --git a/.claude/skills/ssr-form-migration/reference/gotchas.md b/.claude/skills/ssr-form-migration/reference/gotchas.md index 9758af0d..446ae4e2 100644 --- a/.claude/skills/ssr-form-migration/reference/gotchas.md +++ b/.claude/skills/ssr-form-migration/reference/gotchas.md @@ -108,12 +108,32 @@ should ride in the form, not a parallel snapshot). It bit the percentage-split e 50% then adding a row reverted the first row to its snapshot value, yielding a 66.67/33.33 split. Two ways it shows up and how to handle until the snapshot is gone: -**Fixed (Stage 1 of the wizard→plain-form rewrite):** the operation handlers now read the -live `:step-params` rows (already schema-decoded by `mm/wrap-wizard`, so typed) and fall -back to the snapshot only when absent — typed values survive add/remove/toggle. The -percentage e2e was restored to the realistic type-then-add ordering as a regression guard. -The *full* removal (no snapshot at all; the posted form *is* the state) is the remaining -rewrite stages. +**Fixed (Stage 1):** the operation handlers read the live `:step-params` rows (already +schema-decoded by `mm/wrap-wizard`) so typed values survive add/remove/toggle. + +**Done (Stage 2 — the snapshot round-trip is gone).** The EDN `snapshot` hidden field + +custom readers + `merge-multi-form-state` are removed. A `db/id` hidden rides in the form; +`wrap-derive-state` rebuilds `:multi-form-state` per request from `entity ∪ step-params`, +and `EditWizard.render-wizard` renders a plain form (no snapshot/edit-path/current-step +hiddens). The ~34 `:snapshot` reads still work — `:snapshot` is now a derived map, not a +round-tripped blob. + +**Trap that cost hours — derive `entity ∪ step-params` correctly.** First cut was +`(merge base step-params)`. Bug: `base` always carries the entity's *persisted* accounts, +so after the user removes every row (step-params has no accounts key) the merge falls back +to base → the persisted accounts **resurrect** on the next operation. Fix: editable fields +(accounts, vendor, memo, approval, action, mode, amount-mode) come **only** from the live +form (absent = cleared); only entity-only fields (`db/id`, client, amount, description, +status, type) come from the entity. Lesson: with a posted form, "field absent" means +*cleared*, not "use the persisted value" — never merge the entity's editable fields back in. + +**Verify the snapshot removal on a FRESH server, and don't trust a long-lived in-process +test server.** Protocol/defrecord (`EditWizard.render-wizard`) and middleware reloads do +**not** fully take in a running REPL — the server kept rendering the old snapshot field +after `:reload`, and an in-process server that isn't reseeded between `npx playwright` +invocations accumulates state that makes order-dependent tests flake. Both produced hours +of phantom failures. Restart the REPL clean (or reseed) before trusting an e2e result; CI +boots a fresh server per run, so the fresh-server number (38 pass / 1 unrelated) is the real one. ## Characterization tests rot against table order and removed wizard chrome diff --git a/.claude/skills/ssr-form-migration/reference/scorecard.md b/.claude/skills/ssr-form-migration/reference/scorecard.md index 99126359..e594f44b 100644 --- a/.claude/skills/ssr-form-migration/reference/scorecard.md +++ b/.claude/skills/ssr-form-migration/reference/scorecard.md @@ -39,7 +39,7 @@ Each migration appends one row (after-numbers), referencing the before in the di | Phase | Modal | LOC | Routes | no-cursor twins | faked roots | snapshot merges | OOB | mixed hx- | cookbook reused / added | |-------|-------|-----|--------|-----------------|-------------|-----------------|-----|-----------|-------------------------| | 1 (baseline) | Transaction Edit `transaction/edit.clj` | 1608 | ~12 | 1 | 2 | ~75 | 0 | 8 | — / seeded 7 entries | -| 2 (in progress) | Transaction Edit `transaction/edit.clj` | 1545 | **~5** | **0** | **0** | ~75 | 0 | 8 | — / 0 | +| 2 | Transaction Edit `transaction/edit.clj` | 1584 | **~5** | **0** | **0** | **0 round-trip** | 0 | 8 | — / 0 | > **Phase 2 progress.** Achieved with parity held (swap spec **6/6**, transaction-edit > spec **8/8**, full suite **38 pass / 1 unrelated fail / 0 skip**, up from 30/2/7): diff --git a/e2e/transaction-edit.spec.ts b/e2e/transaction-edit.spec.ts index 7b9dcafa..fb4634d2 100644 --- a/e2e/transaction-edit.spec.ts +++ b/e2e/transaction-edit.spec.ts @@ -124,14 +124,13 @@ async function getAccountLocation(page: any, rowIndex: number): Promise } async function removeAllAccounts(page: any) { - const accountRows = page.locator('#account-grid-body tbody tr.account-row'); - const rowCount = await accountRows.count(); - - for (let i = rowCount - 1; i >= 0; i--) { - const row = accountRows.nth(i); - const removeButton = row.locator('.account-remove-action'); - await removeButton.click(); - // Wait for the Alpine.js removal animation (500ms + buffer) + // Re-query each iteration: every remove is a whole-form swap that re-renders the rows, + // so a row index captured up front goes stale. Click the last remove button until none + // remain. + for (let guard = 0; guard < 20; guard++) { + const removeButtons = page.locator('#account-grid-body .account-remove-action'); + if (await removeButtons.count() === 0) break; + await removeButtons.last().click(); await page.waitForTimeout(700); } } diff --git a/src/clj/auto_ap/ssr/transaction/edit.clj b/src/clj/auto_ap/ssr/transaction/edit.clj index ed9c1518..8c754ff4 100644 --- a/src/clj/auto_ap/ssr/transaction/edit.clj +++ b/src/clj/auto_ap/ssr/transaction/edit.clj @@ -773,7 +773,9 @@ [:div.mt-4 {:hx-post (bidi/path-for ssr-routes/only-routes ::route/unlink-payment) :hx-trigger "unlinkPayment" :hx-target "#payment-matches" - :hx-include "this" + ;; include the whole form so the db/id hidden rides along (the plain + ;; form derives state from db/id instead of a serialized snapshot) + :hx-include "closest form" :hx-swap "outerHTML" :hx-confirm "Are you sure you want to unlink this payment?"} @@ -1327,15 +1329,20 @@ (if current-step (mm/get-step this current-step) (mm/get-step this :links))) - (render-wizard [this {:keys [multi-form-state] :as request}] - (mm/default-render-wizard - this request - :form-params - (-> mm/default-form-props - (assoc :hx-post - (str (bidi/path-for ssr-routes/only-routes ::route/edit-submit)) - :hx-ext "response-targets")) - :render-timeline? false)) + (render-wizard [this {:keys [multi-form-state form-errors] :as request}] + ;; Plain-form render: no snapshot / edit-path / current-step hidden fields. The entity + ;; id rides in the form; all other state is the live form (step-params) re-derived + ;; against the entity on each request (see wrap-derive-state). + (let [step (mm/get-current-step this)] + [:form#wizard-form (-> mm/default-form-props + (assoc :hx-post (str (bidi/path-for ssr-routes/only-routes ::route/edit-submit)) + :hx-ext "response-targets")) + (fc/start-form multi-form-state (when form-errors {:step-params form-errors}) + (list + (com/hidden {:name "db/id" :value (-> multi-form-state :snapshot :db/id)}) + (fc/with-field :step-params + (com/modal {:id "wizardmodal"} + (mm/render-step step request)))))])) (steps [_] [:links]) (get-step [this step-key] @@ -1350,32 +1357,53 @@ (def edit-wizard (->EditWizard nil nil)) -(defn initial-edit-wizard-state [request] - (let [tx-id (-> request :route-params :db/id) - entity (dc/pull (dc/db conn) - '[:db/id - :transaction/vendor - :transaction/client - :transaction/description-original - :transaction/status - :transaction/type - :transaction/memo - {[:transaction/approval-status :xform iol-ion.query/ident] [:db/ident]} - :transaction/amount - :transaction/accounts] - tx-id) - entity (-> entity - (update :transaction/vendor :db/id) - (update :transaction/client :db/id))] - (mm/->MultiStepFormState entity - [] - entity))) +(defn entity->base + "The persisted transaction, shaped like the form's base state (what the old snapshot was + seeded with). The plain form derives its state fresh from this + the live posted form, + instead of round-tripping an EDN snapshot hidden field." + [tx-id] + (-> (dc/pull (dc/db conn) + '[:db/id + :transaction/vendor + :transaction/client + :transaction/description-original + :transaction/status + :transaction/type + :transaction/memo + {[:transaction/approval-status :xform iol-ion.query/ident] [:db/ident]} + :transaction/amount + :transaction/accounts] + tx-id) + (update :transaction/vendor :db/id) + (update :transaction/client :db/id))) -(defn- render-account-grid-body [request] - (fc/start-form (:multi-form-state request) nil - (fc/with-field :step-params - (fc/with-field :transaction/accounts - (account-grid-body* request))))) +(defn wrap-derive-state + "Plain-form replacement for the EDN-snapshot round-trip. Builds :multi-form-state from + the entity (loaded by the db/id hidden field, or the route on initial open) overlaid + with the live posted step-params -- no serialized snapshot. Runs after wrap-decode / + wrap-wizard, which provide nested + schema-typed step-params. The 30-odd `:snapshot` + reads keep working: snapshot is now `entity ∪ step-params`, derived per request." + [handler] + (fn [request] + (let [tx-id (->db-id (or (some-> request :form-params (get "db/id")) + (-> request :route-params :db/id))) + base (entity->base tx-id) + posted (-> request :multi-form-state :step-params) + ;; Fields the form does NOT edit always come from the entity. Everything else is + ;; the live posted form, which is authoritative even when ABSENT -- an absent + ;; field means the user cleared it (e.g. removed all account rows), not "fall + ;; back to the entity's persisted value". Merging base's editable fields back in + ;; would resurrect persisted accounts after a remove-all. + entity-only (select-keys base [:db/id :transaction/client :transaction/amount + :transaction/description-original + :transaction/status :transaction/type]) + ;; On initial open there is no posted form -> render the entity. On every post + ;; the form is authoritative for the editable fields. + step-params (if (seq posted) posted base) + snapshot (if (seq posted) (merge entity-only posted) base)] + (handler (-> request + (assoc :entity (d-transactions/get-by-id tx-id)) + (assoc :multi-form-state (mm/->MultiStepFormState snapshot [] step-params))))))) (defn render-full-form "Helper to render the complete transaction edit form for whole-form re-rendering." @@ -1518,18 +1546,18 @@ (apply-middleware-to-all-handlers {::route/edit-wizard (-> mm/open-wizard-handler (wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client))) + (wrap-derive-state) (mm/wrap-wizard edit-wizard) - (mm/wrap-init-multi-form-state initial-edit-wizard-state) - (wrap-entity [:route-params :db/id] d-transactions/default-read) + (mm/wrap-decode-multi-form-state) (wrap-schema-enforce :route-schema [:map [:db/id entity-id]])) ::route/edit-wizard-navigate (-> mm/next-handler (wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client))) - (wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read) + (wrap-derive-state) (mm/wrap-wizard edit-wizard) (mm/wrap-decode-multi-form-state)) ::route/edit-submit (-> mm/submit-handler (wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client))) - (wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read) + (wrap-derive-state) (mm/wrap-wizard edit-wizard) (mm/wrap-decode-multi-form-state)) ::route/location-select (-> location-select @@ -1540,16 +1568,14 @@ [:account-id {:optional true} [:maybe entity-id]]])) ::route/edit-form-changed (-> edit-form-changed-handler + (wrap-derive-state) (mm/wrap-wizard edit-wizard) - (wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read) (mm/wrap-decode-multi-form-state)) ::route/unlink-payment (-> unlink-payment (wrap-must {:activity :edit :subject :transaction} (fn get-client [request] (-> request :entity :transaction/client))) - (wrap-entity [:multi-form-state :snapshot :db/id] d-transactions/default-read) + (wrap-derive-state) (mm/wrap-wizard edit-wizard) - (mm/wrap-decode-multi-form-state) - #_(wrap-schema-enforce :form-schema - save-schema))} + (mm/wrap-decode-multi-form-state))} (fn [h] (-> h (wrap-client-redirect-unauthenticated)))))