fix(schedule3-subpage): address PR #344 review — one parser, gated commit, keyed contract

All three findings confirmed against the code; the reviewer's analysis of the
parser split was exactly right (toNum yields 1000/16/1234 for 1e3/0x10/12,34
where committedNum yields null).

1. The row cells and the footer now share one parse. `numeric()` moves from lax
   `toNum` to strict `committedNum`, so a derived Crown cannot disagree with the
   Subtotal Crown above it. Also closes the sidebar: `toNum` had no finiteness
   guard, so Infinity rendered as ∞.

2. The blur commit now holds invalid and unusable entries, as every other
   surface does. One correction to the review here: gating on `errs` would not
   have worked. `rowErrors` is populated only by `persist`
   (useEditableCostRows.ts:204) — i.e. on a Save attempt — so it is still empty
   during the entry the gate has to catch, and the field is not marked invalid
   until Save either. The gate runs `config.validate` on the blurred value
   instead.

3. The `deriveSummary` doc comment now states the keyed contract and points at
   the note in schedule3OtherAcceptableCosts/derived.ts that explains why.

Three tests added, each mutation-verified against the specific fix it covers.
Finding 1 needed the in-flight-add path to be observable at all: once the commit
gate is in place, a lax-only value can never reach `committed`, so the only
surface where the two parsers still meet is a locally added row, which falls back
to its live values while its PUT is in flight — and `1e3` clears validation, so
it can be added.

Also dropped a fourth test I had written for "one field's blur commits its
neighbours' half-typed entries": that is unreachable, because moving focus to
another field blurs the first one. The `prev[row.key]` merge is kept as a
correctness tidy, not a bug fix, and is commented as such.

Suite 1294/1296 (two timeout flakes, 221/221 in isolation), zero unhandled
errors, build clean, no new type errors.

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

Author: Rylan-cgi

frontend/src/components/schedule3OtherAcceptableCosts/__tests__/OtherAcceptableCosts.test.tsx

@@ -101,6 +101,54 @@ describe('Other Acceptable Costs sub-page (Story 4.4) — edit-in-place + batch
     expect(footerCells()).toEqual(['600', '500', '100'])
   })
 
+  test('a lax-only entry moves NEITHER the row Crown nor the footer (#344 review)', async () => {
+    // `1e3` is the case where the page's two parsers disagreed: `validateOtherAcceptable` accepts it
+    // (`Number('1e3')` is 1000, in range, so no error is shown), the row's derived Crown used lax
+    // `toNum` and read 1000, and the footer mirror used strict `committedNum` and read null. The page
+    // then contradicted itself — Crown 700 in the row, Crown 100 in the footer, over a Total $ field
+    // displaying `1e3`. Two fixes meet here: the derived columns now parse with `committedNum` like the
+    // footer, and a strictly-unparseable entry no longer commits at all. `0x10` and `12,34` are the
+    // same class.
+    server.use(http.get(URL, () => HttpResponse.json(doc)))
+    render(<OtherAcceptableCostsPage />)
+    const user = userEvent.setup()
+    await screen.findByDisplayValue('Consulting')
+
+    const total = within(rowOf('Consulting')).getByLabelText('Edit total')
+    await user.clear(total)
+    await user.type(total, '1e3')
+    await user.tab()
+
+    // Both derived surfaces HOLD the last committed figures, and agree with each other.
+    expect(within(rowOf('1e3')).getByText('500')).toBeInTheDocument() // 800 − 300, as served
+    expect(footerCells()).toEqual(['1,400', '500', '900'])
+  })
+
+  test('an out-of-range entry holds the footer while the field sits invalid (#344 review)', async () => {
+    // The rule `useCommittedValues` documents and every other surface followed: an invalid entry holds
+    // its previous committed value, because legacy's failed round-trip left the last valid figures on
+    // screen. This sub-page committed unconditionally, so 999,999,999 (over the ±99,999,999 cost range)
+    // drove the footer to a figure the server can never produce while the field sat red beside it.
+    server.use(http.get(URL, () => HttpResponse.json(doc)))
+    render(<OtherAcceptableCostsPage />)
+    const user = userEvent.setup()
+    await screen.findByDisplayValue('Consulting')
+
+    const total = within(rowOf('Consulting')).getByLabelText('Edit total')
+    await user.clear(total)
+    await user.type(total, '999999999')
+    await user.tab()
+
+    // The entered text stays on screen for correction...
+    expect(within(rowOf('999,999,999')).getByLabelText('Edit total')).toHaveValue('999,999,999')
+    // ...but the footer holds the last figures the server could actually produce.
+    //
+    // NOTE this page does not mark the field invalid until a Save is attempted (`rowErrors` is filled
+    // by `persist` alone), so there is no red state to assert here yet. That is why the commit gate
+    // runs `config.validate` itself instead of reading `rowErrors`.
+    expect(footerCells()).toEqual(['1,400', '500', '900'])
+  })
+
   test('a locally added row counts toward the footer while its PUT is in flight (#291)', async () => {
     // PROVEN by the code review: the footer fell back to `{}` for a row appended before its PUT
     // landed, so it silently omitted a row visibly sitting in the grid. It now falls back to the
@@ -126,6 +174,34 @@ describe('Other Acceptable Costs sub-page (Story 4.4) — edit-in-place + batch
     })
   })
 
+  test('an in-flight added row derives its Crown with the FOOTER\'s parser (#344 review)', async () => {
+    // The one path where the row/footer parser split is still observable. A row appended before its
+    // PUT lands is absent from `committed`, so `committedFor` falls back to its LIVE values — and
+    // `1e3` clears `validateOtherAcceptable` (`Number('1e3')` is 1000, in range), so it can be added.
+    // With the row cells on lax `toNum` and the footer on strict `committedNum`, the row read Crown
+    // 1,000 while the footer excluded it from Harvest entirely: 1,400 under a visible row of 1e3.
+    server.use(
+      http.get(URL, () => HttpResponse.json(doc)),
+      http.put(URL, async () => {
+        await delay(50)
+        return HttpResponse.json(doc)
+      }),
+    )
+    render(<OtherAcceptableCostsPage />)
+    const user = userEvent.setup()
+    await screen.findByDisplayValue('Consulting')
+
+    await user.type(screen.getByLabelText('Description'), 'Exponential')
+    await user.type(screen.getByLabelText('Total $'), '1e3')
+    await user.click(screen.getByRole('button', { name: /^add$/i }))
+
+    const added = await screen.findByDisplayValue('1e3')
+    const row = added.closest('tr') as HTMLElement
+    // One parser, so the row agrees with the footer: neither counts the unparseable entry.
+    expect(within(row).queryByText('1,000')).not.toBeInTheDocument()
+    expect(footerCells()).toEqual(['1,400', '500', '900'])
+  })
+
   test('lists groups as editable inputs with live-derived crown + subtotal + add form', async () => {
     server.use(http.get(URL, () => HttpResponse.json(doc)))
     render(<OtherAcceptableCostsPage />)

frontend/src/components/schedule3SubPage/index.tsx

@@ -15,6 +15,7 @@ import {
 } from '@carbon/react'
 import { TrashCan } from '@carbon/icons-react'
 import { fmtNumber, groupInput, numStrGroup, toNum } from '@/utils/number'
+import { committedNum, isUnusableStrictEntry } from '@/utils/derivedMath'
 import EditableSubPageLayout from '@/components/core/EditableSubPageLayout'
 import SubPanel from '@/components/core/SubPanel'
 import { useEditableCostRows, type EditRow } from '@/hooks/useEditableCostRows'
@@ -100,7 +101,11 @@ export interface Schedule3SubPageConfig<
   summaryItems: Schedule3SubPageSummaryItem<TDoc>[]
   /**
    * Optional display-only mirror for the Totals footer (defect #291): given the blur-committed row
-   * values, return one figure per `summaryItems` entry, in the same order. Present only on the pages
+   * values, return a figure per `summaryItems` entry KEYED BY that entry's `key` — not a positional
+   * list. The keyed contract is deliberate (see `schedule3OtherAcceptableCosts/derived.ts`, where
+   * `OtherAcceptableSubtotal` is "keyed so it cannot be mis-paired with the summary labels
+   * positionally"): a triple looked up by name cannot silently pair with the wrong label when a
+   * column is added or reordered. Present only on the pages
    * whose legacy footer refreshed during entry — Other Acceptable Costs, whose Total $ / PO&P $
    * handlers rendered `footerValues` (schedule3SubtotalOtherCosts.xhtml:74,83). The Included
    * Unacceptable page omits it: its only handler was `render="cost"`, with no derived target, so
@@ -162,8 +167,20 @@ function Schedule3SubPage<TRow extends Schedule3SubPageRow, TDoc extends Schedul
   }, [editor.data])
 
   // Numeric view of a row's entered values, for the live-derived read-only columns (e.g. Crown $).
+  //
+  // `committedNum`, NOT `toNum` (PR #344 review): this must be the SAME parse the footer mirror uses
+  // (`deriveOtherAcceptableSubtotal` → `committedNum` → `parseDecimalInput`), or the two disagree on
+  // every form the lax parser accepts and the strict one rejects. `1e3` was the clearest case —
+  // `validateOtherAcceptable` passes it (`Number('1e3')` is 1000, in range), the row's Crown $ showed
+  // `1,000 − pop`, and the footer excluded the row entirely because `committedNum('1e3')` is null. Same
+  // for `0x10` → 16 and a mis-grouped `12,34` → 1234. That is the same self-contradiction the comment
+  // on `rowCells` says was ruled out — row Crowns of 700 and 400 under a Subtotal Crown of 900 —
+  // reached by a parser mismatch instead of a timing one.
+  //
+  // It also closes a smaller gap the review noted in passing: `toNum` has no finiteness guard, so
+  // `Infinity` rendered `∞` in Crown $. `committedNum` rejects it, and the wire never carried it.
   const numeric = (values: SubPageValues): Record<string, number | null> =>
-    Object.fromEntries(config.fields.map((f) => [f.key, toNum(values[f.key])]))
+    Object.fromEntries(config.fields.map((f) => [f.key, committedNum(values[f.key] ?? '')]))
 
   // Client-side column sort, matching the legacy Schedule 3 sub-page dataTables: Description, every
   // editable field, AND each derived read-only column (e.g. Crown $) are sortable. See useRowSort for
@@ -219,14 +236,34 @@ function Schedule3SubPage<TRow extends Schedule3SubPageRow, TDoc extends Schedul
                 size="sm"
                 value={row.values[field.key] ?? ''}
                 onChange={(e) => setRowValue(row.key, field.key, e.target.value)}
-                // Re-group on blur only — regrouping mid-keystroke would fight the caret.
+                // Re-group on blur only — regrouping mid-keystroke would fight the caret. The
+                // re-group is cosmetic and unconditional (`groupInput` returns invalid text
+                // unchanged, so a typo stays on screen); only the COMMIT is gated below.
                 onBlur={() => {
                   const grouped = groupInput(row.values[field.key] ?? '')
                   setRowValue(row.key, field.key, grouped)
-                  // Commit the grouped string, so `committed` and the field hold the same text.
+                  // An invalid or unusable entry HOLDS its previous committed value (PR #344 review).
+                  // This is the rule `useCommittedValues` documents and every other surface already
+                  // followed — Schedule 4 reimplements the same guard. Without it an out-of-range
+                  // 999,999,999 in Total $ moved the footer to a figure the server can never produce.
+                  //
+                  // Validate HERE rather than reading `rowErrors`: that map is populated only by
+                  // `persist` (useEditableCostRows.ts:204), i.e. on a Save attempt, so it is still
+                  // empty during the entry this gate has to catch. `errs` below is right for the
+                  // field's `invalid` styling and wrong as a commit gate.
+                  const next = { ...row.values, [field.key]: grouped }
+                  const blurErrs = config.validate(row.description, next)
+                  if (blurErrs[field.key] !== undefined || isUnusableStrictEntry(grouped)) {
+                    return
+                  }
+                  // Merge onto the row's previous committed values rather than the live `row.values`,
+                  // so a blur advances only the field that blurred. Not a reachable bug today —
+                  // moving focus to another field blurs this one first — but it keeps the snapshot's
+                  // meaning exact rather than relying on focus ordering.
                   setCommitted((prev) => ({
                     ...prev,
-                    [row.key]: { ...row.values, [field.key]: grouped },
+                    // Commit the grouped string, so `committed` and the field hold the same text.
+                    [row.key]: { ...(prev[row.key] ?? row.values), [field.key]: grouped },
                   }))
                 }}
                 invalid={Boolean(errs[field.key])}