refactor(client): consolidate Badge/Modal reuse, add shared hooks, trap modal focus

[steilerDev]

Summary

Addresses the component-reuse violations flagged in #1816 (Medium aggregate, two High items — one already resolved by #1812's dead-code deletion):

• KeyboardShortcutsHelp → shared Modal: was a hand-rolled dialog with no portal, no Escape handling, and no focus management. Now renders through Modal, inheriting all of that for free.
• Modal focus trap: Modal gains Tab-cycling focus trap (the checklist's own "Focus management: Modals must trap focus" requirement). PhotoMetadataModal's private, near-identical trap implementation is removed in favor of the shared one — all 18 Modal consumers benefit.
• 4 Badge consolidations: BudgetHealthIndicator, UpcomingMilestonesCard, CriticalPathCard (an uncited sibling of UpcomingMilestonesCard found in the same file/CSS module during the migration), and SubsidyPipelineCard now use the shared Badge component + BadgeVariantMap instead of hand-rolled status spans with local class maps.
• 3 new shared hooks: useDebounce, useDebouncedCallback, useClickOutside — extracted from 8 previously-duplicated implementations (SearchPicker, DocumentBrowser, DiaryPage, BudgetOverviewPage, DiaryEntryEditPage, DataTableColumnSettings, OverflowMenu).
• Cleanup: removed the stale StatusBadge/ and HouseholdItemStatusBadge/ directories (components were already consolidated into Badge; only orphaned test files remained).
• i18n: wired an already-translated (en + de) key for WorkItemDetailPage's subtasks empty state instead of hardcoded English.

Fixes #1816

Deferred to follow-up issues (scope discipline, documented in the dev-team-lead spec)

• The 25+ ad-hoc FormError/EmptyState bypass sites cited in Component-reuse violations: SearchPicker/Modal forks, Badge reimplemented 3x, FormError/EmptyState bypassed, missing shared hooks #1816 — confirmed mechanical, but sized for their own PR given this PR already touches the shared Modal (18 consumers) and adds 3 new hooks across 8 files.
• DataTableHeader.tsx's click-outside duplication — its exclusion check matches a popover rendered by a sibling component that doesn't currently forward a ref; converting it cleanly needs a forwardRef change to that sibling, which is scope beyond a hook swap.
• DocumentSkeleton.tsx and the plain-text loading divs — the issue itself marks these as judgment calls requiring visual redesign.
• Pre-existing bug found during QA: DiaryEntryEditPage fires one unwanted immediate autosave on mounting a draft entry (skipAutoSaveOnMountRef is consumed on a no-op render before entry finishes loading, so the guard is spent by the time the form fields actually populate). Confirmed via diff against origin/beta that this code path is byte-identical and predates this PR — unrelated to the debounce-hook migration. Worked around in the new regression test (flush + mockClear()) rather than masked; recommend a dedicated follow-up issue.

Test plan

• Unit tests: 3 new hook test files (27 tests, 100% coverage each) + updates across 13 existing test files
• npx tsc --noEmit clean (verified independently in review)
• npm run stylelint clean (verified independently in review)
• npx eslint clean on all changed files — no new warnings (verified independently in review; 2 pre-existing unrelated warnings elsewhere in the codebase, untouched by this PR)
• npx prettier --check clean on all changed files (verified independently in review)
• Directly ran the 18 most relevant test files (not just the self-report) — 466/466 passing
• CI Quality Gates (pending)

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 qa-integration-tester (Sonnet 4.5) noreply@anthropic.com

[steilerDev]

[security-engineer] Security review of PR #1848 (fix/1816-component-reuse)

Verdict: APPROVED

Reviewed git diff origin/beta..HEAD — pure client-side refactor (Modal focus trap, Badge/Modal consolidation, 3 new shared hooks, stale directory cleanup). No server changes, no new dependencies, no dangerouslySetInnerHTML/innerHTML/eval introduced anywhere in the diff.

Modal focus trap (client/src/components/Modal/Modal.tsx)

• The keydown listener is document-level but the handler's first line is if (e.key !== 'Tab' || !contentRef.current) return; — it is a pure no-op for every other key and never calls preventDefault() on non-Tab keys, so it cannot interfere with other inputs, shortcuts, or the existing Escape-key handler.
• preventDefault() + refocus only fires when document.activeElement is exactly the first or last focusable element within contentRef — mid-focus Tab presses pass through untouched (confirmed by Modal.test.tsx's new boundary tests).
• Listener is registered once (useEffect(..., [])) and unregistered on unmount via the returned cleanup — no leak.
• Minor (informational, non-blocking): each Modal instance attaches its own document-level Tab listener, so if two modals are ever stacked simultaneously both handlers fire on every Tab press. Not exploitable and no current call site renders nested modals, but worth a mental note if a future feature stacks modals.

useClickOutside (client/src/hooks/useClickOutside.ts)

• mousedown listener is added/removed inside the same useEffect closure (document.addEventListener / matching removeEventListener on cleanup, keyed off [enabled]), so no listener leak across re-renders, toggling enabled, or unmount.
• Callback and targets are read through refs (handlerRef, targetsRef) updated every render rather than captured in the effect's dependency array — avoids stale-closure bugs without needing to re-bind the listener on every render.
• Consuming components (OverflowMenu, SearchPicker, DataTableColumnSettings) correctly gate the listener with enabled = isOpen, matching the removed bespoke implementations' behavior.

useDebounce / useDebouncedCallback: standard timeout-based debounce, clearTimeout on cancel/unmount, no injection surface (no eval/Function, no dynamic property access from user input).

No SQL/command/XSS injection vectors, no auth/authz surface touched (client-only), no sensitive data handled differently, no new dependencies to check for CVEs. Test coverage for the new hooks and the focus-trap boundary conditions is thorough and confirms the implementation matches the described behavior.

[steilerDev]

[product-architect] Architecture / contract review of the component-reuse refactor.

Verdict: APPROVED — no contract impact, faithful consolidations, and the regression the first commit introduced was correctly root-caused and fixed with a mutation-tested guard. Findings below are medium/low for follow-up, none blocking.

Contract impact

None. The diff touches only client/ — no shared/ types, no server/ routes, no API/schema surface. Nothing for Backend/Frontend to rebuild against.

Hook API design

• useDebounce / useClickOutside — contracts are sound. useClickOutside's latest-ref pattern for both targets and handler (re-subscribing only on enabled change) is the right call for consumers that recreate the targets array each render. Good JSDoc.
• useDebouncedCallback — the useMemo(() => ({ trigger, cancel }), [trigger, cancel]) fix is correct and gives the return object the referential stability consumers need for effect dep arrays. Solid RCA in the fix commit.

Findings

MEDIUM — Document the referential-stability guarantee on useDebouncedCallback (issue #1816 follow-up).
The useMemo enforces stability, but nothing states that stability is an intentional part of the hook's contract that consumers depend on. This is exactly the invariant whose absence just cost an E2E cycle; a future refactor (e.g. "why is this wrapped in useMemo, the members are already stable") could silently reintroduce the ping-pong. Add a one-line JSDoc to the hook: the returned { trigger, cancel } is referentially stable across renders (stable unless delayMs changes) and is safe to place in a dependency array. useClickOutside has JSDoc; useDebouncedCallback and useDebounce have none.

MEDIUM — This incident is a generalizable convention worth codifying (I own this; tracking as a follow-up, not blocking this PR).
The root cause — a custom hook returning a fresh object/array literal each render — is a repo-wide trap, not a one-off. I'll add a line to CLAUDE.md Coding Standards (and mirror it on the Architecture wiki): "Custom hooks returning objects or arrays must memoize the returned value so it is referentially stable in consumer dependency arrays." Flagging here for traceability; no change required on this branch.

LOW / informational — Modal focus-trap a11y.
The shared trap is a faithful port of PhotoMetadataModal's removed private trap (verified: same Tab/Shift+Tab cycling, no behavior lost) and now correctly benefits all 18 consumers. Two standard WAI-ARIA APG dialog refinements remain, both pre-existing (never present in Modal or the old private trap, so not regressions — noting for a future a11y pass):

• No focus restoration to the invoking element on close.
• getFocusableElements filters disabled but not hidden / aria-hidden / zero-size elements, so Tab could theoretically land on an off-screen focusable.

LOW / informational — useClickOutside listens on mousedown only. pointerdown (or adding touchstart) would be marginally more robust on touch devices; synthesized mouse events make the current impl work in practice. Future enhancement.

Nice work on the RCA + regression/mutation tests for the debounce fix — that is the right depth of response to an E2E-caught bug.

[steilerDev]

[ux-designer] Design review of PR #1848 (fix/1816-component-reuse), scoped to git diff origin/beta..HEAD under client/.

1. Modal focus trap — PASS

getFocusableElements() (client/src/components/Modal/Modal.tsx:15-21) is queried fresh on every Tab keypress against contentRef.current, which wraps header + body + footer, so all of a modal's interactive elements (including footer action buttons) are included — verified this correctly subsumes what PhotoMetadataModal's deleted private trap used to query for ('[role="dialog"] button, ...').

• Zero focusable elements: guarded by if (focusable.length === 0) return; — no-op, no throw.
• Single focusable element: first === last, so both Tab and Shift+Tab re-focus the same element — correct self-loop, and Modal.test.tsx's new "single focusable" test confirms it (a modal's own close button always guarantees at least one focusable element in practice, so true zero is more theoretical than reachable here — fine).
• Tab/Shift+Tab wrap at both boundaries, non-Tab keys are ignored (e.key !== 'Tab' early return, confirmed doesn't fire onClose or move focus), and mid-list Tab presses are left to native browser behavior — all covered by the 6 new Modal.test.tsx cases.
• KeyboardShortcutsHelp now correctly inherits portal/Escape/focus-trap/dialog semantics it previously hand-rolled without. Verified via the rewritten test file: role="dialog" + aria-modal="true" present, Escape closes, close-button aria-label now resolves through the shared common:aria.closeDialog key.

Minor/informational, already flagged by security-engineer and not reintroduced by this PR: each Modal instance's Tab listener is document-scoped, so two simultaneously stacked modals would both react to Tab. No current call site stacks modals — not a regression, just a note for future modal-inside-modal features.

2. Badge consolidation — verify token fidelity — 2 findings, both non-blocking

Dark mode: confirmed every new variant class (budgetHealth*, schedule*, subsidy*) routes through Layer 2 semantic tokens with existing [data-theme='dark'] overrides (--color-success-badge-bg/-text, --color-danger-bg(-strong), --color-danger-text-on-light, --color-status-not-started-bg/-text, --color-status-in-progress-bg/-text, --color-warning-bg) — no new dark-mode gaps.

Naming fits the existing variant scheme (compare .not_started/.in_progress WorkItemStatus group, .planned/.purchased HouseholdItemStatus group already in the file) — grouping comments and prefix convention (budgetHealth*/schedule*/subsidy*) are consistent and readable.

Finding (Medium, non-blocking): the "ported verbatim / zero visual delta" claim doesn't hold for all three source components. The shared .badge base (padding: var(--spacing-1) var(--spacing-2-5); font-size: var(--font-size-xs); font-weight: var(--font-weight-medium)) was unified from StatusBadge/HouseholdItemStatusBadge/DiaryOutcomeBadge/DiarySeverityBadge — but the three newly-migrated components each had their own, different base sizing that this consolidation silently overrides:

Component	Old padding (v/h)	New padding (v/h)	Old font-size	New font-size	Old weight	New weight
SubsidyPipelineCard	2px / 8px	4px / 10px	12px (xs)	12px (xs)	500 (medium)	500 (medium)
CriticalPathCard/UpcomingMilestonesCard	4px / 8px	4px / 10px	12px (xs)	12px (xs)	500 (medium)	500 (medium)
BudgetHealthIndicator	4px / 12px	4px / 10px	14px (sm)	12px (xs)	600 (semibold)	500 (medium)

The first two are a harmless ±2px padding nudge (both already used spacing tokens, just different ones — a reasonable side-effect of standardizing on one base). BudgetHealthIndicator is a real, visible downgrade: its badge shrinks from 14px semibold to 12px medium and loses 2px of horizontal breathing room. Colors/contrast are unchanged (no WCAG issue), but this is an important status indicator (On Budget / At Risk / Over Budget) and the size/weight drop wasn't called out in the PR description as an intentional design change. Recommend either: (a) accept the downsize explicitly as the new standard badge size across the app, or (b) give BudgetHealthIndicator a size modifier (e.g. a .badgeLg or similar composable modifier class alongside .badge) to preserve its original prominence. Either is fine — just needs to be a decision, not a side effect of the refactor. Non-blocking; fine to fix in refinement.

KeyboardShortcutsHelp td padding (0.875rem → var(--spacing-4), 14px→16px): acceptable. tokens.css has no 14px spacing token (only --spacing-3=12px / --spacing-4=16px), so snapping up to the nearest token is the correct call under a token-only system — 2px is imperceptible on table row padding. th/kbd/mobile-override paddings are all now token-backed with no other deltas. No dead CSS left behind (.modal/.modalBackdrop/.modalContent/.modalHeader/.modalTitle/.closeButton chrome correctly fully removed now that Modal owns it; .keyColumn/.descriptionColumn/.keyCell/.descriptionCell all still referenced in the component).

3. CriticalPathCard div→span — PASS

Old markup was <div className={\${styles.badge} ${healthClass}`}>withstyles.badge { display: inline-flex; ... }— a

with its default block display already overridden toinline-flex. New renders awith the samedisplay: inline-flexin the shared base class. Both resolve to the same computeddisplay: inline-flex`, so the claim of no layout shift holds — this is a tag-semantics cleanup (span is the more correct element for inline status text) with no visual side effect.

4. BudgetHealthIndicator role="status" wrapper — PASS

<span role="status"><Badge .../></span> (client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.tsx:45-47) keeps the live region on an ancestor of the badge text, so aria-live="polite" (implicit from role="status") still fires when the nested Badge's text content changes on a status transition. role="status" is applied exactly once (not duplicated onto the inner Badge span) — correct, no redundant aria-live per the recurring-bug note in my own review history. BudgetHealthIndicator.test.tsx was updated to assert the class on status.querySelector('span') rather than on the status element itself, matching the new two-span structure.

Informational (not blocking)

• common.json's keyboardShortcuts.closeAriaLabel (en: "Close", de: "Schließen") is now orphaned — KeyboardShortcutsHelp no longer needs its own aria-label since Modal supplies common:aria.closeDialog. Not part of this PR's stated cleanup scope; flagging for a follow-up i18n-orphan sweep rather than blocking here.

Verdict: APPROVED (non-blocking comment)

Modal focus trap (item 1), CriticalPathCard tag change (item 3), and the role="status" wrapper (item 4) all check out exactly as described. Item 2's dark-mode/naming/token-usage checks all pass; the one real finding — BudgetHealthIndicator's font-size/weight/padding shrink — is a Medium, non-blocking visual-consistency item per the design system's severity policy (no accessibility or dark-mode regression), safe to address in refinement or as a quick follow-up commit before merge, reviewer's choice.

[github-actions]

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

The release is available on GitHub release

Your semantic-release bot 📦🚀