fix(reports): discard the fetched report when the use case changes (#1943)

[steilerDev]

Summary

• Changing the use case left report and sourceId intact, and step 2's Next button is gated only on sourceId — so a user could pick a use case, pick a source, go back to step 1, pick a different use case, and click straight through to step 3 still holding a report fetched under the previous use case.
• Post-Attachment tier rules per report type (quotation -> deposit -> invoice) replace per-invoice stage matching #1930 this was no longer just a stale-figures cosmetic bug: a budget-overview → claim switch along that route could carry quotation-tier documents into an exported claim PDF handed to a bank — defeating Attachment tier rules per report type (quotation -> deposit -> invoice) replace per-invoice stage matching #1930's AC2 via a route it doesn't cover.
• The reset clears report, reportStatus, sourceId, and both invoice/line exclusion sets — inside the existing guardedUpdate discard-confirmation callback, so unsaved edits or AI-generated content still prompt for confirmation first and only clear once confirmed.
• A one-shot deepLinkAppliedRef stops the ?sourceId= deep-link effect (keyed on !report) from re-arming once report is cleared and silently re-selecting the original query-string source under the new use case.

Fixes #1943

Notes for the record

• AC4 (a report exported under use case X is identical to one produced from a clean start under X, in both directions and for every use-case pair) is satisfied structurally by this fix — both paths converge on the same handleSourceChange call under the current useCase closure — but is not independently tested here. The deeper determinism claim belongs to getSourceReport, already covered by Attachment tier rules per report type (quotation -> deposit -> invoice) replace per-invoice stage matching #1930/feat(reports): filter report attachments by document tier per report type (#1930) #1942's own suite. A cross-direction diff test would be a nice-to-have follow-up.
• The new E2E scenarios (13/14) have never been executed locally — Playwright's Chromium is network-policy-blocked in this sandbox (verified: no browser binary present at all), so CI is their first real run.

Test plan

• Unit tests pass locally (72/72 in ReportWizardPage.test.tsx, 26/26 in ReportWizardPage.aiGeneration.test.tsx — unaffected/no regression)
• E2E Scenarios 13/14 pass in CI (first execution)
• CI Quality Gates pass (typecheck, tests, build, audit)

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
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com

[steilerDev]

[product-architect] Architecture / test-coverage / code-quality review of PR #1945 (#1943).

Verdict: APPROVED — no critical or high findings. Three medium and two low findings below are non-blocking follow-ups; one of them (M1) I'd like filed and fixed inside this Bank Report Wizard cluster before the beta -> main promotion, for the reason #1943 was Must Have in the first place.

What I verified

• The five clears sit inside guardedUpdate's callback (ReportWizardPage.tsx L215–222), so AC7 holds in both directions — confirmed by the two new confirm/cancel tests, and the cancel test also proves the pending change never partially applied.
• handleSourceChange closes over useCase with [useCase, guardedUpdate] as its dep array, so re-selecting a source after a use-case change genuinely fetches under the new type. The step-2 disabled={!sourceId} gate (L705) is now re-locked, which is the actual hole BUG: changing the report use case keeps the previously fetched report — a claim report can embed quotation-tier documents #1943 described.
• includeCoverLetter is unconditionally recomputed on every report load (L259) — correctly excluded from the reset.
• overrides / aiContent are cleared by guardedUpdate whenever they are non-empty (they are the dirty predicate), so the "already cleared" justification is exact, not approximate.
• Ran client/src/pages/ReportWizardPage/ReportWizardPage.test.tsx locally: 72/72 pass.
• No schema, API contract, or ADR impact. Client-only diff — no wiki update owed.

Answers to the three questions raised on handback

1. Is AC4's "structural corollary" reasoning sound? — Mostly, but there is one real gap

The reasoning is correct for the deterministic path: after the fix, step 3 is reachable only via a handleSourceChange executed under the current useCase closure, and per-(type, sourceId) determinism belongs to getSourceReport (covered by #1930/#1942). I accept that a cross-pair diff test would be redundant with the backend suite.

But AC4 says always identical, and two history-dependent inputs survive:

• M1 below — an in-flight fetch from the pre-change source can still win a race and re-populate report. This is a genuine "history influences the result" path, not a hypothetical.
• L2 below — sticky user preferences (attachDocuments, reportLanguageOverride) mean X -> Y -> X is not byte-identical to a clean X. Correct by design, but literally outside AC4's wording.

So: the reasoning is sound about why the fetch is right; it is not sufficient to establish AC4 as literally stated. That is an AC-precision issue plus M1, not a defect in this change.

2. Does deepLinkAppliedRef fully close the reentrancy? — Yes, for every path I can reach today

• Step-3 manual Retry (L730–733): calls handleSourceChange(sourceId) with a non-null sourceId; it never touches sourceIdFromQuery and never consults the ref. Safe.
• Remount: the ref resets on remount, which is correct — a fresh page load should re-apply the deep link. I verified the only producer of ?sourceId= is BudgetSourcesPage.tsx:1318 (navigate('/budget/reports?sourceId=...')) from a different route, so it always remounts, and ReportWizardPage never calls setSearchParams. The comment's immutability premise checks out as written.
• Unremarked side benefit: the ref also closes a latent pre-existing hazard. Pre-fix, when a deep-link fetch failed, report stayed null indefinitely, so any later change to overrides/aiContent re-identified guardedUpdate -> handleSourceChange -> the effect's deps, silently re-firing the deep-link fetch. It can't now. Worth knowing that this ref is load-bearing for more than AC8.

One cheap hardening (L3): make it useRef<string | null>(null) holding the applied source id rather than a bare boolean. Same cost, and it removes the landmine if a second ?sourceId= producer is ever added on the same route — today's "unreachable" is only true because there is exactly one producer, and that fact lives nowhere but the comment.

3. Is reportStatus: 'loading' equivalent to initial mount (AC6)? — Yes, exactly

'loading' is literally the useState initial value (L110), so the value is identical, and there is no render difference on the way through: handleUseCaseChange is reachable only from Step1UseCase, maxReachedStep drops to 2 in the same commit, and step 3's reportStatus === 'loading' && <Skeleton/> branch is not mounted. The AC6 test correctly has to route through a real re-selection to observe anything at all — that is the right shape for this assertion, not a weakness.

Minor observation only, no change requested: 'loading' with report === null and sourceId === null is semantically an idle state spelled 'loading'. Harmless because the step is unreachable in that combination; noting it because a future change that makes step 3 reachable without a source would surface an indefinite skeleton.

Findings

M1 — Medium: the reset clears state but does not invalidate the in-flight write that will re-populate it

handleSourceChange (L245–270) starts getSourceReport(useCase, sid) with no abort and no staleness token. The reset added here clears report, but a fetch already in flight from the previous use case still resolves into setReport(r) / setReportStatus('ready').

The benign case is safe — I checked: if only the stale fetch resolves, sourceId is null and maxReachedStep is 2, so step 3 stays unreachable and the next source selection overwrites report. The problematic case is out-of-order resolution:

1. A = getSourceReport('budget-overview', src) in flight.
2. User goes back, picks claim -> reset fires.
3. User re-picks the same source -> B = getSourceReport('claim', src).
4. If A settles after B, setReport(A) wins — while sourceId is set, maxReachedStep is 3, and reportStatus is 'ready'.

That is precisely the #1943 end state: step 3 reachable holding a report fetched under budget-overview while useCase === 'claim', i.e. quotation-tier documents in a claim export. A and B are different report types over the same source with different server-side filtering, so differing latencies are entirely plausible; A starting first does not guarantee it finishes first.

This is structurally the same class of second-order effect as the deep-link one you already caught — a write path that re-populates the state the reset just cleared. The deep-link path was enumerated; this one wasn't. There is a pre-existing symmetric variant (rapid source A -> B in step 2), so one fix covers both: a monotonic request token.

const reportRequestRef = useRef(0);
// in handleUseCaseChange's guarded callback AND at the top of handleSourceChange:
const requestId = ++reportRequestRef.current;
// in the .then/.catch:
if (reportRequestRef.current !== requestId) return;

Non-blocking for this PR — it does not make anything worse, and it is a strict improvement over the status quo. But since #1943 was rated Must Have specifically because the artifact goes to a bank, I'd rather this land in the same cluster than sit in the backlog. Please file it and fix before promoting this cluster to main.

M2 — Medium: in-flight AI generation is not invalidated, and this one is not symmetric

runAiGeneration (L571–611) resolves into setAiContent(result). While generating, aiContent is still null, so guardedUpdate's dirty predicate is false and a use-case change applies immediately with no confirmation. The in-flight result then lands and is applied by applyAiContent to whatever report is loaded next.

The KEEP-list justification calls this a pre-existing symmetric gap. The mechanism is symmetric — a source change has the same hole. The consequence is not: the request carries type: useCase (L593) and, post-#1931, the prompt is purpose-focused. So a use-case change produces AI narrative written for the wrong report purpose — a claim-purpose executive summary on a budget-overview report — whereas a source change only produces text about the wrong source. Different kind of wrong, not just a different degree.

It also quietly defeats AC7's intent: the discard confirmation never runs, because the content it exists to protect doesn't exist yet at decision time. The step-5 stepper is reachable while isGeneratingAi is true (only the Enhance button is disabled, L882), so the window is real.

Same request-token fix applies. Non-blocking; file alongside M1.

M3 — Medium (architectural, explicitly not for this PR): the component has outgrown ad-hoc state resets

ReportWizardPage.tsx is 1,156 lines with 38 useState/useRef hooks, and the "what a transition invalidates" invariant is hand-maintained across two handlers that must stay in sync. #1943 is exactly the failure mode that shape produces, and the AC8 second-order effect is exactly the failure mode a boolean-ref patch on top produces. Both M1 and M2 are the same shape again.

Recommend a follow-up refactor to useReducer with explicit SELECT_USE_CASE / SELECT_SOURCE / REPORT_LOADED(requestId) actions. That makes the KEEP list and the staleness guard structural rather than a code comment, and it is the only way I see to stop re-litigating this handler every time a new piece of derived state is added. I'll file it; it should not gate this fix.

L1 — Low: AC5's enumeration is incomplete

Two pieces of handler-adjacent state are absent from the enumerated KEEP/CLEAR list:

• skippedDocuments — survives the change, and is only overwritten on a successful generatePdfFromContent. If the next generation fails, step 5 re-displays the previous report's skipped-document warnings (L928–937). Cosmetic, symmetric, low.
• aiError — survives; a stale error banner can outlive the report it referred to.

The claim-flow group (claimSuccess, claimedInvoiceCount, claimedDepositCount, finishedWithoutMarking, claimError) was enumerated as KEEP and I agree with keeping it. I checked the specific worry: budget-overview -> claim can carry claimSuccess forward, but so can source A -> source B within claim, and Step5Actions is only reachable with a freshly loaded report, so these are stale display values and never inputs to the exported PDF. Genuinely symmetric — leave it.

L2 — Low: AC4 as literally worded is stronger than what ships

attachDocuments and reportLanguageOverride are sticky, so X -> Y -> X with attachDocuments toggled off does not reproduce a clean-start export under X. This is correct behaviour (they are user preferences, not derived state) and the KEEP justification is right. But someone reading AC4's "always identical" during UAT will mark it failed. Worth one clarifying line on #1943 rather than a code change.

L3 — Low: deepLinkAppliedRef as useRef<string | null> instead of boolean

See question 2 above.

E2E scenarios 13/14 — inspection (never executed; CI is first run)

Both look correct to me. Specifically checked:

• Locators resolve on the right steps. step1NextButton ([class*="metadataCard"] [class*="btnPrimary"].first()) is unambiguous on step 1 — the use-case cards are useCaseCard, not btnPrimary. step2NextButton/step2BackButton live in the always-rendered buttonRow, which is outside Step2Source, so toBeDisabled() is stable even while the amounts skeleton is up.
• The amounts skeleton doesn't break the source assertions. Step2Source returns <Skeleton> while isLoading (Step2Source.tsx:37–38), so input[name="source"] is detached during the re-fetch. Playwright retries sourceRow() until it attaches, so both toBeChecked() and not.toBeChecked() wait correctly rather than failing fast.
• Scenario 13's terminal assertion is sound. seedAllocatedInvoice(..., status: 'pending'), and sourceReportService.test.ts:270 ("claim report excludes quotation and claimed statuses") confirms a claim report includes pending/paid — so regularInvoiceRow will be present after re-selecting under claim. Good choice: it proves the wizard recovers, not merely that it blocks.
• Scenario 14 will not flake on M1. A late-resolving stale fetch sets report but not sourceId, so not.toBeChecked() / toBeDisabled() still hold. The race is invisible to this test — which is why M1 needs its own guard rather than E2E coverage.
• Both are viewport-agnostic and will run across all three projects; nothing in them needs a test.skip.

One weakness, informational: scenario 14's core assertions are both negative (not.toBeChecked, toBeDisabled), and a negative Playwright assertion passes on its first successful poll. They would pass vacuously against a re-selection that is merely delayed rather than prevented. As written it's sound — the deep-link re-entry would fire in the same React commit as the use-case change, well before goNextFromStep1() — but if you want belt-and-braces, anchor on a positive settle signal (await the step-2 source list rendering) before the negative assertions.

Follow-ups I'll file

1. In-flight getSourceReport / generateReportContent staleness token (M1 + M2) — requested before this cluster's beta -> main promotion.
2. ReportWizardPage useReducer refactor (M3) — backlog.
3. AC5 enumeration addendum + AC4 wording clarification on BUG: changing the report use case keeps the previously fetched report — a claim report can embed quotation-tier documents #1943 (L1, L2) — comment on the issue.

[github-actions]

🎉 This PR is included in version 2.13.0-beta.45 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀