Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .claude/agent-memory/qa-integration-tester/MEMORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

## Recent bug/story notes (2026-08)

- [Story #1923 — report table cleanup](story-1923-report-table-cleanup.md) (2026-08-02) — unnumbered shared †/‡ markers, isDeposit/isClaim/areaText fixture ripple across 8 report test files, overviewPdf allocated-cell-is-always-an-array-of-runs gotcha, worktree `node_modules/@cornerstone/shared` symlink pointing at a differently-branched base repo (false-positive `tsc` errors; trust jest).
- [Bugs #1895/#1896/#1918 — claim/deposit scope fixes](bugs-1895-1896-1918-claim-deposit-scope.md) (2026-08-01) — `markInvoicesClaimed` gained `sourceId`+required `depositIds` params (cross-source claim guard, decoupled sweep, quotation+sweepable-deposit no longer 409s); `getSourceReport` drops zero-portion `budgetLines[]` on `claim` reports only; text-content query collision gotcha (banner text contains data also shown elsewhere on page).

## Recent bug/story notes (2026-07)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
---
name: story-1923-report-table-cleanup
description: QA test coverage for issue #1923 (shared footnotes, inline deposit labels, claim metadata, total-only summary, area in Usage) — unnumbered marker semantics, isDeposit/isClaim/areaText fixture ripple across 8 report test files, and a worktree symlink false-positive gotcha.
metadata:
type: project
---

Story/issue #1923 (branch `feat/1923-report-table-cleanup`, worktree `develop-claim-semantics`).
Production code (buildReportContent.ts, ReportContentEditor.tsx, overviewPdf.ts,
sourceReportService.ts, shared/src/types/sourceReport.ts) was already implemented when QA started —
only test files needed writing/updating. All 8 touched files pass (302 tests total), 100%
statement/line coverage on buildReportContent.ts and sourceReportService.ts, 100% on
ReportContentEditor.tsx, ~100%/98% branch on overviewPdf.ts.

**New/changed behavior to remember for future report-content work:**
- Split (`†`) and reduced-deposit (`‡`) markers are now UNNUMBERED and SHARED — at most one
footnote entry each (`id: 'split'`/`'deposit-reduced'`, no vendor/invoice prefix). A constituted
(tagged-to-this-source) deposit produces `isDeposit: true` on the row instead of a `‡` marker or
footnote — no footnote entry at all for that case.
- `summaryRows` is always exactly 1 entry (`key: 'total'`) — no more per-status subtotal rows.
- `ReportContent` gained `isClaim: boolean` (gates the source-info block/stack in both the editor
and the PDF). `ReportContentRow` gained `isDeposit: boolean` and `areaText: string | null`.
- **overviewPdf.ts's allocated-amount cell `.text` is now ALWAYS an array of runs** (`allocatedRuns`),
even with zero deposit/refund — not conditionally a plain string. Any test using a `rowTexts()`-
style helper that does `cell.text` directly must handle both string and array-of-`{text}` shapes
(join array runs) or every existing "plain amount" assertion silently breaks (`toBe('€400.00')`
vs received `[{text:'€400.00'}]`). Fixed by making the shared `rowTexts()` helper flatten arrays.
- `SourceReportLinkedItem` gained `areaId`/`areaName` (nullable), resolved server-side via
`LEFT JOIN areas` on `work_items.area_id`/`household_items.area_id` — leaf-only area name, no
parent-path expansion (verified with an explicit child-area-with-parent test).
- `linkedItem` stays `null` when the join partially resolves but the item's own title/name is falsy
(empty string) — a real, defensible edge case reachable via `row.work_item_id && row.work_item_title`
guard in the service; used this to cover "linkedItem null unaffected" rather than trying to force
an FK-violating null-linkedItem state (not practically reachable given the schema's constraints).

**Worktree gotcha reconfirmed and clarified**: raw `npx tsc --noEmit -p client/tsconfig.json` (or
server/tsconfig.json) in this worktree resolves `@cornerstone/shared` via `node_modules/@cornerstone/shared`,
which is a symlink to the **base repo checkout** (`/Users/.../cornerstone/shared`), not the worktree's
own `shared/`. If the base repo happens to be checked out on a *different, unrelated branch* (observed:
base was on `fix/1895-1918-claim-deposit-scope` while this worktree was on `feat/1923-...`), that stale
symlinked `dist/index.d.ts` produces convincing but FALSE-POSITIVE type errors (e.g. "Property 'areaId'
does not exist on type 'SourceReportLinkedItem'") for types the current branch's `shared/src` genuinely
already has. **Jest itself did not reproduce these errors and all tests passed with correct runtime
values** for the files I'd already fixed — client project's jest config has `moduleNameMapper:
'^@cornerstone/shared$' -> '<rootDir>/shared/src/index.ts'` (worktree source, always fresh); the
**server** project has no such mapper, yet still passed cleanly too (mechanism unclear).

**Definitive fix applied this round** (repoint the worktree's own node_modules symlink instead of
relying on jest's leniency, so raw `tsc -p` becomes trustworthy again for this session):
```bash
rm node_modules/@cornerstone/shared
ln -s /absolute/path/to/THIS/worktree/shared node_modules/@cornerstone/shared
cd shared && npx tsc && cd .. # rebuild worktree-local shared/dist
```
After this, `npx tsc --noEmit -p client/tsconfig.json` and `-p server/tsconfig.json` both went from
~15-30 false-positive errors to 0, and stayed 0 after all real fixes. **Use this fix proactively at the
start of any session that needs a real `tsc -p` sanity sweep** (e.g. when a coordinator reports a CI
typecheck failure) rather than trusting jest's silence alone — jest's leniency masks real errors too
easily to be the sole signal when hunting for "any other fixture drift somewhere in the tree" (this is
exactly how the ReportInvoiceList.test.tsx / realRender.test.ts CI failures escaped my first local pass).
Do NOT touch the base checkout itself — this only repoints the worktree's own node_modules entry.

**Real bug this fix uncovered** (CI Quality Gates failure on PR #1924, reported by coordinator):
`ReportInvoiceList.test.tsx` (~L727/733) and `realRender.test.ts` (~L447/453) built `SourceReportLinkedItem`
literals missing `areaId`/`areaName` — straightforward fixture-drift fixes (add the two null fields).
But `realRender.test.ts` also had a **second, deeper bug**: a real-i18n end-to-end test
(`renders both real deposit-footnote wordings ("constituted" vs "reduced")...`) still asserted the
OLD numbered/vendor-prefixed constituted-deposit footnote text (`'‡1: Constituted Vendor (U-5) — This
is a deposit.'`), which the AC2.1/AC2.2 change removed entirely (constituted deposits now render as an
inline `isDeposit` badge/run, not a footnote). Rewrote the test to assert real translated text for both:
the inline deposit-label run (array-of-runs allocated cell, second run text ` (Deposit)`/`
(Abschlagszahlung)`) AND the still-existing shared/unnumbered reduced-deposit footnote — plus explicit
negative assertions that the old "This is a deposit."/"Dies ist eine Abschlagszahlung." footnote text
no longer appears anywhere. Lesson: a `grep`-based sweep for the type-shape drift (missing fields) is
necessary but not sufficient — real-render/integration tests asserting exact translated STRINGS for a
feature whose wording changed need their own pass, since `tsc` won't catch stale string assertions.

**Round 3** (coordinator follow-up on PR #1924): the Deposit badge label moved further — out of
per-consumer `t()` calls entirely and into the shared content model as `ReportContentLabels.deposit`
(built once in `buildReportContent.ts` via `reportT`; `ReportContentEditor.tsx` and `overviewPdf.ts`
both just read `content.labels.deposit` / `reportContent.labels.deposit` now — `overviewPdf.ts` also
switched to named color/fontSize constants `DEPOSIT_NOTE_TEXT_COLOR`/`DEPOSIT_NOTE_FONT_SIZE` from
`reportPdf/shared.ts` instead of inline magic values). By the time this request landed, an external
process (not me) had already patched most existing fixtures' `labels`/`makeLabels()` objects to include
`deposit: 'REPORT_DEPOSIT_LABEL'`-style values — but NOT the 4 files I'd fixed in round 2
(`applyAiContent.test.ts`, `applyOverrides.test.ts`, `coverLetterPdf.test.ts`, `merge.test.ts`), which
still lacked the `deposit` key on their `ReportContentLabels` fixtures and failed `tsc` (`TS2741:
Property 'deposit' is missing`) once I re-swept. **Lesson: when a shared type gains a new required
field mid-story, re-run the `tsc -p client/tsconfig.json` sweep after EVERY round, even on files you
"already fixed" in a prior round for a different reason** — the type can grow again between rounds
without any signal other than a fresh typecheck.

For the mixed-language regression itself: added to `realRender.test.ts`'s existing
`describe('production i18n singleton — getFixedT resolves a language independent of the ambient one')`
block (established pattern for "UI locale stays X while report language resolves Y" — uses the REAL
app i18n singleton via `(await import('../../i18n/index.js')).default` + `i18n.getFixedT(lang, 'budget')`,
not the file's separate isolated `i18next.createInstance()` used everywhere else in that file). Gotcha:
helper functions declared with `function` inside a nested `describe(...)` callback (e.g.
`makeUsageFeatureReport()` inside the `'Usage column...'` block) are scoped to that closure only —
NOT visible from a sibling top-level `describe` block later in the same file. Had to inline a minimal
one-invoice fixture using the file's top-level `makeInvoice()` helper instead of reaching into the
nested one. Final test: builds `content` via `reportT = i18n.getFixedT('de', 'budget')` while asserting
`i18n.language` stays `'en'` throughout (both before and after — proves `getFixedT` never calls
`changeLanguage()`), asserts `content.labels.deposit === 'Abschlagszahlung'` (exact real string, not
`.toContain`), contrasts with `getFixedT('en', ...)` → `'Deposit'`, and pins the same value through to
the rendered PDF's inline deposit run.
10 changes: 10 additions & 0 deletions .claude/agent-memory/ux-designer/feature-spec-history.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ Adds an opt-in "Enable AI assistance" toggle (Step 4) + "Generate with AI" batch
- **Table/mobile-card breakpoint**: reused `ReportInvoiceList`'s existing `max-width: 767px` split verbatim rather than the page's own ad hoc `860px` breakpoint (`.step4Layout` collapse) — the two breakpoints coexist in this file for different purposes (860px = two-column layout collapse, 767px = table→cards), don't conflate them.
- Full field inventory for the cover letter (from `coverLetterPdf.ts`): sender (household name+address), recipient (`source.contactAddress`), reference (`source.reference`, optional), subject (per-use-case string), body (per-use-case template with `{{total}}`). A signature block also exists in the generated PDF (echoes household name a second time) but isn't in the issue's "settled decisions" list of 5 editable fields — spec'd it as derived-display-only (mirrors Sender), flagged as an open question rather than deciding unilaterally.

## Issue #1923 — Report table cleanup: shared footnotes, inline deposit labels, claim metadata, total-only summary, area in Usage

Client-only content-model cleanup on `ReportContentEditor.tsx`/`overviewPdf.ts` (mostly de-numbering footnotes + moving 2 facts from footnote-only to inline). Spec posted covering all 5 ACs.

- **"Deposit" inline label reuses `Badge`'s existing `.attachmentDeposit` variant** (`--color-attachment-deposit-bg`/`-text`, teal) rather than inventing a new badge color — the exact same wording (`sourceReports.table.attachmentType.deposit`) already exists as a Document-Type badge elsewhere in the same reports feature (`ReportInvoiceList.tsx`). When a new inline "this row is an X" label is needed, check `attachmentType`-style badges first — this app already has a full palette of document/entry-type pills before reaching for a new variant.
- **PDF "no Badge" fallback pattern**: pdfmake has no pill primitive, so a Badge-equivalent becomes a bracketed plain-text suffix `(Deposit)` in the *same* translation key, rendered as a separate lower-weight text run (`{ text: '...', color: '#6b7280', fontSize: 8 }`) via pdfmake's array-of-runs `text` field — not a new stacked line, not a filled/colored cell (per-run `fillColor` doesn't compose reliably with a whole-cell `text` array in pdfmake tables). `#6b7280` is the PDF-baked literal equivalent of `--color-text-muted`/`--color-gray-500` (confirmed via `merge.ts`'s hardcoded `styles.small.color`); PDF exports have no dark mode by design (fixed light-background documents), so hardcoded hex is expected/correct there, unlike in `client/src/**/*.css` where it would be a stylelint violation.
- **Conditional block removal — no placeholder, just omit from the render tree**: for "this metadata block doesn't apply to this report type" (claim reports skip `sourceInfoBlock`), the correct spec is `{condition && <div>...}` (full omission), relying on the parent's existing `display:flex; gap: var(--spacing-N)` to naturally close the space — never a `display:none`-but-present placeholder or a manually tightened margin override. Same principle applies to the pdfmake side: skip the whole `content.push(...)` call rather than pushing an empty/near-empty stack.
- **Secondary/muted metadata line under an editable field**: reused the `.dateLineLabel`/`.footnotes` muted-xs-text convention (`--font-size-xs` + `--color-text-muted`) for the new "area name" sub-line under Usage — this is the established in-file precedent for "annotation, not content" text, not a new pattern. Placed *below* the `EditableField`, never inline/parenthetical beside it, specifically because AC required it be visually distinguishable as non-editable — inline-beside-an-input reads as part of the same string.
- **PDF stack-building refactor flagged, not just a style note**: extending `overviewPdf.ts`'s Usage cell from a 2-way ternary (`attachmentsNote ? stack : text`) to a 3-optional-line array build (usage + area + attachmentsNote) is a real code-shape change for `frontend-developer`, called out explicitly in the spec as an implementation note so it isn't missed as "just add one more line."

## Issue #1876 — Deposit Refunds with Negative Claim Adjustments

`InvoiceDepositsSection` gains an entry-type choice (Deposit/Refund); refunds render as negative rows reusing the exact same status Badge/labels (Pending/Paid/Claimed) — no relabeling, per explicit user decision.
Expand Down
4 changes: 4 additions & 0 deletions .claude/agent-memory/ux-designer/pr-review-findings.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ metadata:
- Legend dot `8px` = `var(--spacing-2)` — always swap raw px dot sizes to nearest spacing token
- `--color-border-strong` as text `color` for a separator — use `--color-text-muted` instead

## PR #1924 — Report table cleanup (#1923) (APPROVED via `gh pr comment`)

Clean match to spec: `.depositBadge` composed `attachmentDeposit` verbatim, mobile Badge correctly omitted `.depositLabel` (relies on `.mobileCardAllocated` flex-gap instead), `.usageAreaText` rendered as separate div/span never concatenated into the editable value, summary border-removal + `.summaryAmount`-only size bump both matched exactly, PDF deposit-suffix run and usage-stack area line matched fontSize/color/ordering. No findings. Verdict was posted as a `gh pr comment` with an explicit "Verdict: APPROVED" line.

## PR #1490 — Measurement & Freehand Tools (APPROVED/comment)

See `pr-1490-measurement-freehand.md`. Medium: `labelAttrs { display:'none' }` dead code in render.ts — refinement item.
Expand Down
27 changes: 27 additions & 0 deletions client/src/components/reports/ReportContentEditor.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,28 @@
color: var(--color-status-quotation-text);
}

/* Deposit badge and area text */
.depositBadge {
composes: attachmentDeposit from '../Badge/Badge.module.css';
}

.depositLabel {
margin-left: var(--spacing-2);
}

.mobileCardAllocated {
display: flex;
align-items: center;
gap: var(--spacing-2);
flex-wrap: wrap;
}

.usageAreaText {
font-size: var(--font-size-xs);
color: var(--color-text-muted);
margin-top: var(--spacing-1);
}

/* Summary Table */
.summaryTable {
width: 100%;
Expand All @@ -167,6 +189,10 @@
border-bottom: 1px solid var(--color-border);
}

.summaryTable tbody tr:last-child td {
border-bottom: none;
}

.summaryLabel {
font-weight: var(--font-weight-semibold);
color: var(--color-text-primary);
Expand All @@ -175,6 +201,7 @@
.summaryAmount {
font-weight: var(--font-weight-semibold);
color: var(--color-text-primary);
font-size: var(--font-size-base);
}

/* Footnotes */
Expand Down
Loading
Loading