Skip to content

feat(reports): wizard settings step with report language selection - #1903

Merged
steilerDev merged 3 commits into
betafrom
feat/1899-settings-step-report-language
Jul 31, 2026
Merged

feat(reports): wizard settings step with report language selection#1903
steilerDev merged 3 commits into
betafrom
feat/1899-settings-step-report-language

Conversation

@steilerDev

Copy link
Copy Markdown
Owner

Summary

  • Adds a new Settings step to the Report Wizard, expanding it to 5 steps: users can now choose the report language (English/German) independent of the app UI language, alongside the relocated report toggles.
  • The exported PDF renders fully in the selected report language, using getFixedT for translated strings and createFormatters for locale-aware date/currency/percent formatting — the app's own UI language is never affected.
  • Fixes a tablet-viewport density issue in the WizardStepper component.

Fixes #1899

Test plan

  • Unit tests pass (95%+ coverage) — Step4Settings, Step5Actions, ReportWizardPage, formatters, realRender
  • Integration tests pass
  • CI Quality Gates pass (typecheck, tests, build, audit)

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com
Co-Authored-By: Claude frontend-developer (Haiku 4.5) noreply@anthropic.com
Co-Authored-By: Claude translator (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) noreply@anthropic.com

5-step wizard adds a new Settings step (report language en/de +
relocated toggles); the exported PDF now renders fully in the
selected language via getFixedT + createFormatters, while the app
UI language stays untouched. Also fixes tablet stepper density.

Fixes #1899

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com>
Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com>
Co-Authored-By: Claude translator (Sonnet 4.5) <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

[ux-designer] Design review against my visual spec on #1899 (issuecomment-5141421842).

Verdict: APPROVED (posted as a comment — self-review approval is blocked by GitHub for this account: "Can not approve your own pull request")

Checked against every section of the spec:

  • Structure: Step4Settings.tsx — single .settingsCard (renamed verbatim from .optionsCard, same tokens), language .settingsSection (h3 .sectionTitle + role="group" + aria-labelledby="report-language-heading" + helper) followed by the two toggles — matches spec.
  • Language labels: literal untranslated "English" / "Deutsch" strings, not t()-wrapped — correct, matches the ProfilePage.tsx autonym precedent I called out.
  • Focus indicator: no custom ring added, no outline: none anywhere in the diff — native :focus-visible preserved on the radios, consistent with the sibling .optionCheckbox controls as specified.
  • Toggles: attachDocuments/includeCoverLetter moved byte-identical (same className usage, htmlFor/id, disabled+title hint logic) — confirmed via diff.
  • ReportWizardPage.module.css: every new rule (.settingsSection, .settingsDivider, .sectionTitle, .languageGroup) uses only existing tokens (--color-border, --color-text-primary, --spacing-*, --font-size-*, --font-weight-semibold) — no hardcoded values, no new colors, so dark mode requires no special-casing.
  • WizardStepper tablet fix: @media (min-width: 768px) and (max-width: 1023px) block added with the exact values I specified (.stepList gap → --spacing-2, .connector width → 20px, .label font-size → --font-size-xs), correctly placed before the existing Mobile Breakpoint block.
  • Step 5 (old step 4): Step5Actions.tsx keeps .step4Body/.step4Layout/.step4Column unchanged, just with the two toggle props removed — no redesign, as required.
  • i18n: stepper.settings, settingsStep.languageHeading, settingsStep.languageHelper added in both en and de exactly as specced.

One informational nit, non-blocking: .settingsDivider in the CSS does double duty — it carries the border-top/padding-top divider treatment and the display:flex; flex-direction:column; gap: var(--spacing-3) section-container role, applied directly to the wrapper of the two toggle rows (rather than a separate empty divider element + a .settingsSection wrapper as sketched in my spec's ASCII diagram). Visual output is identical to spec intent (border-top above the toggles, spacing-3 gap between rows) — just flagging the naming so a future reader isn't confused by a class called "Divider" acting as a flex container. No change requested.

No accessibility or dark-mode issues found.

… displayed author

PR #1903 showed a human GitHub username as author but gh pr review --approve
still failed with "Can not approve your own pull request" — record that the
check isn't reliably predictable from author.login.

Co-Authored-By: Claude ux-designer (Sonnet 4.6) <noreply@anthropic.com>
@steilerDev

Copy link
Copy Markdown
Owner Author

[product-architect]

Architecture, code-quality and test-coverage review of the 5-step wizard restructure + report-language plumbing.

Verdict: APPROVE

The core architectural claim of this PR holds up under inspection, and the test suite is genuinely strong. Two Medium findings and a few Low/informational items below — none of them break a contract or introduce architectural debt that must be paid before merge, so they're noted for refinement rather than blocking.


What I verified as sound

1. The getFixedT / createFormatters architecture — correct, and "no reportPdf changes needed" is genuinely true rather than a shortcut.

  • Both locale bundles are statically imported in client/src/i18n/index.ts (enBudget / deBudget into a single resources object), so i18n.getFixedT('de', 'budget') resolves synchronously against an already-loaded bundle. No lazy-load race, no changeLanguage() needed, no ambient mutation. This is the load-bearing precondition for the whole design and it is satisfied.
  • I grepped client/src/lib/reportPdf/*.ts (excluding tests) for toLocale*, Intl., i18n, resolvedLocale, and hardcoded locale tags. The only hits are import type { TFunction } and new Date().toISOString() — locale-independent by construction. The builders take t: TFunction + formatters?: Formatters as parameters (merge.ts:19-27) and read nothing ambient. The claim that their existing params absorb the change is accurate.
  • No namespace-prefixed keys (t('common:…')) exist anywhere in reportPdf/, so a single getFixedT(lang, 'budget') covers the full key surface. Had there been one, the fixed-ns translator would still have resolved it, but it's good that the question doesn't arise.
  • reportT and reportFormatters are both useMemo'd on their real inputs, so regeneratePdf's identity stays stable and the existing "don't let derived memos re-fire the effect" invariant (documented at ReportWizardPage.tsx:209-210 / 276-279) is preserved rather than quietly broken.

2. The useFormatters refactor is behavior-identical for every other consumer app-wide. The diff is a pure extract-function: same resolvedLocale === 'de' ? 'de-DE' : 'en-US' mapping, same 11 keys, same delegation arguments per closure, and — importantly — the same object identity semantics. useFormatters was never memoized; it built a fresh object literal on every render before, and it returns a fresh createFormatters(...) object on every render now. No consumer's referential-equality behavior changes. formatters.test.ts's parity block pins each of the 11 closures to its raw counterpart, which is the right way to prevent future divergence between the two call paths.

3. Step renumbering is complete. maxReachedStep correctly switched to monotonic Math.max(s, 4) / Math.max(s, 5) at the two forward transitions; handleUseCaseChange/handleSourceChange keep their deliberate absolute resets. stepHeadingsRef indices 0–4 are all wired to their respective <h2 tabIndex={-1}>. Forward-lock is proven by the new E2E assertion that isStepInteractive(5) === false from step 4 and that stepItems.nth(4) renders no <button> at all. The claim modal remains gated on showClaimConfirm && report, reachable only from Step5Actions — unaffected by the renumber.

4. Component split is clean. Step4Options.tsx is gone (git detected the rename to Step5Actions.tsx); zero code references remain — the only Step4Options hits in the tree are historical provenance comments in test docstrings, which are useful, not dangling. .optionsCard was renamed to .settingsCard with no orphaned selector left behind, index.ts needed no change, and there's no duplicate .sectionTitle in the module. The global reset (styles/index.css) zeroes all margins, so dropping the spec's explicit margin: 0 0 … on .sectionTitle in favour of the parent's gap is equivalent, not a regression.

5. Test quality is high, and the realRender.test.ts singleton test is exactly the right call. Every other test in that file runs against an isolated i18next.createInstance(), which means the production path — ReportWizardPage calling i18n.getFixedT on the app-wide singleton — was genuinely untested. The new block closes that gap and, correctly, asserts the non-mutation of i18n.language on both sides of the getFixedT call, not just the German resolution. That's the actual invariant the story depends on.

Also verified: keys added to both en and de bundles; German helper copy reads naturally; new CSS is fully tokenized; the tablet media query matches the UX spec verbatim; no any or @ts- escapes in the new code.


Findings

M1 (Medium) — reportLanguage never re-syncs when resolvedLocale resolves after mount

ReportWizardPage.tsx:56 seeds the picker with useState<ResolvedLocale>(resolvedLocale), which captures the value at first render only. This is not a hypothetical race:

  • LocaleContext.syncWithServer calls setResolvedLocale(...) asynchronously after listPreferences() returns, fired from App.tsx's LocaleServerSync once auth resolves.
  • That same function removes the locale localStorage key whenever the server has a preference (LocaleContext.tsx:128). So on every subsequent load, resolvedLocale starts from detectBrowserLocale() and only later flips to the server value.

Concretely: a user whose app locale is German but whose browser is en-US gets resolvedLocale === 'en' at mount, flipping to 'de' shortly after. If ReportWizardPage mounts inside that window — a deep link or a refresh on /budget/reports, which is the wizard's normal entry point — the picker defaults to English against a German UI, contradicting AC 2 ("defaults to my current UI locale"). The user can see and correct it, which is why this is Medium rather than High, but it's a real defaulting bug on the story's own primary acceptance criterion.

The tests can't catch it because LocaleProvider's async paths never resolve under jsdom — correctly documented in the test header, but that documentation is also the tell that this window is untested.

Suggested fix (seed-until-touched):

const languageTouchedRef = useRef(false);
useEffect(() => {
  if (!languageTouchedRef.current) setReportLanguage(resolvedLocale);
}, [resolvedLocale]);

with onReportLanguageChange in the Step4Settings call site setting languageTouchedRef.current = true before setReportLanguage. A regression test can drive it by rendering, then flipping the context value.

M2 (Medium) — the BCP-47 locale mapping is now duplicated

resolvedLocale === 'de' ? 'de-DE' : 'en-US' now exists in two places: formatters.ts:398 (useFormatters) and ReportWizardPage.tsx:202. This is precisely the pair that silently diverges when a third locale is added — the new locale would format correctly in the UI and fall back to en-US in every exported report, with nothing failing loudly. Please extract one exported helper, e.g. resolvedLocaleToIntlTag(locale: ResolvedLocale): string, colocated with the ResolvedLocale type, and call it from both. Cheap now, annoying later.

L1 (Low) — extract createFormatters's return type

The 11-property return type is written inline at formatters.ts:301-317. Now that this is a public factory with two independent call sites, promote it to an exported LocaleFormatters interface alongside the existing Formatters so the contract is referenceable.

L2 (Low) — step-5 CSS class names still say "step4"

.step4Body / .step4Layout / .step4Column (ReportWizardPage.module.css:154-166, Step5Actions.tsx:47) and the step-5 label key sourceReports.stepper.options are all now off-by-one against the step they name. Both were deliberate minimal-diff choices (and the i18n key reuse avoids a pointless translation churn), but a rename pass while the context is fresh will save someone a double-take later.

L3 (Informational) — AC 3's filename bullet is vacuously satisfied; say so

handleDownload / handleUploadPaperless build ${useCase}-${slug}-${today} from a raw enum value, a source-name slug, and an ISO date — none locale-dependent, so nothing changes with report language. That's the right engineering answer (stable, sortable, collision-resistant filenames), but the AC explicitly asks for language-following filenames. Please state the reasoning in the PR body so product-owner signs off on it deliberately rather than assuming it was overlooked.

L4 (Informational) — currency is absent from the regenerate effect's dep list

The eslint-disabled dep array at ReportWizardPage.tsx:305-314 now includes reportLanguage but not currency. If GET /api/config returns a non-EUR currency and resolves after the first generation, the PDF keeps EUR formatting until some other option changes. This is pre-existing (useFormatters was equally absent before) and vanishingly unlikely given how many clicks precede the first generation — but currency is now an explicit useMemo dep of reportFormatters, so adding it is a one-token fix.

L5 (Informational, my own follow-up) — ADR-034 needs a note

ADR-034 (client-side report PDF generation) doesn't record that report language is decoupled from the ambient UI locale via getFixedT + createFormatters. I'll add that to the wiki separately; not a blocker for this PR.


CI

Quality Gates, Static Analysis, Trailer Check, Docker, Coverage Report and E2E smoke all pass — the beta merge gate is green.

E2E Tests (Shard 5/16) failed. The run was still in progress when I looked, so per-test logs weren't retrievable and the annotation is a bare exit-code-1. This does not block the beta merge under the project's gating policy, but it must be triaged before the betamain promotion, where E2E Gates is required. Note that a recurring shard-5 flake in this same wizard area is already on record from the previous Bank Report Wizard round — please confirm whether this is that known flake or the newly added Scenario 12 before promotion, rather than assuming the former.


Verdict: APPROVE (posted as a comment — GitHub rejects --approve because the PR is authored by the same account.)

@steilerDev steilerDev left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[product-owner] Requirements review of PR #1903 against Story #1899 acceptance criteria.

Verdict: APPROVED WITH ONE MUST FIX

Posted as a comment rather than --approve because GitHub blocks self-approval on this account ("Can not approve your own pull request") — same constraint the ux-designer hit above. Treat this as a product-owner approval, conditional on the single MUST FIX below. The gap is a default-value seeding issue, not a functional break, so per the verdict matrix it is flagged --comment / MUST FIX rather than --request-changes — it does not need to re-enter the review loop, but it must be fixed before merge.


AC Coverage

1. Wizard gains a fifth step — PASS (4/4)

AC Verdict Evidence
1.1 Five steps in order PASS steps = use-case, source, invoices, settings, actions → Report Type → Budget Source → Select Invoices → Settings → Preview & Export. ReportWizardPage.test.tsx asserts exactly 5 listitems in the stepper nav; E2E Scenario 8 asserts stepItems count 5; Scenario 12 asserts nth(3)="Settings", nth(4)="Preview & Export".
1.2 Nav/gating/focus consistent across 5 PASS Step 4 Back→3, Next→setMaxReachedStep(max, 5) + step 5; step 5 Back→4. stepHeadingsRef indices 0–4 all assigned, focus effect reads currentStep - 1 — consistent. E2E Scenario 9 extended to prove step 5 is a non-interactive element (no <button>) while maxReachedStep is 4.
1.3 Mobile "step X of Y" reports 5 PASS Unit tests assert "Step 1 of 5", "Step 4 of 5", "Step 5 of 5"; E2E Scenario 10 updated to 5 dots + toContainText('5').
1.4 Deep link no regression PASS E2E Scenario 7 (?sourceId=) walks through the new Settings step to a ready preview + enabled Download, so the #1879 fix is still exercised end-to-end.

2. Settings step contents — PASS (6/7), 1 MUST FIX

AC Verdict Evidence
2.1 Language picker offering en/de PASS Step4Settings.tsx radio group, English/Deutsch. Autonyms deliberately untranslated per the ux-designer spec and the ProfilePage precedent — reviewed, not a missed-translation bug.
2.2 Defaults to current UI locale MUST FIX See finding below.
2.3 Two toggles moved, defaults/behaviour preserved PASS Moved verbatim — optionRow/optionCheckbox/optionLabel/optionHelper, htmlFor/id pairing, disabled={coverLetterDisabled} and the title disabled-reason hint on both input and label all survive the move.
2.4 Toggles no longer on Step 5 PASS Step5Actions.tsx drops 53 lines; unit test asserts step 5 has no radio "English" and no "Include cover letter" label, and step 4 has no Download/Upload/Mark-claimed/iframe. Exactly one place each.
2.5 Selection retained across back/forward PASS State lifted to ReportWizardPage (same pattern as attachDocuments). Unit test picks Deutsch → Back to step 3 → Next → still checked.
2.6 App UI locale is NOT changed PASS Implemented via i18n.getFixedT(reportLanguage, 'budget')changeLanguage() is never called. realRender.test.ts asserts the production singleton's i18n.language stays 'en' before and after getFixedT('de', …). E2E Scenario 12 asserts the heading/stepper/buttons stay English after selecting Deutsch.
2.7 Keyboard + screen reader PASS role="group" + aria-labelledby="report-language-heading"; implicit <label> wrapping gives each radio an accessible name (tests select by getByRole('radio', { name: 'English' })); native radio arrow-key traversal; native :focus-visible retained with no outline: none. Matches the ux-designer spec §2/§8.

3. Report renders in the selected language — PASS (8/8)

AC Verdict Evidence
3.1 All PDF text in selected language PASS reportT threaded as the 6th arg to generateReportPdf. Confirmed overviewPdf.ts / coverLetterPdf.ts / merge.ts import TFunction type-only from i18next and never touch the singleton — every string is injection-resolved. realRender.test.ts's it.each([en, de]) covers the full unmocked pipeline including the #1898 Usage column, attachment notes (incl. _one/_other plurals) and both deposit-footnote wordings.
3.2 Cover letter in selected language PASS Same t flows merge.tsbuildCoverLetterContent; covered by the same en/de it.each render.
3.3 Dates via shared formatters PASS createFormatters('de-DE' | 'en-US', currency) — no raw Intl/toLocaleDateString() introduced. useFormatters() refactored to delegate to the same factory, so hook and PDF paths cannot drift.
3.4 Currency locale-formatted, code unchanged PASS currency comes from useLocale() (server config) and is independent of reportLanguage; only the locale string varies. Test exercises the real closures: formatCurrency(1234.56) contains 1,234.56 and getCurrencySymbol() is under en-US.
3.5 No English leakage / no raw keys PASS The en/de parity renders assert real German copy (Auftragnehmer, PDF herunterladen, 2 Anhänge: Angebot, Rechnung) — a raw key or an English fallback would fail these.
3.6 Filename convention follows the language PASS (as written) — see note Filename is ${useCase}-${slug}-${today}.pdf; the Paperless title is the same string without the extension. useCase is the raw enum (claim/budget-overview/proof-of-funds), slug is user data, today is ISO-8601. There are no translated components and no locale-dependent date formatting, so the conditional clause ("any translated components…") is vacuously satisfied and nothing needed to change. I am accepting this as-is deliberately, not by omission — ISO dates in filenames are sortable and filesystem-safe, and localising them to 31.07.2026 would be a usability regression. If we ever translate the use-case token into the filename, this AC becomes live again.
3.7 Wizard chrome stays in UI locale PASS Step4Settings receives the page's UI t; E2E Scenario 12 asserts heading, both stepper labels and Back/Next remain English after switching the report to Deutsch.
3.8 Explicit resolution against the selected bundle PASS realRender.test.ts's new production-singleton test is exactly the right closer: it proves the app-wide i18n (not an isolated test instance) has both bundles loaded and that getFixedT('de') resolves real German while ambient stays 'en'. This closes the gap every prior test in that file left open.

4. Step 5 unchanged — PASS (1/1)

Step5Actions.tsx is Step4Options.tsx minus the two toggle rows (70% rename similarity); preview, download, Paperless upload, mark-claimed and the step4Layout two-column grid all carry over untouched. E2E Scenarios 1, 2, 5, 6, 7 and the expansion spec all still reach and exercise the same actions.

Translations — PASS

Three new keys, both locales, full parity:

Key en de
sourceReports.stepper.settings Settings Einstellungen
sourceReports.settingsStep.languageHeading Report language Berichtssprache
sourceReports.settingsStep.languageHelper Only affects the exported report — your app language stays the same. Betrifft nur den exportierten Bericht – die Sprache der App bleibt unverändert.

Helper copy in both locales reinforces AC 2.6 directly, as specified. Every new t() path resolves in en/budget.json and de/budget.json.

Scope — clean

The WizardStepper.module.css tablet-range rule (768–1023px) is in scope: it is a direct consequence of adding a 5th label and was called out in the ux-designer spec §4 as a required fix. Agent-memory updates ride along per project convention. No undocumented behaviour changes. Out-of-scope items (#1900 preview editing, #1901 AI generation, new locales, cross-run persistence) are all correctly absent.


MUST FIX before merge — AC 2.2

reportLanguage is seeded with a one-shot useState initializer:

const [reportLanguage, setReportLanguage] = useState<ResolvedLocale>(resolvedLocale);

That initializer runs only on ReportWizardPage's first render, but resolvedLocale is not stable at that moment. LocaleContext.syncWithServer — fired from App.tsx's LocaleServerSync once user resolves — applies the user's server-stored locale preference asynchronously, and clears localStorage when it does. So on every later hard load readStoredPreference() returns 'system' and resolvedLocale starts at the browser-detected locale before flipping to the stored preference.

Consequence: a user whose stored preference is de but whose browser reports a non-German locale — precisely the "my bank corresponds in a different language than the one I use the app in" persona this story is written for — who lands on /budget/reports via hard load or a ?sourceId= deep link gets the picker pre-selected to English while the UI is German, and it never self-corrects, because the value is captured at mount rather than when Step 4 renders. The AC is explicitly worded "when the language picker renders, then it defaults to my current UI locale."

Suggested fix, which preserves AC 2.5 (retention for the wizard run) and AC 2.6 (picker never drives the app locale):

const [reportLanguageOverride, setReportLanguageOverride] = useState<ResolvedLocale | null>(null);
const reportLanguage = reportLanguageOverride ?? resolvedLocale;

with setReportLanguageOverride wired to the picker's onChange. Until the user makes an explicit choice the default tracks the UI locale; once they choose, the choice sticks for the run. Worth a unit test that a late resolvedLocale change updates the default but does not clobber an explicit selection.

Note — not blocking

CI: Quality Gates green. E2E Tests (Shard 5/16) is red, but the two failures are tests/invoices/invoices.spec.ts ("Effective Amount" column, Issue #1876) and tests/navigation/dashboard.spec.ts (card re-enable) — both unrelated to the report wizard and consistent with the known pre-existing shard-5 flake already tracked for triage before the next betamain promotion. Not attributable to this PR and not a beta merge blocker.

Once the AC 2.2 seeding fix lands, this story is Done from a requirements standpoint.

Seeding useState(resolvedLocale) captured the first-render value only;
LocaleContext.syncWithServer applies the stored locale asynchronously,
so a hard load could pre-select the wrong report language. The default
now derives from resolvedLocale until the user explicitly overrides it.

Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com>
@steilerDev
steilerDev merged commit db49676 into beta Jul 31, 2026
31 of 32 checks passed
@steilerDev
steilerDev deleted the feat/1899-settings-step-report-language branch July 31, 2026 10:41
@github-actions

Copy link
Copy Markdown
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant