feat(budget): move per-source filter to server-side via ?deselectedSources=#1361
feat(budget): move per-source filter to server-side via ?deselectedSources=#1361steilerDev merged 7 commits intobetafrom
Conversation
…urces= The client-side filter introduced in #1356/#1358 left subsidy oversubscription stuck at the project-wide value while filtered cost shrank, producing nonsensical math where Payback could exceed visible Cost. This story moves the filter server-side: GET /api/budget/breakdown now accepts a ?deselectedSources=<id1>,<id2>,unassigned query parameter, filters lines at the top of the breakdown service pipeline, re-runs the subsidy engine against the filtered cost set, and emits filter-aware aggregates and adjustments. The client refetches on each chip toggle (50ms debounce + AbortController + stale-while-revalidate) and removes the entire client-side aggregation layer (~530 lines). Backend: - shared/src/types/budgetBreakdown.ts: BudgetSourceSummaryBreakdown adds subsidyPaybackMin and subsidyPaybackMax (pro-rata from filtered engine). - server/src/services/budgetBreakdownService.ts: accepts deselectedSources; filters lines at top; per-source projected stays unfiltered; per-source payback derived pro-rata from filtered engine; cascade-prunes empty items/areas server-side. - server/src/routes/budgetOverview.ts: parses ?deselectedSources=, accepts comma-separated source IDs and the literal "unassigned"; unknown IDs are silently ignored. - wiki/API-Contract.md: documents the new query param + response shape. Frontend: - client/src/lib/budgetOverviewApi.ts: fetchBudgetBreakdown accepts deselectedSources?: string[]. - client/src/pages/BudgetOverviewPage/BudgetOverviewPage.tsx: 50ms debounce, AbortController cancellation, stale-while-revalidate via .breakdownRefetching opacity dim, error banner with dismiss button. - client/src/components/CostBreakdownTable/CostBreakdownTable.tsx: deletes computeFilteredAggregates, computePerSourcePayback, FilteredAggregates, FilteredEntityTotals, areaHasVisibleLines, all filtered memos, all cascade-hide guards, all filteredAggregates ternaries. Renders directly from server fields. Source row Payback column uses resolveProjected(source.subsidyPaybackMin, source.subsidyPaybackMax, perspective). Synthetic 'unassigned' source from the response is rendered through the normal source-row loop. i18n: refetchError + dismissError keys added in EN and DE. Architectural decisions (recorded on issue #1360): - A: budgetSources[] always includes ALL configured sources; per-source projectedMin/Max stays unfiltered (deselected source rows stay informative). - B: Per-source subsidyPaybackMin/Max emitted by server (pro-rata from filtered engine). Deselected sources get 0. - C: Pure server filter, no caching beyond stale-while-revalidate on client. - D: 50ms debounce + AbortController. - Scope: /api/budget/overview filter-awareness deferred (hero card stays project-wide for v1). This explicitly supersedes the #1356 decision that "subsidy adjustments stay project-wide". Closes #1360 Co-Authored-By: Claude backend-developer (Haiku 4.5) <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> Co-Authored-By: Claude product-owner (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude product-architect (Sonnet 4.5) <noreply@anthropic.com> Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com>
…urces Migration 0021 seeds a 'discretionary-system' row in budget_sources, and architect decision A on issue #1360 means budgetSources[] now always includes ALL configured sources (including system-seeded ones). Tests that asserted empty arrays or fixed lengths/indices were stale. - Replace toHaveLength(N) + [0]! patterns with .find(s => s.id === ...) lookups across the 'budgetSources aggregate' describe block. - Filter out discretionary-system + unassigned in "empty sources" assertions. - Scenario 9 length assertion relaxed to >= 3. - Scenarios 7b and 10: drop categoryIds filter on subsidies — the lines insert with budgetCategoryId=null, so a category-filtered subsidy correctly returned 0 payback. Use a universal subsidy instead. Fixes #1360 Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
CI revealed two more groups of stale assertions: - 4 client-side cascade-hide tests in CostBreakdownTable.test.tsx asserted filter behavior that has moved server-side in #1360. Deleted scenarios 31, 8, 32, 33, 35 plus the now-unused buildBreakdownWithMixedSourceLines helper. Equivalent coverage exists in backend integration tests + E2E. - Service test "no user sources" assertion now also filters the synthetic 'unassigned' entry that the server emits when any line has null source. - Service test Scenario 7b: added maximumAmount: 100 to the subsidy program so the 20% subsidy actually overflows its cap. subsidyAdjustments only emits oversubscribed entries; without a cap the array stays empty. - insertSubsidyProgram helper accepts optional maximumAmount. Fixes #1360 Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
…ce rows
The CostBreakdownTable component renders an early-return empty state
when both workItems.areas and householdItems.areas are empty. The
'makeFilteredEmptyBreakdown' fixture used by 4 tests caused the table
to switch into empty state after refetch — at which point the source
detail rows and the Available Funds button no longer existed in the
DOM, so subsequent Playwright assertions failed with "element(s) not
found".
- New makeFilteredBreakdownBankLoanDeselected({includeSourceB?}) fixture:
filtered response that keeps at least the unassigned line, so
workItems.areas is non-empty and the source rows stay rendered.
- Use the new fixture in all four affected tests instead of the empty
one (where the test isn't specifically validating the empty state).
- Move aria-pressed='false' assertions BEFORE awaiting the refetch
promise — the attribute is driven by URL search params (synchronous
on click), not by the network round-trip.
- "Cascade-hide on mobile": expand Work Items section before the
deselection (while hasData=true). The previous order tried to expand
AFTER the empty fixture replaced the table.
Fixes #1360
Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
…#1360) The CostBreakdownTable's empty-state guard previously triggered whenever both wiAreas and hiAreas were empty — including the case where a user deselects all sources and the server prunes every item. That UX dead-end prevented re-enabling sources because the source rows themselves were no longer rendered. Production fix: the guard now also requires budgetSources.length === 0. When sources are configured (selected or deselected), the full table renders with source detail rows, the Available Funds expand button, the Sum row (€0.00 across the board), and the Remaining Budget row (showing the project-wide availableFunds). The user can re-enable any source by clicking its row. Test: add Scenario 24 to "Server-driven render path (#1360)" verifying the all-deselected case keeps the rest of the table visible with zeroed totals and absent empty-state copy. Fixes #1360 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>
…tree The 'restores filter on load' test navigates with ?deselectedSources=src-a and the filtered fixture returns empty areas. The previous helper tried to expand the Work Items section + Main Area + Main Work Item, but those buttons don't exist when areas is empty. Just goto + waitForLoaded; the test goal is verifying the URL param drives the filter state and Source A lines are absent — no expansion needed. Fixes #1360 Co-Authored-By: Claude e2e-test-engineer (Sonnet 4.5) <noreply@anthropic.com>
- Switch .breakdownErrorBanner color from --color-text-primary to --color-text-inverse so the banner text remains readable on top of --color-bg-inverse in both light and dark mode (the previous pairing resolved to near-white text on light grey in dark mode). - Replace dismiss button focus-visible outline with the standard --shadow-focus pattern used across the app. Fixes #1360 Co-Authored-By: Claude frontend-developer (Haiku 4.5) <noreply@anthropic.com>
|
[product-architect] Approve (verdict: approve — comment instead of Architect decisions A/B/C/D verified end-to-end:
Nit (informational, not blocking): post-migration 0023, no production line has LGTM. |
steilerDev
left a comment
There was a problem hiding this comment.
[security-engineer] PR #1361 security review — APPROVED
No blocking findings.
Verified the three security-relevant surfaces:
Query param parsing (): Raw string split on ,, trimmed, empty strings filtered out via filter(Boolean). Result is a Set<string> consumed only by Set.has() lookups — no user input reaches any SQL query. Unknown IDs are silently ignored, which is the correct behavior (no error oracle). No injection vector.
Filter implementation (): Filtering is entirely in-memory on already-fetched row arrays (filteredWiLineRows, filteredHiLineRows). The isLineFiltered() helper uses only Set.has() and strict null equality — no SQL interpolation of user input at any point. Per-source projections iterate the unfiltered original rows (architect decision A), which is correct and expected.
Authorization: The existing preValidation session hook remains in place and unchanged. The new query-param path is inside the same authenticated handler. No bypass introduced.
Frontend: encodeURIComponent(join(',')) correctly URL-encodes the deselected IDs before they reach the server. The unassigned literal is a hardcoded string in the client and is handled as a known sentinel on the server — no reflection of arbitrary user data into rendered output. No dangerouslySetInnerHTML or raw DOM injection found in the changed files.
Informational — no schema validation on deselectedSources: The query param has no Fastify schema constraint (no maxLength on individual IDs, no maxItems on the comma-separated list). In practice the Set is only ever used for Set.has() comparisons so there is no exploit path, but adding a schema-level bound (e.g., max param string length of 4096 chars) would be a defense-in-depth improvement. Low priority, consistent with prior findings on this route (#466).
Checklist: injection ✓ | auth ✓ | sensitive data ✓ | input validation ✓ | dependencies ✓ | secrets ✓
steilerDev
left a comment
There was a problem hiding this comment.
[security-engineer] PR #1361 security review -- APPROVED
No blocking findings.
Verified the three security-relevant surfaces:
Query param parsing (budgetOverview.ts lines 30-71): Raw string split on ,, trimmed, empty strings filtered out via filter(Boolean). Result is a Set<string> consumed only by Set.has() lookups -- no user input reaches any SQL query. Unknown IDs are silently ignored, which is the correct behavior (no error oracle). No injection vector.
Filter implementation (budgetBreakdownService.ts): Filtering is entirely in-memory on already-fetched row arrays (filteredWiLineRows, filteredHiLineRows). The isLineFiltered() helper uses only Set.has() and strict null equality -- no SQL interpolation of user input at any point. Per-source projections iterate the unfiltered original rows (architect decision A), which is correct and expected.
Authorization: The existing preValidation session hook remains in place and unchanged. The new query-param path is inside the same authenticated handler. No bypass introduced.
Frontend: encodeURIComponent(join(",")) correctly URL-encodes the deselected IDs before they reach the server. The unassigned literal is a hardcoded string in the client and handled as a known sentinel on the server -- no reflection of arbitrary user data into rendered output. No dangerouslySetInnerHTML or raw DOM injection found in the changed files.
Informational -- no schema validation on deselectedSources: The query param has no Fastify schema constraint (no maxLength on individual IDs, no maxItems on the comma-separated list). In practice the Set is only ever used for Set.has() comparisons so there is no exploit path, but a schema-level bound (e.g., max param string length of 4096 chars) would be a defense-in-depth improvement. Low priority, consistent with prior findings on this route (#466).
Checklist: injection OK | auth OK | sensitive data OK | input validation OK | dependencies OK | secrets OK
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] PR #1361 review — APPROVED (advisory; cannot self-approve as repo owner)
All 31 acceptance criteria from issue #1360 verified. Architect decisions A/B/C/D honored. The canonical bug (subsidy oversubscription consistency) is fixed.
Server (AC #1–#10) — getBudgetBreakdown(db, deselectedSources) filters at the top of the pipeline (filteredWiLineRows/filteredHiLineRows). Subsidy engine re-runs on the filtered set, items/areas with no surviving lines are pruned, route accepts ?deselectedSources= with comma-separated UUIDs and the unassigned literal. Unknown UUIDs degrade silently. Route defaults to empty Set (backward compatible). Backend tests Scenarios 1–10 in budgetBreakdownService.test.ts and Scenarios 11–15 in budgetOverview.breakdown.test.ts cover every server-side AC including the 401 unauth case.
Decision A honored: sourceProjectedMap iterates the UNFILTERED workItemLineRows/hiLineRows, so deselected sources keep their full projected contribution in budgetSources[]. BudgetSourceSummaryBreakdown in shared/src/types/budgetBreakdown.ts now carries subsidyPaybackMin/subsidyPaybackMax (Decision B), pro-rata attributed from the filtered engine run via sourcePaybackMap. Synthetic unassigned entry emitted whenever any null-source lines exist.
Client (AC #11–#17) — BudgetOverviewPage.tsx debounce + AbortController + stale-while-revalidate effect implements Decision D with DEBOUNCE_MS = 50. The wrapper div toggles styles.breakdownRefetching (opacity 0.6, pointer-events: none) so the previous breakdown stays visible during refetch — no flicker. Refetch errors render an inline role=\"alert\" banner with a Dismiss button; the previous breakdown is preserved (AC #14). Initial load reads deselectedSourceIds from the URL before the first fetch (AC #15).
Code deletion (AC #18–#28) — every named symbol from issue #1360 is gone: computeFilteredAggregates, computePerSourcePayback, filteredAdjustedTotalPayback, filteredRawProjected, filteredAvailableFunds, unassignedAllocatedCost, visibleLineIds, FilteredAggregates/FilteredEntityTotals, areaHasVisibleLines, all hasSourceFilter && … cascade guards, all filteredAggregates-conditional ternaries. Section/area/item rows read directly from server-provided *.rawProjectedMin/Max and *.subsidyPayback. Per-source row Payback reads source.subsidyPaybackMin/Max.
Tests (AC #29–#32) — service + route tests cover every server AC. Client tests cover debounce/abort/SWR/URL-on-mount/error path (Scenarios 24–29). E2E budget-source-filter.spec.ts adds the canonical subsidy oversubscription scenario (Scenario D), stale-while-revalidate (Scenario E), URL-on-mount (Scenario F), and rapid debounce coalescence (Scenario H).
Late-cycle empty-state UX fix — verified in CostBreakdownTable.tsx: early-return empty state now fires only when !hasData && !hasSources. When all sources are deselected (server prunes areas, but budgetSources[] is non-empty), the full table renders — source rows, Available Funds, Sum, Remaining Budget all visible with zeroed totals. The user is no longer trapped in an empty-state dead end. Regression test "Scenario 24" in CostBreakdownTable.test.tsx locks this in.
Security-engineer approved separately. Cleared for merge.
steilerDev
left a comment
There was a problem hiding this comment.
[ux-designer]
Reviewed the UX surfaces in PR #1361. Two findings — one critical (dark mode contrast), one medium (recurring focus token pattern). This would be --request-changes on an external PR; posting as a comment since this is our own PR.
Critical — Dark mode contrast failure in .breakdownErrorBanner
The error banner uses background: var(--color-bg-inverse) but color: var(--color-text-primary) for both the banner text and the dismiss button.
In dark mode these tokens resolve to:
--color-bg-inverse→--color-gray-100=#f3f4f6(light gray surface)--color-text-primary→--color-slate-50=#f1f5f9(near-white text)
Near-white text on a light gray background is essentially invisible — far below WCAG AA 4.5:1. The same token conflict exists on the dismiss <button> inside the banner.
Fix: On any element using --color-bg-inverse as its background, pair it with --color-text-inverse for text (resolves to #fff in light mode, --color-gray-900 in dark mode — correct in both).
.breakdownErrorBanner {
background: var(--color-bg-inverse);
color: var(--color-text-inverse); /* not --color-text-primary */
}
.breakdownErrorBanner button {
color: var(--color-text-inverse); /* not --color-text-primary */
}This matches the established GanttTooltip pattern documented in the Style Guide.
Medium — Focus ring uses outline instead of --shadow-focus token
/* current */
.breakdownErrorBanner button:focus-visible {
outline: 2px solid var(--color-primary);
outline-offset: 2px;
}Per the design system, focus rings must use box-shadow: var(--shadow-focus). outline: 2px solid var(--color-primary) is a recurring pattern flagged in PRs #402 and #414.
Fix:
.breakdownErrorBanner button:focus-visible {
box-shadow: var(--shadow-focus);
outline: none;
}Approved patterns
.breakdownRefetching—opacity: 0.6,pointer-events: none,transition: opacity var(--transition-fast)— all tokens, correctly guarded by@media (prefers-reduced-motion: reduce) { transition: none; }. No issues.- New i18n keys
refetchError/dismissErroruset()correctly in both EN and DE. - Empty-state UX fix (deselecting all sources keeps table visible) — correct behavior, no styling concerns.
- No new design tokens introduced — confirmed.
|
🎉 This PR is included in version 2.4.0-beta.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Moves the per-source filter from the client to the server, fixing the subsidy
oversubscription consistency bug introduced by #1356/#1358 (filtered Cost
could be lower than displayed Payback because the subsidy engine ran against
the unfiltered project).
GET /api/budget/breakdown?deselectedSources=<id1>,<id2>,unassignedacceptsa comma-separated list of deselected source IDs (and the
unassignedliteral). Empty/missing param = full project-wide breakdown.
against the filtered cost set, and emits filter-aware aggregates +
per-source pro-rata payback (
subsidyPaybackMin,subsidyPaybackMax).the deselected source stays informative — user sees what they're filtering
away).
stale-while-revalidate (previous breakdown stays visible at reduced opacity).
computeFilteredAggregates,computePerSourcePayback, all filtered memos, all cascade-hide guards).Closes #1360
This explicitly supersedes the #1356 decision that "subsidy adjustments stay
project-wide".
Test plan
consistency scenario added — canonical bug verification)
source IDs,
unassignedliteral, cascade pruning, per-source unfilteredprojections, per-source pro-rata payback
Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com