feat(budget-lines): full edit + linked-item move (#1553)

[steilerDev]

Summary

Closes #1553 — Full edit (all fields + linked item move) for budget lines on invoice/quotation detail page.

• Invoice → Budget Lines → Actions → Edit now opens the rich BudgetLineForm with all properties editable (description, category, confidence, funding source, vendor, pricing details, itemized amount, linked item), not just the itemized amount.
• Linked-item move semantics: changing the parent updates the underlying budget-line row in place (same-table) or performs an atomic insert+repoint+delete (cross-table WI ↔ HI). The IBL FK is repointed transactionally.
• WI/HI parity: the inline edit on Work Item / Household Item detail pages now exposes the same parent-picker affordance (onMoveBudgetLine wired through both detail pages). Same-table moves go through the entity-scoped PATCH; cross-table moves are only allowed via the invoice path (clear translated error otherwise).
• Backend: new editAndMoveBudgetLine() service on invoiceBudgetLineService; extended PATCH /api/invoices/:invoiceId/budget-lines/:id schema/handler; WI/HI PATCH endpoints extended with same-table newWorkItemId/newHouseholdItemId support.
• Error codes: BUDGET_LINE_ALREADY_LINKED (409), ITEMIZED_SUM_EXCEEDS_INVOICE (400), VALIDATION_ERROR, NOT_FOUND — all wired and translated.
• i18n: 9 new keys under budgetLineForm translated to German (Arbeitspaket, Haushaltsartikel, Budgetposition, Verknüpftes Element).
• Wiki: API-Contract.md updated to document the new PATCH body shape and error codes.

Locked architect decisions

• Cross-table category mapping: form-provided budgetCategoryId → existing WIB category → bc-household-items fallback.
• Endpoint strategy: extended the existing PATCH endpoint; no new route.
• Subsidy linkage on cross-table move: dropped silently (matches assignToHouseholdItem precedent).
• Audit trail: out of scope.
• WI/HI cross-table (no invoice link): rejected at the API; UI surfaces translated moveCrossTableNoInvoiceError.

Fixes #1553

Test plan

• Unit tests pass (95%+ coverage on new code)
• Integration tests pass (route + service)
• E2E smoke + 6 scenario specs pass against desktop/tablet/mobile
• Wiki API contract updated and committed in submodule
• Translator validated against glossary; no glossary additions needed

Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com

[steilerDev]

[security-engineer]

Security review of PR #1554 — full edit + linked-item move for invoice budget lines.

Verdict: APPROVE (no blocking findings)

All new code paths are properly authenticated, use Drizzle parameterized queries throughout, and the transactional atomicity is correctly implemented. No Critical or High findings. Three Low/Informational observations noted below.

---

Checklist

• No SQL/command/XSS injection vectors — all queries use Drizzle ORM with tagged sql\`` template literals (parameterized)
• Authentication enforced on all new/modified endpoints — request.user guard present in all three PATCH handlers (invoiceBudgetLines, workItemBudgets, householdItemBudgets)
• No sensitive data exposed in logs, errors, or client responses
• User input validated and sanitized at API boundaries — Fastify schema with additionalProperties: false, minProperties: 1 on the IBL PATCH
• No new dependencies introduced
• No hardcoded credentials or secrets
• Error responses do not leak internal details — NOT_FOUND used for both invoice-not-found and line-not-found (prevents enumeration of invoice IDs)
• CORS configuration unchanged

---

Tenant/Scope Safety — PASS

The application is single-tenant (no multi-tenancy, one user base per instance). Scope isolation is enforced by:

1. IBL route (invoiceBudgetLines.ts:144-155): verifies ibl.invoiceId === invoiceId before operating (line 627), giving ownership check on the source line.
2. WIB route (workItemBudgets.ts:148-159): verifies wib.workItemId === workItemId before operating (line 178), giving ownership check on the source budget line.
3. HIB route (householdItemBudgets.ts:151-162): verifies hib.householdItemId === householdItemId before operating (line in householdItemBudgetService.ts), giving ownership check on the source budget line.

All target entity existence checks are performed inside the transaction before the write operations. There is no cross-scope pivot path.

---

Authorization — PASS

All three modified PATCH handlers check if (!request.user) throw new UnauthorizedError() before calling any service function. The existing requireRole() preHandler is not applicable here because these endpoints are accessible to both Admin and Member roles, consistent with the existing auth pattern for budget routes.

---

Mutual Exclusion of Move Fields — PASS

Mutual exclusion of newWorkItemId / newHouseholdItemId is enforced at two independent layers:

• Schema layer: additionalProperties: false prevents unknown fields; both fields are optional strings, so no schema-level mutual exclusion (this is expected — schema can't express XOR constraints)
• Service layer: explicit ValidationError thrown in editAndMoveBudgetLine() (line 608-609), updateAndMoveWorkItemBudget() (line 143-145), and updateAndMoveHouseholdItemBudget() — defense-in-depth is correct

---

Transactional Atomicity — PASS

All cross-table and same-table moves are wrapped in db.transaction(). The better-sqlite3 driver is synchronous, so the transaction captures the complete sequence atomically. All guard checks (target-exists, BUDGET_LINE_ALREADY_LINKED) run inside the transaction, preventing TOCTOU races.

---

SQL Injection — PASS

All raw sql\`template expressions are parameterized. Example atinvoiceBudgetLineService.ts:722-724`:

sql`${invoiceBudgetLines.invoiceId} = ${invoiceId} AND ${workItemBudgets.workItemId} = ${targetId} AND ${invoiceBudgetLines.id} != ${lineId}`

All values are passed as Drizzle column references or bound parameters — no string interpolation.

---

Findings

[LOW] workItemBudgets / householdItemBudgets PATCH schemas: newWorkItemId / newHouseholdItemId allow null with no minLength on the string branch

Affected files:

• server/src/routes/workItemBudgets.ts:80-81 — { type: ['string', 'null'] } allows empty string ""
• server/src/routes/householdItemBudgets.ts:83-84 — same pattern

Description: The move fields accept null (to express "no move") but when a string is provided there is no minLength: 1 constraint. An empty string "" passes schema validation and flows into the service's !== null check, which evaluates to true (non-null string), causing the service to attempt a lookup for entity ID "". The service correctly throws NotFoundError in that case, so there is no exploit path — but the defence is at the wrong layer.

Remediation: Add minLength: 1 to the string branch of both fields, consistent with the pattern used elsewhere in the codebase:

"newWorkItemId": { "type": ["string", "null"], "minLength": 1 }

This finding matches the established pattern of informational/low items already in the security backlog (finding #25 in GitHub Issue #315).

---

[INFORMATIONAL] IBL PATCH schema: newWorkItemId / newHouseholdItemId allow null with no schema-enforced mutual exclusion

Affected file: server/src/routes/invoiceBudgetLines.ts:57-68

Description: JSON Schema cannot express XOR constraints, so both fields can be simultaneously non-null and pass the schema. The service correctly catches this at line 608 and returns a ValidationError. This is accepted design — documenting for completeness. No exploit path exists.

---

[INFORMATIONAL] workItemBudgets and householdItemBudgets PATCH schemas: missing minProperties: 1

Affected files:

• server/src/routes/workItemBudgets.ts:60-93 — no minProperties: 1 in the schema (was there before this PR? confirmed: pre-existing absence)
• server/src/routes/householdItemBudgets.ts:65-96 — same

Description: An empty {} PATCH body passes schema validation. The service performs a no-op update (setting only updatedAt). Not a security issue — no data loss or corruption. The IBL PATCH schema correctly has minProperties: 1 (line 68 in the diff). The WIB/HIB schemas are pre-existing and this PR did not introduce the gap, but it is worth noting given the WIB/HIB schemas were extended in this PR.

---

No blocking findings. This PR is clear to merge from a security perspective.

[steilerDev]

[product-architect]

Architecture review: Approve with medium findings for refinement. (Cannot self-approve via GitHub; treat this comment as an approval verdict.)

What I verified

• Transaction patterns: Service layer matches the spec. Same-table = single UPDATE; cross-table = atomic insert+repoint+delete inside db.transaction(). Mirrors the canonical assignToHouseholdItem pattern from budgetLineAssignService.ts. (server/src/services/invoiceBudgetLineService.ts:667-973)
• Mutual exclusion of move fields: Enforced at service layer (line 608) ahead of the transaction. Good — keeps the route schema simple and validation centralized.
• WI/HI cross-table rejection: Correctly raises ValidationError with a clear message (workItemBudgetService.ts:149-153, householdItemBudgetService.ts:153-157). The architect call to make IBL the canonical cross-table owner is sound — keeps the cross-table category mapping rule in one place.
• Category fallback rule (WI→HI): Implemented per spec — form value → existing WIB value → bc-household-items fallback (invoiceBudgetLineService.ts:825-828). HI→WI uses form value → existing HIB value (line 915-916), no hardcoded fallback, which is correct since WIB allows null budget_category_id.
• Subsidy linkage dropped on cross-table: Confirmed — neither work_item_budget_subsidies nor household_item_budget_subsidies is touched in the transaction. Matches the locked architect decision and the assignToHouseholdItem precedent.
• Shared types: EditAndMoveBudgetLineRequest is well-shaped and matches the route JSON schema (invoiceBudgetLines.ts:43-78). UpdateBudgetLineRequest correctly extended with the two move fields.
• API Contract wiki: wiki/API-Contract.md updated with the full PATCH body shape, new error code (BUDGET_LINE_ALREADY_LINKED), and atomic-transaction note. Submodule pointer correctly bumped in commit 0d51fe60.

Medium findings (refinement, non-blocking)

1. WI/HI PATCH wiki sections undocument the new fields. wiki/API-Contract.md lines 2406-2413 (WI) and 6846-6853 (HI) do not list newWorkItemId / newHouseholdItemId in the field tables, nor quantity / unit / unitPrice / includesVat (which the route schema accepts at server/src/routes/workItemBudgets.ts:75-82 and the equivalent HI route). The "Notes" section correctly says cross-table is rejected, but the same-table move affordance is not documented. Worth a follow-up wiki commit during epic refinement.

2. WI/HI same-table guard is stricter than the IBL service. server/src/services/workItemBudgetService.ts:204-212 and the equivalent HI guard (server/src/services/householdItemBudgetService.ts:210-220) check whether the target entity has any IBL linked (across all invoices). The IBL service's same-table guard at server/src/services/invoiceBudgetLineService.ts:718-731 scopes to the same invoice (invoiceId = X AND ... AND id != lineId). Per spec AC-10 ("a different IBL row on the same invoice"), the IBL service matches the spec; the WI/HI guard is broader. Two budget lines on the same WI but different invoices is a valid state today (e.g., one WIB linked to invoice A, another linked to invoice B). Recommendation: scope the WI/HI guard to the same invoice for consistency. If the intent is to deliberately keep WI/HI safer, document it in a comment.

3. Dead-ish code: updateInvoiceBudgetLine. server/src/services/invoiceBudgetLineService.ts:361-446 is no longer called from the route (replaced by editAndMoveBudgetLine at the PATCH handler). It's still exported and covered by tests, so coverage stays clean — but it's effectively dead production code. Either remove it (and prune its tests) or document why it's kept. Low priority, refinement item.

What I did not block on

• The transaction in editAndMoveBudgetLine is correctly structured (validation outside, mutations inside) and the result return value pattern works for fetching post-transaction state.
• Test coverage on the new code paths looks solid: 4 new service/route test files totaling ~1100 LOC, plus FE form tests and 802-LOC E2E spec.
• Cross-table category fallback to bc-household-items is the right call since HIBs always carry that category in the orphan-assignment path.

Verdict: approve — these findings can land in epic refinement.

[steilerDev]

[ux-designer]

PR #1554 — Full Edit + Parent Move for Budget Lines — Design Review

---

Token Adherence — PASS

All new CSS in BudgetLineForm.module.css uses design tokens exclusively. The latest fix commit (d4f12d4c) correctly removed the rgba() fallbacks on .parentPickerTabActive and .parentPickerError, leaving clean var(--token-name) references throughout. Verified token existence in tokens.css for every property used by the new rules:

Class	Token(s)	Status
.currentParentRow	--color-bg-primary, --color-border, --radius-md, --spacing-2/3, --font-size-sm, --color-text-primary	✓
.entityTypePill_work_item	--color-status-in-progress-bg/text	✓
.entityTypePill_household_item	--color-hi-status-scheduled-bg/text	✓
.ghostChangeButton	--color-primary, --color-primary-hover, --shadow-focus, --font-size-sm	✓
.ghostCancelButton	--color-border-strong, --color-text-secondary, --color-bg-tertiary, --radius-md, --shadow-focus, --transition-normal	✓
.moveHint	--color-warning-bg, --color-warning, --color-warning-text-on-light, --radius-md, --spacing-2/3	✓
.itemizedAmountField	--color-border, --spacing-4/2	✓
.requiredStar	--color-danger, --spacing-1	✓

Dark mode is handled automatically — all tokens listed above have correct [data-theme="dark"] overrides in tokens.css.

---

Visual Consistency — PASS

• currentParentRow: background: var(--color-bg-primary) with border: 1px solid var(--color-border) matches the section-card pattern used throughout the app (1px border, --color-bg-primary interior).
• entityTypePill: uses the correct WI/HI palette tokens and --radius-full for a pill shape as specced.
• ghostCancelButton: mirrors cancelButton structure (same padding, --color-bg-tertiary hover, --shadow-focus focus-visible) — visually coherent.
• moveHint: --color-warning-bg / --color-warning / --color-warning-text-on-light — matches the warning banner pattern established in #1545.
• itemizedAmountField: top-border visual separator is clean and consistent with existing form section separators.
• Shared component reuse: Modal, FormError, WorkItemPicker, HouseholdItemPicker all used correctly. No one-off parallel implementations introduced.

---

Responsive Behavior — PASS

• Touch targets: ghostChangeButton gets min-height: 44px at ≤ 1024px — spec-compliant.
• parentPickerTab gets min-height: 44px; flex: 1 at ≤ 1024px — spec-compliant.
• Mobile (≤ 639px): .parentPickerTabs already display: flex by default; the breakpoint rule is harmless redundancy but causes no visual regression.
• prefers-reduced-motion: all new transition-bearing classes (ghostChangeButton, ghostCancelButton, moveHint) correctly added to the existing prefers-reduced-motion: reduce block.

---

Findings

Non-blocking (fix before merge or in refinement)

1. aria-controls points to an element that does not exist in the DOM when the button is visible

BudgetLineForm.tsx lines 515–516:

aria-expanded={false}
aria-controls="parent-picker-body"

div#parent-picker-body (line 524) is only mounted when isPickerExpanded === true. The "Change" button is only mounted when isPickerExpanded === false. The two never coexist in the DOM, so the aria-controls reference is always dangling — screen readers cannot follow the relationship. The aria-expanded value is also hardcoded to false rather than computed.

Correct pattern: keep both the button and the controlled region in the DOM simultaneously, toggling visibility with CSS (display: none / conditional class), and updating aria-expanded dynamically:

// Keep button always mounted, let expanded state drive aria-expanded
<button
  aria-expanded={isPickerExpanded}
  aria-controls="parent-picker-body"
  onClick={() => setIsPickerExpanded(v => !v)}
>
  {isPickerExpanded ? t('budgetLineForm.cancelChangeParentButton') : t('budgetLineForm.changeParentButton')}
</button>

// Always mounted; CSS hides when collapsed
<div id="parent-picker-body" hidden={!isPickerExpanded}>
  {/* expanded content */}
</div>

This also eliminates the need for the separate ghostCancelButton in the expanded view.

Severity: Medium — aria-controls with a missing referent is a WCAG 4.1.2 (Name, Role, Value) violation but does not create a keyboard trap and the picker itself is keyboard-operable; the assistive technology impact is that users cannot programmatically navigate from the button to the controlled region.

---

2. Missing :focus-visible on .parentPickerTab and .modeBtn

BudgetLineForm.module.css lines 255–274 (.parentPickerTab) and lines 130–159 (.modeBtn) have no :focus-visible rule. These are keyboard-focusable buttons with no focus indicator in the component's own stylesheet. The browser default :focus ring may provide something, but the design system pattern (all other buttons in this file) is outline: none; box-shadow: var(--shadow-focus).

Note: .parentPickerTab is pre-existing, but the PR introduces new tab button rendering in the expanded move picker — it is a good moment to fix while the file is open. .modeBtn is also pre-existing.

Correct fix for both:

.parentPickerTab:focus-visible,
.modeBtn:focus-visible {
  outline: none;
  box-shadow: var(--shadow-focus);
}

Severity: Medium — WCAG 2.4.7 (Focus Visible), affects keyboard-only users. The browser default ring may partially compensate depending on the OS/browser combination, so this is not a complete focus-invisible failure — but it is inconsistent with every other interactive element in the file.

---

Informational

• The "Change" ghost button uses text-decoration: underline to signal interactivity. This is distinct from other ghost/link buttons in the codebase; if a future pass standardizes ghost-button affordances, this may warrant revisiting. Not a blocking issue.
• The unassigned-path BudgetLineForm invocation inside EditBudgetLineModal (lines ~1647–1680) still passes a mostly-empty form state (hardcoded pricingMode: 'direct', empty pricing fields) — this is pre-existing behavior for the unassigned path and outside this story's scope.

---

Summary

All new CSS is fully tokenized (the d4f12d4c fix resolved the only token compliance gap). Dark mode, responsive behavior, and shared component reuse all pass. Two medium accessibility findings require attention before or shortly after merge:

1. aria-controls referent not in DOM when button is visible — structural ARIA fix needed in BudgetLineForm.tsx.
2. Missing :focus-visible on .parentPickerTab and .modeBtn — two CSS rule additions in BudgetLineForm.module.css.

Neither finding blocks functional correctness or creates a keyboard trap. Approving with the expectation these are fixed in a follow-up commit on this branch or a targeted refinement.

[steilerDev]

[ux-designer]

PR #1554 — Full Edit + Parent Move for Budget Lines — Design Review

---

Token Adherence — PASS

All new CSS in BudgetLineForm.module.css uses design tokens exclusively. The latest fix commit (d4f12d4c) correctly removed the rgba() fallbacks on .parentPickerTabActive and .parentPickerError, leaving clean var(--token-name) references throughout. Verified token existence in tokens.css for every property used by the new rules:

Class	Token(s)	Status
.currentParentRow	--color-bg-primary, --color-border, --radius-md, --spacing-2/3, --font-size-sm, --color-text-primary	✓
.entityTypePill_work_item	--color-status-in-progress-bg/text	✓
.entityTypePill_household_item	--color-hi-status-scheduled-bg/text	✓
.ghostChangeButton	--color-primary, --color-primary-hover, --shadow-focus, --font-size-sm	✓
.ghostCancelButton	--color-border-strong, --color-text-secondary, --color-bg-tertiary, --radius-md, --shadow-focus, --transition-normal	✓
.moveHint	--color-warning-bg, --color-warning, --color-warning-text-on-light, --radius-md, --spacing-2/3	✓
.itemizedAmountField	--color-border, --spacing-4/2	✓
.requiredStar	--color-danger, --spacing-1	✓

Dark mode is handled automatically — all tokens listed above have correct [data-theme="dark"] overrides in tokens.css.

---

Visual Consistency — PASS

• currentParentRow: background: var(--color-bg-primary) with border: 1px solid var(--color-border) matches the section-card pattern used throughout the app (1px border, --color-bg-primary interior).
• entityTypePill: uses the correct WI/HI palette tokens and --radius-full for a pill shape as specced.
• ghostCancelButton: mirrors cancelButton structure (same padding, --color-bg-tertiary hover, --shadow-focus focus-visible) — visually coherent.
• moveHint: --color-warning-bg / --color-warning / --color-warning-text-on-light — matches the warning banner pattern established in #1545.
• itemizedAmountField: top-border visual separator is clean and consistent with existing form section separators.
• Shared component reuse: Modal, FormError, WorkItemPicker, HouseholdItemPicker all used correctly. No one-off parallel implementations introduced.

---

Responsive Behavior — PASS

• Touch targets: ghostChangeButton gets min-height: 44px at ≤ 1024px — spec-compliant.
• parentPickerTab gets min-height: 44px; flex: 1 at ≤ 1024px — spec-compliant.
• Mobile (≤ 639px): .parentPickerTabs already display: flex by default; the breakpoint rule is harmless redundancy but causes no visual regression.
• prefers-reduced-motion: all new transition-bearing classes (ghostChangeButton, ghostCancelButton, moveHint) correctly added to the existing prefers-reduced-motion: reduce block.

---

Findings

Non-blocking (fix before merge or in refinement)

1. aria-controls points to an element that does not exist in the DOM when the button is visible

BudgetLineForm.tsx lines 515–516:

aria-expanded={false}
aria-controls="parent-picker-body"

div#parent-picker-body (line 524) is only mounted when isPickerExpanded === true. The "Change" button is only mounted when isPickerExpanded === false. The two never coexist in the DOM, so the aria-controls reference is always dangling — screen readers cannot follow the relationship. The aria-expanded value is also hardcoded to false rather than computed.

Correct pattern: keep both the button and the controlled region in the DOM simultaneously, toggling visibility with CSS (display: none / conditional class), and updating aria-expanded dynamically:

// Keep button always mounted, let expanded state drive aria-expanded
<button
  aria-expanded={isPickerExpanded}
  aria-controls="parent-picker-body"
  onClick={() => setIsPickerExpanded(v => !v)}
>
  {isPickerExpanded ? t('budgetLineForm.cancelChangeParentButton') : t('budgetLineForm.changeParentButton')}
</button>

// Always mounted; CSS hides when collapsed
<div id="parent-picker-body" hidden={!isPickerExpanded}>
  {/* expanded content */}
</div>

This also eliminates the need for the separate ghostCancelButton in the expanded view.

Severity: Medium — aria-controls with a missing referent is a WCAG 4.1.2 (Name, Role, Value) violation but does not create a keyboard trap and the picker itself is keyboard-operable; the assistive technology impact is that users cannot programmatically navigate from the button to the controlled region.

---

2. Missing :focus-visible on .parentPickerTab and .modeBtn

BudgetLineForm.module.css lines 255–274 (.parentPickerTab) and lines 130–159 (.modeBtn) have no :focus-visible rule. These are keyboard-focusable buttons with no focus indicator in the component's own stylesheet. The browser default :focus ring may provide something, but the design system pattern (all other buttons in this file) is outline: none; box-shadow: var(--shadow-focus).

Note: .parentPickerTab is pre-existing, but the PR introduces new tab button rendering in the expanded move picker — it is a good moment to fix while the file is open. .modeBtn is also pre-existing.

Correct fix for both:

.parentPickerTab:focus-visible,
.modeBtn:focus-visible {
  outline: none;
  box-shadow: var(--shadow-focus);
}

Severity: Medium — WCAG 2.4.7 (Focus Visible), affects keyboard-only users. The browser default ring may partially compensate depending on the OS/browser combination, so this is not a complete focus-invisible failure — but it is inconsistent with every other interactive element in the file.

---

Informational

• The "Change" ghost button uses text-decoration: underline to signal interactivity. This is distinct from other ghost/link buttons in the codebase; if a future pass standardizes ghost-button affordances, this may warrant revisiting. Not a blocking issue.
• The unassigned-path BudgetLineForm invocation inside EditBudgetLineModal (lines ~1647–1680) still passes a mostly-empty form state (hardcoded pricingMode: 'direct', empty pricing fields) — this is pre-existing behavior for the unassigned path and outside this story's scope.

---

Summary

All new CSS is fully tokenized (the d4f12d4c fix resolved the only token compliance gap). Dark mode, responsive behavior, and shared component reuse all pass. Two medium accessibility findings require attention before or shortly after merge:

1. aria-controls referent not in DOM when button is visible — structural ARIA fix needed in BudgetLineForm.tsx.
2. Missing :focus-visible on .parentPickerTab and .modeBtn — two CSS rule additions in BudgetLineForm.module.css.

Neither finding blocks functional correctness or creates a keyboard trap. Approving with the expectation these are fixed in a follow-up commit on this branch or a targeted refinement.

[steilerDev]

[product-owner] Review of PR #1554 against issue #1553 acceptance criteria.

Verdict: REQUEST CHANGES

Solid work overall — backend service is well-structured, transactional, and all four scenarios (in-place, same-table, cross-table either direction) are covered. i18n keys translated in lock-step, test authorship correct (qa-integration-tester / e2e-test-engineer), :focus-visible indicators present. However, there is a critical silent-data-loss bug in the invoice-side edit flow that blocks approval.

---

Blocking — must fix

AC-2 — Critical data loss on save (FAIL)

The accepted deviation in the PR scope was that budgetSourceId and vendorId would not be pre-populated. But the implementation also fails to pre-populate pricingMode, quantity, unit, unitPrice, and includesVat, and crucially the save handler unconditionally sends those (empty) form values back to the server. The result is silent data loss for any non-trivial budget line.

Reproduction (no user changes required):

1. Create a budget line with pricingMode=unit, quantity=10, unit='m³', unitPrice=50, includesVat=false, link to invoice.
2. From the invoice detail page, click Actions → Edit on that line.
3. Modal opens. Form shows pricingMode=direct, empty qty/unit/unitPrice, includesVat=true (checked), no vendor/source selected.
4. Click Save without changing anything.
5. Underlying WIB row is now quantity=null, unit=null, unitPrice=null, includesVat=true, budgetSourceId=null, vendorId=null.

Root causes (two layers):

1. client/src/pages/InvoiceDetailPage/InvoiceBudgetLinesSection.tsx lines 309–322 (openEditBudgetLineModal) hard-codes pricingMode: 'direct', empty quantity/unit/unitPrice, includesVat: true, empty budgetSourceId/vendorId regardless of the line's existing values. AC-2 explicitly requires the "same rule used by useBudgetSection.toFormState" for pricing-mode detection.
2. handleBudgetLineFullEditSubmit (lines 715–780) and handleMoveBudgetLine (lines 785–835) unconditionally include quantity, unit, unitPrice, includesVat, budgetSourceId, vendorId in the PATCH payload — so the wiped form state is written back to the database.
3. Underlying structural gap: InvoiceBudgetLineDetailResponse (shared/src/types/invoiceBudgetLine.ts:95) does not surface quantity, unit, unitPrice, includesVat, budgetSourceId, or vendorId from the WIB/HIB row. The detail resolver in server/src/services/invoiceBudgetLineService.ts:73–179 would need to include these fields before the client can pre-populate them. Once surfaced, applying useBudgetSection.toFormState (or equivalent) in openEditBudgetLineModal resolves the form-init half, and the submit handler can either send only changed fields or send the full pre-populated state.

Test coverage gap: existing unit/integration/E2E tests all use fixture lines with quantity: null, unit: null, unitPrice: null, includesVat: true, budgetSourceId: null, vendorId: null — i.e., they cannot catch this regression. A regression test that creates a unit-priced line with a vendor + non-default source and verifies all fields are preserved after a no-op edit save is required.

This is not a formatting/polish issue — it's a data-integrity defect that affects real-world budget lines (any line created in qty × unit-price mode, any line with a vendor, any line not on the default discretionary source). Recommend backend extends the detail response, then client pre-populates the full form per AC-2, and submit handler either sends only changed fields or relies on a fully pre-populated form.

---

AC verdict summary

Invoice-side full edit (AC-1 to AC-4)

• AC-1 PASS — EditBudgetLineModal for assigned lines renders the full BudgetLineForm with itemized amount + linked-item parent picker.
• AC-2 FAIL — description / plannedAmount / confidence / budgetCategoryId / itemizedAmount / current-parent pre-populated correctly. budgetSourceId and vendorId not pre-populated (accepted deviation). pricingMode, quantity, unit, unitPrice, includesVat also not pre-populated — see "Blocking" above.
• AC-3 PASS — toggling work-item / household-item tabs clears the picker selection and re-renders the appropriate picker.
• AC-4 PASS — single Save submission with client-side validation (negative qty/price disabled, blank planned amount disables submit, itemized amount ≤ 0 short-circuits before server call).

Move semantics (AC-5 to AC-9)

• AC-5 PASS — same-table move uses single UPDATE with new parent FK; IBL FK unchanged.
• AC-6 PASS — cross-table move is wrapped in db.transaction(() => { insert; repoint; delete; }); atomic.
• AC-7 PASS — new row copies all editable fields. Category mapping follows architect decision (form value → existing → bc-household-items fallback). Invoice link points to correct detail page after move.
• AC-8 CONDITIONAL — non-parent field updates do a single UPDATE per the in-place branch, and itemizedAmount is only updated if provided. But because of the AC-2 bug, the "non-parent field update" path is silently sending wiped pricing/vendor/source data along with the legitimate field changes — same fix unblocks this AC.
• AC-9 PASS — all save operations wrapped in a single db.transaction(). remainingAmount recalculated after save.

Server-side guards (AC-10 to AC-13)

• AC-10 PASS — BUDGET_LINE_ALREADY_LINKED thrown when target parent already has a different IBL for the same invoice (same-table and cross-table both checked).
• AC-11 PASS — ITEMIZED_SUM_EXCEEDS_INVOICE thrown when the new itemized total exceeds invoice amount.
• AC-12 PASS — NotFoundError thrown for missing target work item or household item.
• AC-13 PASS — server ValidationError on itemizedAmount <= 0; client form has required + min=0 + disables Save when itemized amount is empty.

WI/HI inline-edit parity (AC-14 to AC-16)

• AC-14 PASS — WorkItemDetailPage.handleMoveBudgetLine is wired through BudgetSection.onMoveBudgetLine; same-table moves go via updateWorkItemBudget with newWorkItemId; cross-table with invoice goes via editAndMoveBudgetLine.
• AC-15 PASS — same wiring on HouseholdItemDetailPage.
• AC-16 ACCEPTED MITIGATION — cross-table moves on a line with no invoice link throw the translated budgetLineForm.moveCrossTableNoInvoiceError. This is the documented architect decision in the PR description and is the user-informed-error outcome the AC calls for. Acceptable.

Validation, errors, a11y, i18n (AC-17 to AC-20)

• AC-17 PASS — ApiClientError mapped via translateApiError; generic fallback uses translated editError.saveFailed. FormError banner used in form.
• AC-18 PASS — 8 new keys + moveCrossTableNoInvoiceError in both en and de; no hardcoded German.
• AC-19 PASS — :focus-visible shadows on all new interactive elements (ghostChangeButton, ghostCancelButton, picker tabs), aria-expanded/aria-controls on Change button, Modal handles Escape.
• AC-20 PASS — currency labels use {{currencySymbol}} interpolation, computed total uses toFixed(2) (matches existing form). No raw Intl.NumberFormat or toLocaleDateString in new code.

---

Required fixes before re-review

1. Extend InvoiceBudgetLineDetailResponse and resolveDetail to surface quantity, unit, unitPrice, includesVat, budgetSourceId, vendorId (and budgetSource / vendor summaries if helpful for the form's select pre-population).
2. Update openEditBudgetLineModal to compute pricingMode per useBudgetSection.toFormState rules and to pre-populate quantity / unit / unitPrice / includesVat / budgetSourceId / vendorId from the line.
3. Either keep the submit-everything pattern (now safe because form is fully pre-populated) or switch to a dirty-field diff and only PATCH changed fields. Either is acceptable.
4. Add a qa-integration-tester regression test: create a budget line with pricingMode=unit, quantity=10, unit='m³', unitPrice=50, includesVat=false, budgetSourceId=<non-default>, vendorId=<set>; open invoice edit modal; click Save without changes; assert the underlying WIB row's fields are byte-for-byte unchanged. Mirror in an E2E scenario.

After those land, this is approvable.

[steilerDev]

[product-owner] Re-review after commit 4e0a6ced.

Verdict: APPROVE (functional ACs)

The critical data-loss bug flagged in my previous review at AC-2 / AC-8 is fully addressed.

Verification

AC-2 (form pre-populated with all underlying fields) — PASS

shared/src/types/invoiceBudgetLine.ts:113-119 now exposes quantity, unit, unitPrice, includesVat, vendorId, budgetSourceId on InvoiceBudgetLineDetailResponse.

server/src/services/invoiceBudgetLineService.ts:91-96, 111-116, 146-151, 195-200 — resolveDetail() reads these from the underlying WIB/HIB row and returns them in the response. Both branches (work_item, household_item) are covered.

client/src/pages/InvoiceDetailPage/InvoiceBudgetLinesSection.tsx:309-326 — openEditBudgetLineModal initializes the full form state with all 11 keys:

• description, plannedAmount, confidence, budgetCategoryId (existing)
• budgetSourceId, vendorId (newly wired)
• pricingMode derived correctly: quantity !== null && unitPrice !== null ? 'unit' : 'direct'
• quantity, unit, unitPrice, includesVat (newly wired, with ?? true fallback for the boolean)
• itemizedAmount set separately

AC-8 (no row insertion/deletion on in-place edit; non-parent fields preserved) — PASS

Round-trip analysis: form loads with underlying values → user saves without changes → submit payload at InvoiceBudgetLinesSection.tsx:742-760 sends back the same values → editAndMoveBudgetLine service (with no newWorkItemId/newHouseholdItemId) applies an UPDATE to the existing WIB/HIB row. No insert/delete, no field clobbering.

CI status

Quality Gates currently running on the latest commit. Approval is contingent on green CI; recommend the orchestrator wait for Quality Gates: pass before merging.

Non-blocking observations (already tracked by security-engineer)

1. Move fields newWorkItemId / newHouseholdItemId accept empty string — service throws NotFoundError as backstop. Add minLength: 1 in a follow-up.
2. WIB/HIB PATCH schemas missing minProperties: 1 — empty body causes a no-op updatedAt touch.

Neither blocks Story #1553 acceptance.

Story acceptance: APPROVED pending CI.

[github-actions]

🎉 This PR is included in version 2.7.0-beta.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀