chore(invoice,budget): dedupe split helper, extract OverflowMenu, surface revert errors (#1413)

[steilerDev]

Summary

• Split helper extraction: splitByDeposits() extracted into depositAggregateUtils.ts and reused across 4 previously-duplicated call sites in budgetSourceService.ts — behaviour-preserving refactor.
• OverflowMenu shared component: New client/src/components/OverflowMenu/ replaces ~330 lines of duplicated menu code in DepositRow / DepositCard. Full WAI-ARIA Menu Button keyboard nav, mobile 44px touch targets, design-token-only CSS, dark mode via token cascade.
• Inline style removal: <tr> opacity transition moved from style={{}} to .tableRowMutating CSS module class (matches fd73bca pattern).
• Revert error surfacing: API errors in handleRevertToPending, handleRevertToPaid, and handleStateConfirm now surface via FormError banners (section-level auto-dismiss 6s; modal-level for state-confirm). Two new i18n keys for network-error fallbacks; coded server errors use existing translateApiError().

Fixes #1413

Test plan

• Unit tests pass (95%+ coverage) — 9 splitByDeposits tests, 37 OverflowMenu tests, 5 error-surfacing tests
• Integration tests pass
• E2E Scenario 8: revert error banner visible and auto-dismisses
• Pre-commit hook quality gates pass

Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com

[steilerDev]

[product-architect]

APPROVED

Reviewed against the four recommendations I raised in #1407 and #1412. All addressed cleanly.

What I verified

1. splitByDeposits() extraction — Matches my recommendation exactly. The helper is a clean factoring of the group-by-invoice + dedup-by-deposit_id + residual/fraction math that was duplicated four times. All four call sites (computeDepositAwareAggregates, computeStatusContribution, aggregateInvoiceStatusBreakdown, computeDiscretionaryInvoiceAmount) now route through it, and the pre-existing aggregate test suites in depositAggregateUtils.test.ts were not modified — strong evidence the refactor is behaviour-preserving. The new InvoiceDepositSplit interface is well-documented and the 9 new scenarios cover empty input, no-deposit, partial coverage, overpayment clamp, zero invoice amount (div-by-zero guard), multi-invoice, and LEFT-JOIN row deduplication. Good shape.

2. OverflowMenu shared component — API is generic enough for reuse beyond deposits: items[] with label/onClick/disabled/variant/icon, placement: 'bottom-end' | 'top-end', triggerIcon, triggerAriaLabel, disabled, data-testid. The component owns its own isOpen state, fully eliminating the menuOpenId / onMenuToggle prop-threading and the two duplicate copies of handleMenuKeyDown / handleTriggerKeyDown (~280 lines of duplicate keyboard-nav code gone). ARIA contract preserved (aria-haspopup, aria-expanded, role="menu", role="menuitem"), so existing E2E locators continue to work. The split is genuinely clean — not just shuffled bulk. VendorsPage still has its own local copy of the equivalent CSS (menuButton / menuItem / menuItemDanger), so a separate follow-up can migrate it onto OverflowMenu later — out of scope for this PR.

3. Inline style={{}} removed — tableRowMutating CSS module class with prefers-reduced-motion guard. Correct.

4. Error surfacing — handleRevertToPending / handleRevertToPaid now show a section-level FormError (auto-dismiss 6 s, with cleanup on unmount). handleStateConfirm shows FormError inside the state-confirm modal. Both branches distinguish ApiClientError (translated code) from generic Error (network-error fallback key). Two new i18n keys added, German mirrored.

Focus-ring token

You asked whether item focus should use var(--color-focus-ring) rather than var(--color-primary). I checked the codebase: the design system is genuinely inconsistent here — MilestonePanel.module.css:778, WorkItemSelector.module.css:142, and the previous local InvoiceDepositsSection.menuItem:focus-visible (now deleted) all used var(--color-primary); DiaryEntryTypeSwitcher uses var(--color-focus-ring); GanttSidebar and MilestonePanel (elsewhere) use var(--color-border-focus). The new OverflowMenu preserves the previous behaviour exactly (the deleted .menuItem:focus-visible rule used the same inset 0 0 0 2px var(--color-primary)), so this PR introduces no regression. The broader token-naming cleanup for menu-item focus rings across the design system is a separate UX-designer concern — not a blocker for this PR.

Architecture / contract compliance

• No DB or migration changes — schema unaffected.
• No API contract changes — wiki updates not required.
• Existing E2E locators (role-based) continue to work; the new Scenario 8 spec exercises the new error-banner path end-to-end through a page.route() mock and properly tears down with page.unroute().
• CI: Quality Gates ✅, E2E Gates ✅ (all 16 shards).

Minor observations (non-blocking)

• InvoiceDetailPage no longer threads invoiceTotal into InvoiceDepositsSection, and AddEditDepositModal no longer takes formatCurrency. Both are correct dead-prop removals exposed by the simplification — good follow-through.
• _invoiceId underscore destructuring in aggregateInvoiceStatusBreakdown is consistent with project conventions for intentionally unused identifiers.

Ship it.

[steilerDev]

[ux-designer]

Design review against the visual spec posted on #1413.

---

Overall verdict: APPROVED (with one non-blocking note)

All critical token mappings, ARIA structure, keyboard navigation, dark mode, and mobile touch targets match the spec. One medium finding and one informational note are called out below; neither is blocking for merge.

---

Checklist pass

Check	Result
--shadow-focus on trigger :focus-visible	PASS — box-shadow: var(--shadow-focus)
--z-dropdown on menu surface	PASS
Spacing tokens throughout (no hardcoded px for padding)	PASS
Mobile 44 px touch target on trigger	PASS — min-width/height: 44px
Mobile 44 px touch target on items (@media max-width: 767px)	PASS — padding: var(--spacing-3) var(--spacing-4)
aria-haspopup="true" on trigger	PASS
aria-expanded on trigger	PASS
role="menu" on dropdown	PASS
role="menuitem" on items	PASS
ArrowDown / ArrowUp / Home / End / Escape keyboard nav	PASS
ArrowDown on trigger opens menu	PASS
Outside-click closes menu	PASS — mousedown listener on document
Escape returns focus to trigger	PASS
No [data-theme="dark"] overrides needed / present	PASS — all tokens cascade
No hardcoded color values	PASS
prefers-reduced-motion guards on trigger and items	PASS
tableRowMutating uses var(--transition-fast) with motion guard	PASS
No inline style={{}} remaining in call site	PASS
Old per-page menu classes removed from InvoiceDepositsSection.module.css	PASS

---

Finding 1 — Non-blocking: menu item focus ring uses --color-primary instead of --shadow-focus (inset form)

File: client/src/components/OverflowMenu/OverflowMenu.module.css, .item:focus-visible

The spec required the inset focus ring to mirror the project's canonical focus token, expressed as the inset equivalent:

/* Spec */
.item:focus-visible {
  box-shadow: inset 0 0 0 3px var(--color-focus-ring);
}

The implementation uses:

/* Implemented */
.item:focus-visible {
  box-shadow: inset 0 0 0 2px var(--color-primary);
}

Two differences:

• Ring width: 2 px instead of 3 px (mismatches trigger's --shadow-focus which is 3 px).
• Color: var(--color-primary) is a fully-opaque blue (#2563eb / #3b82f6 dark), whereas --color-focus-ring is the semi-transparent blue (rgba(59,130,246,0.3) / rgba(96,165,250,0.4) dark) used consistently by every other focusable element in the codebase. The opaque solid blue can appear harsh against some item backgrounds in dark mode.

This is a consistency issue, not a contrast failure or an accessibility blocker. var(--color-primary) is a valid token and the focus ring is visible. Fix before merge or in refinement.

Additionally: .itemDanger:focus-visible has no rule at all. The spec required:

.itemDanger:focus-visible {
  box-shadow: inset 0 0 0 3px var(--color-focus-ring-danger);
}

Without this rule, destructive items receive the same blue focus ring as default items — a minor but noticeable inconsistency when keyboard-navigating to a red item. Fix alongside the default item ring change.

---

Finding 2 — Informational: disabled items are not skipped by arrow-key navigation

File: client/src/components/OverflowMenu/OverflowMenu.tsx, handleMenuKeyDown

The spec note said: "Disabled items must be skipped by the arrow-key handler (querySelectorAll('[role="menuitem"]:not(:disabled)'))"

The implementation queries all [role="menuitem"] including disabled ones. A keyboard user pressing ArrowDown will land on a disabled button — which is technically reachable by focus (browsers do focus disabled buttons in some configurations) but cannot be activated. This is a low/informational finding for the current use cases (disabled items are rare in the deposits section), but worth addressing before OverflowMenu is adopted more widely.

---

Open follow-up (unchanged from prior reviews)

The bottom-sheet modal adaptation for mobile (issue #1404) is not part of this PR's scope and remains an open follow-up item.

---

Approved. The implementation is a clean extraction of the existing inline menu into a well-structured shared component with full token compliance, correct ARIA, and keyboard navigation. The two findings above should be addressed before or shortly after merge.

[github-actions]

🎉 This PR is included in version 2.6.0-beta.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀