fix(reports): remove dead attachmentsNote override and refactor ReportWizardPage to useReducer

[steilerDev]

Summary

• applyOverrides still honours row.<id>.attachmentsNote but the editor renders it as static text #1967 — Remove dead attachmentsNote override from applyOverrides.ts and overrideKeys.ts; field has been static read-only text since PR fix(reports): improve report PDF UX — paragraph breaks, inline meta, inline notes, column toggles #1959, no UI path can produce its override key; adds regression test pinning the removal
• refactor(reports): ReportWizardPage transition state to a useReducer state machine — four defects of one shape in one batch #1947 — Refactor ReportWizardPage from 38 useState/useRef hooks to a useReducer state machine in wizardReducer.ts; staleness now enforced via opaque request-id tokens (M1/M2 fixes); named tier types with explicit factory return-type annotations enforce AC4 at compile time; 57 unit tests at 100% coverage; behaviour-preserving (existing tests unchanged per AC3)

Fixes #1967
Fixes #1947

Test plan

• Unit tests: wizardReducer.test.ts — 57 tests, 100% coverage
• AC5 regression tests for all four defect patterns (BUG: changing the report use case keeps the previously fetched report — a claim report can embed quotation-tier documents #1943 cascade, AC8 deep-link, M1 stale report load, M2 stale AI generation)
• applyOverrides.test.ts / overrideKeys.test.ts — regression test pins dead-code removal
• npm run lint — zero findings in all PR files

🤖 Generated with Claude Code

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com

[steilerDev]

[ux-designer]

Design review of PR #1988 (stories #1967 + #1947).

Verdict: APPROVED

---

Scope reviewed

File	Nature
client/src/lib/reportContent/applyOverrides.ts	Pure function, no JSX/CSS
client/src/lib/reportContent/overrideKeys.ts	Pure type factory, no JSX/CSS
client/src/lib/reportContent/types.ts	Type definitions and comment update only
client/src/pages/ReportWizardPage/wizardReducer.ts	Pure state logic, no JSX/CSS
client/src/pages/ReportWizardPage/ReportWizardPage.tsx	State wiring changes only

---

Token adherence

No CSS files are modified. No hardcoded colors, spacing, radii, or font sizes are introduced anywhere. All existing var(--token-name) references in the component are untouched.

Dark mode

No color values changed. Unaffected.

Accessibility

All existing ARIA attributes are preserved in ReportWizardPage.tsx:

• aria-live="polite" on the AI generating caption — retained
• aria-describedby="enhanceWithAiDescription" on the "Enhance with AI" button — retained
• aria-hidden="true" on the <Spinner> inside the button — retained
• stepHeadingsRef for focus management on step headings — preserved

Discard modal condition change

The condition change from the inline expression isGeneratingAi && Object.keys(overrides).length === 0 && aiContent === null to isGeneratingOnly(wizardState) is semantically equivalent. isGeneratingOnly returns state.aiRequestId !== null && !hasManualEdits(state) && state.aiContent === null, which evaluates identically in all reachable states. Same user-visible strings, same branching logic.

AI elapsed timer effect

The cleanup restructure — moving setAiElapsed(0) from the early-return branch into the cleanup function — is functionally equivalent. The reset fires when isGeneratingAiValue transitions to false in both the old and new versions.

Component reuse

No new components or CSS patterns introduced. No component reuse violations.

---

No non-blocking findings.

[steilerDev]

[product-architect]

Verdict: CHANGES_REQUESTED

One blocking finding (HIGH, ~4 lines, zero behaviour change). Everything else below is non-blocking.

This is a strong refactor overall — the tier partition is exactly the shape I asked for in the action-set review (named tier interfaces, explicit factory return-type annotations, total object literals), the opaque-nullable-token model is implemented correctly, isGeneratingAi is derived rather than stored, the third untokenized race I flagged (the Step-2 fan-out) is now tokenized via STEP2_AMOUNTS_LOADED, and deepLinkAppliedRef no longer keeps the AC8 trigger alive behind a comment. Full CI is green including all 16 E2E shards and E2E Gates, and wizardReducer.ts is at 100/100/100/100 with 57 tests — I re-ran it locally to confirm.

---

Blocking

H1 — AC4's forcing function has a hole in exactly the two cases that need it most

freshContentTier() (wizardReducer.ts:80-82) is the AC4 mechanism for ContentTier, but the two cases that clear content state bypass it and hand-list the fields instead:

• SELECT_SOURCE (wizardReducer.ts:159-162) — overrides: {}, aiContent: null, aiRequestId: null + a comment saying aiError is deliberately preserved
• DISCARD_EDITS (wizardReducer.ts:263-269) — the same four fields, hand-listed

Adding a fifth field to ContentTier produces a compile error in freshContentTier() (good — that is AC4 working), but SELECT_SOURCE and DISCARD_EDITS will silently keep it. That is verbatim the failure mode AC4 exists to eliminate ("rather than silently defaulting to kept"), reintroduced in the very handler whose hand-maintained reset list produced #1943 and M2. A future aiWarnings / aiModelUsed / aiTruncated field surviving a source change is a new defect of the identical shape.

The fix is to compose from the factory and name the single exception, which is byte-for-byte behaviour-identical and keeps every existing test (including the M-I aiError-preservation test) passing:

case 'SELECT_SOURCE':
  return {
    ...state,
    ...freshReportTier(),
    ...freshContentTier(),
    // M-I: handleSourceChange never cleared aiError — the ONLY ContentTier field kept.
    aiError: state.aiError,
    reportRequestId: action.payload.requestId,
    sourceId: action.payload.sourceId,
    currentStep: Math.min(state.currentStep, 3),
    maxReachedStep: 3,
  };

case 'DISCARD_EDITS':
  return {
    ...state,
    ...freshContentTier(),
    // Cleared only when a generation was in flight (original guardedUpdate behaviour).
    aiError: state.aiRequestId !== null ? '' : state.aiError,
  };

After this, a new ContentTier field is cleared by default in both cascades and any intended KEEP has to be written down explicitly — which is what AC4 asks for. Please also add one test that pins the property rather than the field list, e.g. assert that after SELECT_SOURCE every ContentTier field except aiError equals its fresh value.

Same structural note, informational only: SELECT_USE_CASE and SELECT_SOURCE set NavTier fields explicitly rather than via freshNavTier(), so a third NavTier field would also default to kept. NavTier has two fields and neither cascade should reset them wholesale, so I am not asking for a change — just be aware the guarantee is tier-scoped, not global.

---

Non-blocking (follow-ups / nits)

M1 — handleGenerateWithAiClick depends on the whole state object (ReportWizardPage.tsx:577,584). [wizardState, runAiGeneration] invalidates the callback on every dispatch, including each override keystroke. Lift the selector into render and depend on the boolean:

const hasEdits = hasManualEdits(wizardState);
// ...
}, [hasEdits, runAiGeneration]);

No user-visible impact today (the callback goes to a plain <button>), but it is the kind of dependency that makes a later memo() on a child silently useless.

M2 — REPORT_REFRESHED is now the only async write with no request token. ReportWizardPage.tsx:486 dispatches it from the claim-error refetch path. The state.report === null guard covers the cleared case, but not "a report for a different source has since loaded" — that response would still overwrite it. Risk is genuinely low (the claim modal is open across the await, so the source cannot be changed), and the pre-refactor code had no guard at all, so this is an improvement, not a regression. But it is the last instance of the shape #1947 was filed about; consider echoing sourceId/useCase in the payload and no-op'ing on mismatch, or file it as a follow-up so it is not rediscovered as a bug.

L1 — duplicated status union. ReportTier.reportStatus is typed 'loading' | 'ready' | 'error' (wizardReducer.ts:23), which restates PageStatus in ReportWizardPage.tsx:61. Export the union from wizardReducer.ts and have the page alias it so the two cannot drift.

L2 — default: return (action satisfies never, state); (wizardReducer.ts:282). It does work as an exhaustiveness guard, but the comma operator is obscure enough that a future reader may "clean it up" and remove the guard. Prefer the conventional form:

default: {
  const _exhaustive: never = action;
  return state;
}

L3 — Prettier drift. npx prettier --check client/src/pages/ReportWizardPage/wizardReducer.test.ts fails (the isGeneratingOnly block, roughly lines 766-794, has collapsible wrapping). CLAUDE.md's Local Validation Policy requires npm run format to be clean before commit; Prettier is not CI-gated here, which is why it slipped past green CI. Fix with npx prettier --write on that file only — do not run a repo-wide npm run format, which regenerates unrelated union-type drift.

L4 — nextRequestId test couples to the implementation. expect(Number(id2)).toBe(Number(id1) + 1) (wizardReducer.test.ts:101) pins the monotonic counter, but the whole design decision is that the token is opaque and never compared for order. A later switch to crypto.randomUUID() would fail a test asserting a property the design explicitly disclaims. Assert uniqueness and non-emptiness instead.

L5 — docblock count. wizardReducer.test.ts:7 says "all 20 action types"; there are 21.

L6 — #1967 AC1 not satisfied. AC1 requires the option-(a) decision recorded as a comment on #1967. The reconciliation comment was posted on #1941 (satisfying AC5), but #1967 itself currently has zero comments. Please post the decision there for traceability.

---

Verified (no action needed)

Behaviour-equivalence checks I walked line by line, since AC3 makes this a behaviour-preserving refactor:

• GO_TO_STEP now bumps maxReachedStep at every call site, where the old code bumped it only on the step-3 and step-4 Next buttons. Verified no-op: the step-1 Next button only renders when useCase is truthy, by which point SELECT_USE_CASE has already set maxReachedStep: 2; and WizardStepper.tsx:54-56 only makes steps <= maxReachedStep clickable, so the stepper's Math.max can never raise it. All Back buttons target a step below the current max.
• currentStep: Math.min(...) clamps in both cascades are new semantics but unreachable — the use case is only selectable while currentStep === 1 and the source only while currentStep === 2 (the deep-link path fires at step 1). Defensive, not a behaviour change.
• freshContentTier() in SELECT_USE_CASE is a no-op in the clean path (isDirty covers all three content fields, so a non-dirty state already has them empty) and the dirty path dispatches DISCARD_EDITS first via guardedUpdate. aiError: '' matches the old explicit setAiError(''). Both dispatches land in one batch and apply sequentially to the reducer.
• aiElapsed timer inversion (reset moved out of the false-branch body into the cleanup) is equivalent; the only delta is a setState during unmount-while-generating, a silent no-op in React 19.
• deepLinkAppliedRef upgrade + report removed from the deps preserves fire-once semantics: sourceIdFromQuery is immutable per mount (this page never calls setSearchParams), and the previously-reachable-in-principle !report short-circuit is unreachable because report can only be non-null after a source fetch, which requires a use case.
• AI_GENERATION_BLOCKED leaving aiRequestId untouched matches the old early return (the empty-selection guard runs before AI_GENERATION_STARTED, so the token is null there).
• applyOverrides still honours row.<id>.attachmentsNote but the editor renders it as static text #1967 dead-code removal is complete and safe. overrideKey.row() and applyOverrides were the only producer/consumer of the key; the derived ReportContentRow.attachmentsNote field is untouched, so the read-only inline meta rendering and the PDF output are unaffected. The remaining 'row.<id>.attachmentsNote' literals in ReportContentEditor.test.tsx are deliberate absence-assertions and still valid. The new positive/negative control pair in applyOverrides.test.ts pins the removal properly — the negative test would fail if the branch were reinstated, and the positive control proves the row loop is still wired.
• Architecture/contract surface: none. No endpoints, no schema, no migrations, no env vars, no ADR-034 PDF-pipeline change — no wiki update is owed by this PR.
• I also reviewed the .claude/agent-memory/product-architect/recurring-patterns.md additions: they accurately record the action-set review rulings and I am keeping them as written.

Re-request me once H1 is addressed; I do not need another full pass on the rest.

[steilerDev]

[product-architect]

Verdict: APPROVED

Re-reviewed 01d8ff12 against my previous round. H1 is resolved and L3 is fixed.

H1 verified resolved

SELECT_SOURCE (wizardReducer.ts:151-163) and DISCARD_EDITS (wizardReducer.ts:260-266) now both spread ...freshContentTier() and name the single exception on the following line:

• SELECT_SOURCE → aiError: state.aiError (M-I: the source change never cleared it)
• DISCARD_EDITS → aiError: state.aiRequestId !== null ? '' : state.aiError (cleared only when a generation was in flight)

AC4 is now total for ContentTier: a fifth field added to the type produces one compile error, in freshContentTier(), and both cascades pick up its fresh value by default. The conditional in DISCARD_EDITS still reads state.aiRequestId (pre-spread), so the clear condition is unaffected by the spread. I re-grepped the reducer for hand-listed ContentTier keys: the only remaining ones are the factory itself (L81), the two named exceptions (L159, L265), and the AI-lifecycle transitions (L250/253/256/258) — those are targeted single-field mutations, not tier resets, so "keep the rest" is the correct default there. No hole left.

Behaviour is unchanged: freshContentTier() yields exactly the three values that were hand-listed plus aiError: '', which both cases then override back. The M-I aiError-preservation test and the two DISCARD_EDITS tests still pass unmodified, which is the pin that matters.

L3 verified resolved

npx prettier --check is clean on both wizardReducer.ts and wizardReducer.test.ts, and the reformat was correctly scoped to the one file — no repo-wide drift rode along in the commit.

Verification run

• wizardReducer.test.ts: 57 passed, 100% statements / branches / functions / lines on wizardReducer.ts (re-ran locally on the fix commit).
• CI on 01d8ff12: Quality Gates: success (the required beta gate), Static Analysis, Trailer Check, all 6 Jest shards, Docker, Coverage Report, E2E Smoke all green; 13 of 16 full E2E shards green with 3 still running (not a required check for a beta-targeted PR — worth a glance before merge but not a gate).

Non-blocking items carried forward from round 1

Unchanged and explicitly not blocking; either fold into a follow-up or drop as you see fit: M1 handleGenerateWithAiClick depending on the whole wizardState, M2 REPORT_REFRESHED being the last untokenized async write, L1 reportStatus union duplicating PageStatus, L2 the comma-operator exhaustiveness guard, L4 nextRequestId test pinning the monotonic counter the design declares opaque, L5 the "20 action types" docblock (there are 21).

One new informational nit, not worth a commit on its own: in SELECT_SOURCE the ...freshContentTier() spread now sits after the explicit reportRequestId write. Harmless today because the six tier types are pairwise disjoint, but the convention that reads safely is all fresh-tier spreads first, then all explicit field writes — if two tiers ever shared a key, the current ordering would clobber silently and still typecheck.

Still owed outside this PR

#1967 AC1 — the option-(a) decision is recorded on #1941 (AC5) but #1967 itself still has no comment. Please post it there before closing the issue; it is the traceability artifact the AC asks for, and it does not block the merge.

[github-actions]

🎉 This PR is included in version 2.13.1-beta.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 2.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀