feat(household-items): add budget and subsidy integration for household items#401
feat(household-items): add budget and subsidy integration for household items#401steilerDev merged 7 commits intobetafrom
Conversation
…ld items Adds full budget and subsidy tracking for household items, including budget line items per household item, subsidy sources, payback records, and integration into the budget overview. The household item detail page is updated with collapsible budget and subsidy sections. Fixes #392 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) <noreply@anthropic.com> 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 qa-integration-tester (Haiku 4.5) <noreply@anthropic.com>
…ram mocks TypeScript strict mode requires the applicableCategories field on SubsidyProgram objects. Add empty array to all three mock objects in householdItemSubsidiesApi.test.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
- Add budgetSummary (totalPlanned, totalActual, subsidyReduction, netCost) to household item detail response - Fix confidence margin display (multiply by 100 for percentage) - Fix test category name collision with seeded data - Add focus-visible, reduced-motion, touch targets, aria-labels - Fix EPIC reference comments in app.ts Fixes #392 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude backend-developer (Haiku) <noreply@anthropic.com> Co-Authored-By: Claude frontend-developer (Haiku) <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
…ects Update all typed HouseholdItemSummary and HouseholdItemDetail mock objects across test files to include the new budgetSummary field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
steilerDev
left a comment
There was a problem hiding this comment.
[ux-designer]
Design Review — Story 4.6 Budget Integration (PR #401)
I reviewed the CSS additions to HouseholdItemDetailPage.module.css and the corresponding TSX rendering in HouseholdItemDetailPage.tsx against the Cornerstone design system.
Overall verdict: request-changes — Two medium accessibility issues require correction before merge.
Token Adherence
Token usage is excellent throughout the new sections. All colors, spacing, typography, radius, and transition duration values reference Layer 2 semantic tokens. No hardcoded hex values or magic numbers found. Specifically:
- All background colors use
var(--color-bg-secondary),var(--color-bg-tertiary),var(--color-bg-primary)correctly. - The subsidy payback summary correctly uses
var(--color-success-bg),var(--color-success-border),var(--color-success-text-on-light)— consistent with the pattern established in Story 5.x. var(--color-success-text-on-light)on.subsidyReductionis correct for positive-money framing.var(--radius-full)on.budgetLineMetaItemis correct for pill chips.
Findings
[MEDIUM] :focus instead of :focus-visible on form inputs (CSS, lines ~775–780)
HouseholdItemDetailPage.module.css
/* CURRENT — incorrect */
.formInput:focus,
.formSelect:focus {
outline: none;
border-color: var(--color-primary);
box-shadow: var(--shadow-focus-subtle);
}
/* REQUIRED — consistent with all other interactive elements in this file */
.formInput:focus-visible,
.formSelect:focus-visible {
outline: none;
border-color: var(--color-primary);
box-shadow: var(--shadow-focus-subtle);
}Every other interactive element in this file (backLink, infoLink, workItemLink, button, secondaryButton, editButton, deleteButton, cancelButton, confirmDeleteButton) uses :focus-visible. The new form inputs are the only exception and will show a focus ring on mouse click, which is inconsistent with the established pattern and deviates from the design system convention.
Note: shared.module.css .input and .select both use :focus-visible. The new .formInput/.formSelect are functionally identical to those shared classes — consider using composes: input from '../../styles/shared.module.css' to eliminate the duplication entirely.
[MEDIUM] New transitions not included in prefers-reduced-motion guard (CSS, lines ~721–735)
The existing @media (prefers-reduced-motion: reduce) block at line 721 only covers pre-existing button classes. The following new classes all declare transition: but are not guarded:
| Class | Transition declared |
|---|---|
.formInput, .formSelect |
border-color var(--transition-normal) |
.budgetLineItem |
background-color var(--transition-normal) |
.subsidyItem |
background-color var(--transition-normal) |
.unlinkButton |
all var(--transition-normal) |
The prefers-reduced-motion block must be extended:
@media (prefers-reduced-motion: reduce) {
.button,
.secondaryButton,
.editButton,
.deleteButton,
.cancelButton,
.confirmDeleteButton,
.deliveryStep,
.deliveryLine,
.backLink,
.infoLink,
.workItemLink,
/* ADD these new classes: */
.formInput,
.formSelect,
.budgetLineItem,
.subsidyItem,
.unlinkButton {
transition: none;
}
}[LOW] unlinkButton touch target is below the 44×44px minimum on mobile
.unlinkButton has padding: var(--spacing-1) (4px all sides) and font-size: var(--font-size-xl) (20px), yielding an approximate rendered size of 28×28px. WCAG 2.1 Success Criterion 2.5.5 requires a minimum 44×44px touch target for interactive controls.
The mobile breakpoint (max-width: 767px) does not add a minimum height/width to .unlinkButton. Required fix:
/* In the @media (max-width: 767px) block */
.unlinkButton {
min-width: 44px;
min-height: 44px;
display: flex;
align-items: center;
justify-content: center;
}[LOW] unlinkButton uses transition: all — prefer explicit property list
/* CURRENT */
.unlinkButton {
transition: all var(--transition-normal);
}
/* PREFERRED */
.unlinkButton {
transition: color var(--transition-normal), background-color var(--transition-normal);
}transition: all is flagged informational per established review patterns (see PR #380 review).
[LOW] Subsidy <select> has no accessible label
In HouseholdItemDetailPage.tsx, the standalone <select> in the addSubsidyRow block has no <label> element and no aria-label:
{/* CURRENT — no label */}
<select
value={selectedSubsidyId}
onChange={(e) => setSelectedSubsidyId(e.target.value)}
className={styles.formSelect}
disabled={isLinkingSubsidy}
>The placeholder — Link Subsidy Program — option text is not a substitute for an accessible label. Screen readers will not announce the purpose of this control.
{/* REQUIRED */}
<select
value={selectedSubsidyId}
onChange={(e) => setSelectedSubsidyId(e.target.value)}
className={styles.formSelect}
disabled={isLinkingSubsidy}
aria-label="Select subsidy program to link"
>[LOW] unlinkButton accessible name does not identify which subsidy is being unlinked
title="Unlink subsidy" is the same for every unlink button in the list. A screen reader user navigating by button will hear "Unlink subsidy, Unlink subsidy, Unlink subsidy" with no way to distinguish which row each button belongs to.
{/* CURRENT */}
<button
type="button"
className={styles.unlinkButton}
onClick={() => void handleUnlinkSubsidy(subsidy.id)}
title="Unlink subsidy"
>
×
</button>
{/* REQUIRED */}
<button
type="button"
className={styles.unlinkButton}
onClick={() => void handleUnlinkSubsidy(subsidy.id)}
aria-label={`Unlink ${subsidy.name}`}
>
×
</button>[INFORMATIONAL] Mobile breakpoint does not add min-height: 44px to new budget form action buttons
The existing mobile block adds min-height: 44px to .editButton, .deleteButton etc. in the page header actions. The new .budgetFormActions submit/cancel buttons and the "Add" button in .addSubsidyRow are not covered. On mobile viewports these buttons may be rendered smaller than the 44px touch minimum.
Consider adding to the @media (max-width: 767px) block:
.budgetFormActions .button,
.budgetFormActions .cancelButton,
.addSubsidyRow .button {
min-height: 44px;
}[INFORMATIONAL] .formInput/.formSelect duplicates shared.module.css
The new .formInput and .formSelect classes are nearly identical to .input and .select in shared.module.css. Using composes: would eliminate duplication and ensure future shared-class updates are automatically reflected:
.formInput {
composes: input from '../../styles/shared.module.css';
}
.formSelect {
composes: select from '../../styles/shared.module.css';
}This also resolves the :focus vs :focus-visible issue (finding #1) since the shared classes already use :focus-visible.
Summary
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Medium | HouseholdItemDetailPage.module.css ~775 |
:focus instead of :focus-visible on form inputs |
| 2 | Medium | HouseholdItemDetailPage.module.css ~721 |
New transitions missing from prefers-reduced-motion guard |
| 3 | Low | HouseholdItemDetailPage.module.css ~961 |
unlinkButton touch target < 44px on mobile |
| 4 | Low | HouseholdItemDetailPage.module.css ~971 |
transition: all on unlinkButton |
| 5 | Low | HouseholdItemDetailPage.tsx ~947 |
Subsidy <select> has no accessible label |
| 6 | Low | HouseholdItemDetailPage.tsx ~935 |
unlinkButton title does not identify the subsidy by name |
| 7 | Informational | HouseholdItemDetailPage.module.css ~612 |
Mobile breakpoint missing min-height: 44px for new form action buttons |
| 8 | Informational | HouseholdItemDetailPage.module.css ~763 |
.formInput/.formSelect duplicates shared .input/.select patterns |
Token usage is excellent overall — no hardcoded values, good dark mode coverage, correct semantic token choices for the success/subsidy payback framing.
Note: As the reviewer is the same agent that authored the UX spec, --request-changes is the appropriate verdict but cannot be applied to own PRs via the GitHub API. This review is posted as a comment with request-changes intent — the two medium findings must be addressed before merge.
steilerDev
left a comment
There was a problem hiding this comment.
[security-engineer] Security review for PR #401 — Story 4.6: Household Items Budget Integration.
Review Summary
This PR introduces 3 new backend services (householdItemBudgetService, householdItemSubsidyService, householdItemSubsidyPaybackService), 3 new route files, modifications to budgetOverviewService (UNION ALL approach), 2 frontend API client files, and budget/subsidy UI on HouseholdItemDetailPage.
No blocking security findings. The implementation follows all established security patterns from prior EPIC reviews.
Checklist
- No SQL/command/XSS injection vectors in new code
- Authentication enforced on all new endpoints (
if (!request.user) throw new UnauthorizedError()present on every handler — 8 route handlers across 3 files) - No sensitive data exposed in logs, errors, or client responses
- User input validated and sanitized at API boundaries (both schema-layer and service-layer validation)
- No new dependencies introduced
- No hardcoded credentials or secrets
- CORS configuration unchanged
- Error responses use generic messages for auth failures
Positive Observations
Authentication/Authorization: All 8 new route handlers (4 in householdItemBudgets.ts, 3 in householdItemSubsidies.ts, 1 in householdItemSubsidyPayback.ts) correctly check if (!request.user) before doing any work. This is consistent with the established pattern.
Authorization (IDOR protection): The budget line update/delete handlers in householdItemBudgetService.ts verify ownership by querying with both budgetId AND householdItemId in the WHERE clause via and(eq(householdItemBudgets.id, budgetId), eq(householdItemBudgets.householdItemId, householdItemId)). This correctly prevents cross-household-item budget line access.
SQL injection: All database operations use Drizzle ORM parameterized queries throughout. The sql tagged templates in householdItemSubsidyPaybackService.ts use parameterized bindings (${householdItemId}, ${id}) and sql.join for the IN-list construction — no dynamic string concatenation into SQL.
Input validation: Schemas define additionalProperties: false on all request bodies, enums are enforced at both schema and service layer, maxLength: 500 on description fields, minimum: 0 on plannedAmount. Service-layer validates FK existence before INSERT.
XSS: Zero use of dangerouslySetInnerHTML, innerHTML, or eval. All dynamic content in the new budget/subsidy UI sections renders via JSX text nodes and standard attributes. The CONFIDENCE_LABELS record and category/source/vendor names rendered via {node.name} are safe React patterns.
Frontend page size: fetchVendors({ pageSize: 100 }) correctly uses the server-enforced maximum. fetchBudgetCategories(), fetchBudgetSources(), and fetchSubsidyPrograms() are unbounded but these collections are small and this is consistent with the rest of the codebase.
UNION ALL budget overview: The budgetOverviewService.ts refactor correctly uses a shared entityId field across work_item_budgets and household_item_budgets. The lineInvoiceMap only maps work_item_budget_id values; household budget line lookups will return undefined, correctly triggering confidence-margin-based calculation. UUIDs from both tables have negligible collision probability. No cross-entity data leakage risk.
Informational Finding — FK Validation Error Messages Echo Client-Supplied IDs
The error messages in householdItemBudgetService.ts at lines 186, 197, 208 echo the supplied ID back to the caller:
throw new ValidationError(`Budget category not found: ${budgetCategoryId}`);
throw new ValidationError(`Budget source not found: ${budgetSourceId}`);
throw new ValidationError(`Vendor not found: ${vendorId}`);This is the same pattern used in workItemBudgetService.ts (lines 250, 261, 272) and workItemService.ts (lines 252, 264), which has been accepted in previous reviews. In a single-tenant self-hosted application the authenticated user already has full read access to all these entity IDs through their respective list endpoints, so there is no enumeration risk. Noting for consistency tracking only — no action required.
Informational Finding — CONFIDENCE_MARGINS Display Value
In HouseholdItemDetailPage.tsx, the confidence margin is rendered as:
{CONFIDENCE_LABELS[line.confidence]} (±{CONFIDENCE_MARGINS[line.confidence]}%)CONFIDENCE_MARGINS values are decimal factors (e.g., 0.2 for own_estimate). This renders as ±0.2% instead of ±20%. The confidenceMargin field in the API response already contains the decimal value for the same reason. This is a display/UX bug, not a security issue — but worth noting so it can be addressed alongside any UX pass on this page.
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] Story 4.6 (#392) — Household Items Budget Integration
AC-by-AC Review
| AC | Description | Verdict | Notes |
|---|---|---|---|
| #1 | household_item_budgets table via migration |
PASS | Table defined in migration 0010 (from Story 4.1). Budget service correctly performs CRUD on it. |
| #2 | household_item_subsidies junction table |
PASS | Table defined in migration 0010 (from Story 4.1). M:N linking via POST/DELETE. |
| #3 | CRUD API for budget lines | PASS | POST/GET/PATCH/DELETE at /api/household-items/:id/budgets(/:budgetId) all present. |
| #4 | Subsidy API endpoints | PASS (with deviation) | AC specifies PUT /api/household-items/:id/subsidies (replace-all). Implementation uses POST (link one) + DELETE (unlink one). This is consistent with the existing work item subsidy pattern. Non-blocking deviation. |
| #5 | Budget section on detail page | PASS | Budget card shows all budget lines with description, planned amount, confidence, category, source, vendor. Subsidies card shows linked programs with reduction info. |
| #6 | "Add Budget Line" button with form | PASS | Button opens inline form with all required dropdowns (confidence, category, source, vendor). |
| #7 | Edit/delete budget lines with confirmation | PASS | Edit opens pre-filled form; delete shows Confirm/Cancel buttons inline. |
| #8 | Budget overview includes household items | PASS | budgetOverviewService.ts uses UNION ALL to combine work item + household item budget lines. Entity subsidy links also unioned. |
| #9 | Subsidy payback calculation | PASS | householdItemSubsidyPaybackService.ts implements same algorithm: percentage applies to matching-category lines with confidence margins, fixed amount is constant. |
| #10 | budgetSummary in GET detail response |
FAIL | AC requires GET /api/household-items/:id to return a budgetSummary field with totalPlanned, totalActual, subsidyReduction, netCost. The response only has totalPlannedAmount (from Story 4.1). There is no budgetSummary object, no totalActual, no subsidyReduction, and no netCost. The frontend computes "Total Planned" client-side and fetches payback via a separate endpoint, but the API contract does not match the AC. |
Blocking Issues
1. AC #10 — Missing budgetSummary in GET detail response (HIGH)
The GET /api/household-items/:id response must include a budgetSummary field as specified:
{
"budgetSummary": {
"totalPlanned": 5000,
"totalActual": 0,
"subsidyReduction": 750,
"netCost": 4250
}
}Since household items have no invoices, totalActual will always be 0. subsidyReduction should reflect the subsidy payback calculation. netCost = totalPlanned - subsidyReduction. This requires:
- Adding the fields to
HouseholdItemDetailinshared/src/types/householdItem.ts - Computing the values in
householdItemService.tsgetHouseholdItem()(can reusehouseholdItemSubsidyPaybackService) - Including them in the API response
2. Confidence margin display bug (MEDIUM)
The frontend displays confidence margins incorrectly. The CONFIDENCE_MARGINS values are decimal fractions (e.g., own_estimate: 0.2 means 20%), but the template renders them directly:
// HouseholdItemDetailPage.tsx line 826
{CONFIDENCE_MARGINS[line.confidence]}% // Shows "0.2%" instead of "20%"The WorkItemDetailPage correctly multiplies by 100:
// WorkItemDetailPage.tsx line 1476
Math.round(CONFIDENCE_MARGINS[line.confidence] * 100)% // Shows "20%"Fix: Use {Math.round(CONFIDENCE_MARGINS[line.confidence] * 100)} in the household item page to match the work item page pattern.
Non-Blocking Observations
-
Subsidy API pattern deviation (AC #4): AC specifies PUT replace-all, but implementation uses POST/DELETE per-item. This matches the established work item subsidy pattern and is arguably better for UX. No change needed, but the AC should be updated to reflect the actual pattern.
-
app.tscomment references EPIC-09: The route registration comments say// EPIC-09: Household Item Budget Integrationbut this is Story 4.6 in EPIC-04. Minor documentation inaccuracy. -
Test authorship: QA agent (
qa-integration-tester) co-authored the tests per commit trailers. 81 test cases across 4 test files. PASS.
Verdict
REQUEST CHANGES — AC #10 (budgetSummary in GET detail response) is not met, and the confidence margin display is incorrect (shows "0.2%" instead of "20%"). Both must be fixed before approval.
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect]
Architecture Review: Story 4.6 — Household Items Budget Integration
Reviewed for architecture compliance, API contract adherence, schema consistency, and code quality.
Verdict: Request Changes
The implementation correctly mirrors the work item budget system pattern and the budget overview UNION ALL approach is architecturally sound. However, there is a medium-severity display bug that deviates from the established pattern in the work item detail page.
Finding 1 — MEDIUM: Confidence margin display shows decimal fraction instead of percentage
File: client/src/pages/HouseholdItemDetailPage/HouseholdItemDetailPage.tsx, line 826
The household item detail page displays confidence margins as raw decimal fractions:
{CONFIDENCE_LABELS[line.confidence]} (±{CONFIDENCE_MARGINS[line.confidence]}%)This renders as "Own Estimate (±0.2%)" instead of "Own Estimate (±20%)".
The work item detail page correctly multiplies by 100 at WorkItemDetailPage.tsx:1476:
(+{Math.round(CONFIDENCE_MARGINS[line.confidence] * 100)}%)Fix: Change the household item line to:
{CONFIDENCE_LABELS[line.confidence]} (±{Math.round(CONFIDENCE_MARGINS[line.confidence] * 100)}%)Also note the work item page uses + prefix while this code uses ± prefix. The ± prefix is actually more semantically correct for a margin range, so this is fine to keep — just fix the multiplication.
Finding 2 — LOW: Incorrect EPIC reference in app.ts route registration comments
File: server/src/app.ts, lines 156, 161, 166
The route registration comments reference "EPIC-09" which does not exist:
// Household item budget line routes (EPIC-09: Household Item Budget Integration)This is EPIC-04 (Household Items & Furniture Management), Story 4.6. Fix the comments to reference "EPIC-04".
Finding 3 — LOW: Unused variable entityCounter in budget route test
File: server/src/routes/householdItemBudgets.test.ts, around line 44 (in the diff)
const entityCounter = 0;This variable is declared but never used. It should be removed to keep the test file clean.
Informational Notes
-
Budget overview UNION ALL correctness: The approach in
budgetOverviewService.tscorrectly unionswork_item_budgetsandhousehold_item_budgets, renamingwork_item_id/household_item_idto a genericentityId. ThelineInvoiceMap(built frominvoices.work_item_budget_id) will returnundefinedfor household item budget line IDs, which correctly triggers the "no invoices" code path. This is sound. -
Subsidy response shape vs API contract: The API contract shows a simplified 5-field subsidy response shape for GET/POST household item subsidies (just
id,name,reductionType,reductionValue,applicationStatus), but the implementation returns the fullSubsidyProgramobject. This is the same superset pattern established by the work item subsidy endpoints — the response is a strict superset of the contract, so clients that depend on the documented shape will work correctly. No action needed. -
Vendor fetch with
pageSize: 100: TheloadBudgetDatafunction usesfetchVendors({ pageSize: 100 })to populate the vendor dropdown. While the API contract documents maxpageSizeof 100, this silently omits vendors beyond page 1 if there are more than 100. At the target scale (<5 users, <100 vendors), this is acceptable. Same pattern as the household item create form (PR #399). -
N+1 query pattern in
toHouseholdItemBudgetLine: Each budget line resolves its related entities (category, source, vendor, user) with individual SELECT queries. This mirrors the work item budget service pattern. At the scale of this application (few budget lines per household item), this is acceptable. -
Shared type correctness:
HouseholdItemBudgetLinecorrectly importsConfidenceLevel,BudgetSourceSummary,VendorSummaryfrom the work item budget types, andBudgetCategory,UserSummaryfrom their respective modules. TheactualCost: 0,actualCostPaid: 0,invoiceCount: 0literal types enforce the "no invoices" invariant at the type level. -
Schema consistency: No new migration is needed — all tables (
household_item_budgets,household_item_subsidies) were created in migration 0010. The Drizzle schema already includes these tables. The service code correctly uses these tables.
Summary
Please fix the confidence margin display (multiply by 100) before merge. The EPIC reference and unused variable are low-severity cleanup items that should also be addressed.
…helper createBudgetSource validates totalAmount > 0, so use 10000 instead of 0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester (Haiku) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
steilerDev
left a comment
There was a problem hiding this comment.
[ux-designer]
Re-review of PR #401 — Story 4.6 Household Items Budget Integration
All 8 issues from the previous review have been verified as fixed. Approving.
Fix Verification
1. :focus → :focus-visible on .formInput and .formSelect — FIXED
HouseholdItemDetailPage.module.css lines 790–795 correctly use .formInput:focus-visible and .formSelect:focus-visible. No bare :focus pseudo-class remains on these elements.
2. New transitions added to prefers-reduced-motion guard — FIXED
The @media (prefers-reduced-motion: reduce) block (lines 731–749) now includes .formInput, .formSelect, .budgetLineItem, .subsidyItem, and .unlinkButton alongside the previously existing selectors. All animated elements are covered.
3. .unlinkButton touch target min-width/height: 44px at mobile — FIXED
The mobile breakpoint block (lines 716–719) adds min-width: 44px; min-height: 44px to .unlinkButton. Touch target meets the 44×44px minimum.
4. .unlinkButton transition: all → explicit properties — FIXED
Lines 986–988 now use transition: color var(--transition-fast), background-color var(--transition-fast) — explicit properties only, no transition: all.
5. Subsidy select aria-label="Select subsidy program" — FIXED
HouseholdItemDetailPage.tsx line 952 confirms aria-label="Select subsidy program" is present on the standalone select element.
6. Unlink buttons use aria-label={Unlink ${subsidy.name}} — FIXED
Line 936 confirms aria-label={\Unlink ${subsidy.name}`}` — each button has a distinct, item-specific accessible label.
7. Mobile form button touch targets min-height: 44px — FIXED
Lines 721–724 in the mobile block set min-height: 44px on .budgetFormActions .button and .budgetFormActions .cancelButton. Touch targets are adequate.
8. Informational (form input style duplication) — acknowledged
No change required; acknowledged as an informational note.
Additional Spot Checks (no new issues found)
- All color values throughout the CSS module use Layer 2 semantic tokens — no hardcoded hex.
- Dark mode correctness maintained via CSS custom properties throughout.
- Token usage consistent: spacing, font-size, border-radius, shadow, and transition tokens are all referenced via
var(). --z-modalused for modal z-index (line 554).- Focus rings use
var(--shadow-focus)andvar(--shadow-focus-danger)tokens consistently. - Tablet breakpoint correctly uses
max-width: 1023px(lines 1026), avoiding the 1024px overlap issue flagged in prior reviews. aria-modal="true"andaria-labelledbyare correctly set on the delete confirmation dialog (lines 1007–1009).- Focus trap implementation (Tab/Shift+Tab intercept with
modalRef) correctly implemented (lines 132–161).
steilerDev
left a comment
There was a problem hiding this comment.
[product-owner] Re-review of Story 4.6 (#392) — Household Items Budget Integration
Previous Blocking Issues — RESOLVED
1. AC #10: budgetSummary in GET detail response — FIXED
HouseholdItemBudgetSummaryinterface added toshared/src/types/householdItem.tswith all 4 required fields:totalPlanned,totalActual,subsidyReduction,netCostgetBudgetSummary()andgetTotalSubsidyReduction()implemented inhouseholdItemService.tsbudgetSummaryfield included intoHouseholdItemSummary(), so both list and detail API responses include ittotalActualcorrectly hardcoded to 0 (household items have no invoices)netCostcorrectly computed astotalPlanned - subsidyReduction- All test mocks updated across 7+ test files to include the
budgetSummaryfield
2. Confidence margin display — FIXED
- Changed from
{CONFIDENCE_MARGINS[line.confidence]}%to{Math.round(CONFIDENCE_MARGINS[line.confidence] * 100)}% - Now correctly displays "20%" instead of "0.2%" — matches the established WorkItemDetailPage pattern
Additional Fixes Verified
- EPIC-09 -> EPIC-04 comments in
app.ts: All route registration comments now correctly reference "EPIC-04: Household Items & Furniture Management" - Test category name collision: Fixed to avoid conflicts with seeded data
- UX designer findings addressed:
:focus-visible(not:focus),prefers-reduced-motionguard extended,aria-labelon subsidy select and unlink buttons, 44px touch targets on mobile, explicit transition properties onunlinkButton
AC-by-AC Re-verification
| AC | Description | Verdict |
|---|---|---|
| #1 | household_item_budgets table via migration |
PASS |
| #2 | household_item_subsidies junction table |
PASS |
| #3 | CRUD API for budget lines (POST/GET/PATCH/DELETE) | PASS |
| #4 | Subsidy API endpoints (POST/DELETE per-item) | PASS (non-blocking deviation from PUT replace-all) |
| #5 | Budget section on detail page | PASS |
| #6 | "Add Budget Line" button with inline form | PASS |
| #7 | Edit/delete budget lines with confirmation | PASS |
| #8 | Budget overview includes household items (UNION ALL) | PASS |
| #9 | Subsidy payback calculation | PASS |
| #10 | budgetSummary in GET detail response |
PASS |
Test Authorship
All test commits have qa-integration-tester co-author trailers. PASS.
Agent Reviews Present
- product-architect: request-changes (round 1), findings addressed in fix commit
- security-engineer: comment (no blocking findings)
- ux-designer: request-changes (round 1), findings addressed in fix commit
Verdict
APPROVE — All 10 acceptance criteria are met. Both previously blocking issues have been resolved. The non-blocking deviation on AC #4 (POST/DELETE per-item instead of PUT replace-all) is consistent with the established work item subsidy pattern and is acceptable.
Note: CI checks (Quality Gates, Docker) are still running at time of review. Approval is conditional on CI passing. Posted as comment because GitHub does not allow approving own PRs.
|
🎉 This PR is included in version 1.12.0-beta.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
HouseholdItemBudgetLineentities (planned amount, confidence level, vendor, budget category, budget source)HouseholdItemDetailPagewith collapsible budget and subsidy sectionsFixes #392
Test plan
householdItemBudgetService,householdItemSubsidyService,householdItemSubsidyPaybackServicehouseholdItemBudgets,householdItemSubsidies,householdItemSubsidyPayback)householdItemBudgetsApiandhouseholdItemSubsidiesApibudgetOverviewServicecorrectly includes household item budgets via UNION ALLHouseholdItemDetailPagerenders budget and subsidy sections correctlyCo-Authored-By: Claude dev-team-lead (Sonnet 4.6) noreply@anthropic.com