From 1b6f7157669660f5eb53be730a9381d3aa4e29f7 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Tue, 7 Jul 2026 23:23:16 +0200 Subject: [PATCH 1/2] refactor(client): consolidate Badge/Modal reuse, add shared hooks, trap modal focus - KeyboardShortcutsHelp now renders through the shared Modal (was a hand-rolled dialog with no portal, Escape handling, or focus management) - Modal gains a Tab-cycling focus trap; PhotoMetadataModal's private trap implementation is removed in favor of the shared one - BudgetHealthIndicator, UpcomingMilestonesCard, CriticalPathCard, and SubsidyPipelineCard now use the shared Badge component with new Badge.module.css variants instead of hand-rolled status spans - Add useDebounce, useDebouncedCallback, and useClickOutside hooks, and migrate 8 duplicated debounce/click-outside implementations onto them (SearchPicker, DocumentBrowser, DiaryPage, BudgetOverviewPage, DiaryEntryEditPage, DataTableColumnSettings, OverflowMenu) - Remove stale StatusBadge/HouseholdItemStatusBadge directories (components were already consolidated into Badge; only orphaned test files remained) - Wire the existing (already-translated) i18n key for WorkItemDetailPage's subtasks empty state instead of hardcoded English Deferred to follow-up issues (documented in the PR description): the 25+ ad-hoc FormError/EmptyState bypass sites cited in #1816, DataTableHeader's click-outside (needs a ref-forwarding change to a sibling component), and a pre-existing autosave-on-mount bug in DiaryEntryEditPage found by QA while testing this refactor (unrelated to the debounce-hook migration). Fixes #1816 Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) Co-Authored-By: Claude frontend-developer (Haiku 4.5) Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) --- .../qa-integration-tester/MEMORY.md | 1 + .../issue-1816-component-reuse-hooks.md | 71 +++++++ .../ux-designer/photo-annotator-patterns.md | 4 +- .../ux-designer/pr-review-findings.md | 18 ++ .../ux-designer/token-reference.md | 15 ++ client/src/components/Badge/Badge.module.css | 53 ++++++ .../BudgetHealthIndicator.module.css | 25 --- .../BudgetHealthIndicator.test.tsx | 18 +- .../BudgetHealthIndicator.tsx | 57 +++--- .../DataTable/DataTableColumnSettings.tsx | 17 +- .../HouseholdItemStatusBadge.test.tsx | 92 --------- .../KeyboardShortcutsHelp.module.css | 107 +---------- .../KeyboardShortcutsHelp.test.tsx | 79 ++++++-- .../KeyboardShortcutsHelp.tsx | 59 +++--- client/src/components/Modal/Modal.test.tsx | 118 ++++++++++++ client/src/components/Modal/Modal.tsx | 37 +++- .../components/OverflowMenu/OverflowMenu.tsx | 17 +- .../SearchPicker/SearchPicker.test.tsx | 31 ++- .../components/SearchPicker/SearchPicker.tsx | 44 +---- .../StatusBadge/StatusBadge.test.tsx | 90 --------- .../SubsidyPipelineCard.module.css | 31 --- .../SubsidyPipelineCard.tsx | 53 +++--- .../TimelineStatusCards/CriticalPathCard.tsx | 53 ++++-- .../TimelineStatusCards.module.css | 27 --- .../UpcomingMilestonesCard.tsx | 31 ++- .../components/documents/DocumentBrowser.tsx | 21 +- .../photos/PhotoMetadataModal.test.tsx | 73 ------- .../components/photos/PhotoMetadataModal.tsx | 30 --- client/src/hooks/useClickOutside.test.ts | 164 ++++++++++++++++ client/src/hooks/useClickOutside.ts | 38 ++++ client/src/hooks/useDebounce.test.ts | 126 ++++++++++++ client/src/hooks/useDebounce.ts | 15 ++ client/src/hooks/useDebouncedCallback.test.ts | 179 ++++++++++++++++++ client/src/hooks/useDebouncedCallback.ts | 31 +++ .../BudgetOverviewPage/BudgetOverviewPage.tsx | 29 ++- .../DiaryEntryEditPage.test.tsx | 63 +++++- .../DiaryEntryEditPage/DiaryEntryEditPage.tsx | 84 ++++---- client/src/pages/DiaryPage/DiaryPage.test.tsx | 41 ++++ client/src/pages/DiaryPage/DiaryPage.tsx | 34 ++-- .../WorkItemDetailPage.test.tsx | 6 +- .../WorkItemDetailPage/WorkItemDetailPage.tsx | 2 +- 41 files changed, 1319 insertions(+), 765 deletions(-) create mode 100644 .claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md delete mode 100644 client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.module.css delete mode 100644 client/src/components/HouseholdItemStatusBadge/HouseholdItemStatusBadge.test.tsx delete mode 100644 client/src/components/StatusBadge/StatusBadge.test.tsx create mode 100644 client/src/hooks/useClickOutside.test.ts create mode 100644 client/src/hooks/useClickOutside.ts create mode 100644 client/src/hooks/useDebounce.test.ts create mode 100644 client/src/hooks/useDebounce.ts create mode 100644 client/src/hooks/useDebouncedCallback.test.ts create mode 100644 client/src/hooks/useDebouncedCallback.ts diff --git a/.claude/agent-memory/qa-integration-tester/MEMORY.md b/.claude/agent-memory/qa-integration-tester/MEMORY.md index 0be04cf75..3d0f8eb53 100644 --- a/.claude/agent-memory/qa-integration-tester/MEMORY.md +++ b/.claude/agent-memory/qa-integration-tester/MEMORY.md @@ -15,6 +15,7 @@ ## Recent bug/story notes (2026-07) +- [Issue #1816 — component reuse + shared hooks](issue-1816-component-reuse-hooks.md) (2026-07-07) — 3 new 100%-covered hook tests (useDebounce/useDebouncedCallback/useClickOutside), Modal portal query gotcha (container vs document), CODE_BUG: pre-existing spurious autosave on draft-entry mount; `.test.ts`+JSX via React.createElement pattern - [Issue #1815 — stylelint remediation](issue-1815-stylelint-remediation.md) (2026-07-07) — 370-violation CSS lint sweep + CI gate wiring, clean verification pass (0 bugs); @extend-bug diff-check technique, keep-list grep technique - [Issue #1814 — i18n parity guard + usePhotos](issue-1814-i18n-parity-guard.md) (2026-07-07) — new `client/src/i18n/i18n.parity.test.ts`: generalized 14-namespace en/de key parity + 28-file duplicate-key raw-text scanner (canonical pattern going forward); `usePhotos.test.ts` reworked to assert real translateApiError/t() output; parallel translator/frontend edits landed before my first test run — all-green on first try was real, verified via git diff - [Issue #1813 — formatter consolidation](issue-1813-formatter-consolidation.md) (2026-07-07) — "shadow render" LocaleProvider-wrap pattern; CODE_BUG found (BudgetSourcesPage.tsx `formatPercent` ReferenceError, real TS2552, latent/untested by pre-existing tests); client-project ts-jest does NOT catch this TS error class at test-run time (inline tsconfig ≠ full program) — always cross-check with `npx tsc -p client/tsconfig.json --noEmit`; SignatureCapture.tsx lines 113-127 are genuinely unreachable dead code (pre-existing, out of scope); canvas/jsdom mocking recipe for SignatureCapture.test.tsx (new file, closed test-parity gap) diff --git a/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md b/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md new file mode 100644 index 000000000..80060ba52 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md @@ -0,0 +1,71 @@ +# Issue #1816 — component reuse (Modal focus trap, KeyboardShortcutsHelp, Badge x4, 3 shared hooks) + +Date: 2026-07-07 + +## What shipped + +- 3 new hook test files (100% coverage each): `client/src/hooks/useDebounce.test.ts`, + `useDebouncedCallback.test.ts`, `useClickOutside.test.ts`. +- `Modal.test.tsx`: +7 Tab-cycling focus-trap tests. +- `PhotoMetadataModal.test.tsx`: deleted the redundant private focus-trap describe block + (now covered generically by Modal.test.tsx). +- `KeyboardShortcutsHelp.test.tsx`: full rewrite — Modal now portals into `document.body`, + so `container.querySelector(...)` for backdrop/kbd elements silently returns null after + the migration. Must query `document`/`baseElement`, not the render `container`. +- `BudgetHealthIndicator.test.tsx`: retargeted `toHaveClass('onBudget'|'atRisk'|'overBudget')` + from the `role="status"` wrapper to `status.querySelector('span')` (the nested Badge span), + since the CSS class moved onto Badge's own span. Class names also changed + (`onBudget`→`budgetHealthOnBudget` etc, per new Badge.module.css variants). +- `SearchPicker.test.tsx`: added the missing "click inside portal dropdown doesn't close it" + regression test (fireEvent.mouseDown on the listbox itself) — confirms `useClickOutside`'s + `refs.floating` inclusion still protects the portal the same way the old + `document.querySelector('[data-search-picker-dropdown]')` check did. +- `DiaryPage.test.tsx`: added "page param not reset to 1 on mount with `?q=foo&page=3`" — + the regression the `isFirstSearchSync` guard protects against. +- `DiaryEntryEditPage.test.tsx`: added the uploadingCount-cancels-pending-autosave test + (Scenario 44b). Had to extend the existing `PhotoUpload` mock to also capture + `onUploadingCountChange` (previously only `onUpload` was captured). +- `WorkItemDetailPage.test.tsx`: added a comment clarifying the subtasks-empty-state + assertion now exercises the real `detail.subtasks.noSubtasks` i18n key, not a JSX literal + (text unchanged in English so the assertion string didn't need to change). +- Verified unchanged (all passed as-is): UpcomingMilestonesCard, CriticalPathCard, + SubsidyPipelineCard, DocumentBrowser, BudgetOverviewPage, DataTableColumnSettings, + OverflowMenu, MilestonesPage, WorkItemsPage — plus a blast-radius sweep of Modal/SearchPicker + consumers not explicitly listed in the spec (AreaPicker, OrientationPicker, DataTable, + EntityFilter, MassMoveModal, EditBudgetLineModal, BackupsPage, UserManagementPage) — all green. +- Deleted `StatusBadge/StatusBadge.test.tsx` and `HouseholdItemStatusBadge/HouseholdItemStatusBadge.test.tsx` + (stale — no source component existed). + +## CODE_BUG found (pre-existing, NOT introduced by #1816) + +`DiaryEntryEditPage.tsx`: mounting a **draft** entry fires one immediate, no-op-content +`updateDiaryEntry` autosave call even with zero user interaction. Root cause: the metadata-change +effect (`skipAutoSaveOnMountRef` guard, lines ~194-219) consumes its "skip" flag on the FIRST +effect run — which happens on initial mount (before `entry` loads, all metadata state is initial +values). When `loadEntry()` resolves and `entry?.status` flips from `undefined` to `'draft'`, the +effect's dependency array changes value, so it re-runs — but the skip flag is already spent, so it +fires `triggerAutoSave(true)` → an immediate `updateDiaryEntry` call with the *just-loaded, +unedited* content. Confirmed via isolated probe test (mount only, no interaction → +`mockUpdateDiaryEntry` called once). This predates #1816 — the PR only touched the debounce/cancel +plumbing (`doSaveImpl`/`scheduleAutoSave`), not this skip-ref effect. Existing Scenario 44 test +doesn't catch it because it asserts `toHaveBeenCalledWith(...)` not call count. Worked around in +the new Scenario 44b test by flushing+`mockClear()`-ing this spurious call before asserting the +real (debounce-cancel) behavior under test. Recommend filing a separate bug issue if not already +tracked — not in scope for me to fix (test-only agent). + +## Reusable pattern: capturing an untested mock prop + +When a component under test mocks a child (e.g. `PhotoUpload`) and only captures *some* of its +props (e.g. `onUpload` but not `onUploadingCountChange`), and a new spec needs to exercise the +uncaptured prop, extend the mock factory to also capture it — don't create a parallel mock. Wrap +any resulting `setState`-triggering invocation in `act()` since it's called directly, not through +`fireEvent`/`userEvent` (which auto-wrap). + +## `.test.ts` vs `.test.tsx` for hook tests that need DOM fixtures + +`useClickOutside` needed real rendered DOM elements (refs + a raw non-ref `HTMLElement`) to +exercise "accepts both RefObject and raw HTMLElement targets" — but the spec named the file +`useClickOutside.test.ts` (matching the hook's own `.ts`, no JSX). TypeScript disallows JSX syntax +in `.ts` files. Solution: use `React.createElement(...)` directly instead of JSX in the harness +component — keeps the `.test.ts` extension while still rendering real DOM via +`@testing-library/react`. diff --git a/.claude/agent-memory/ux-designer/photo-annotator-patterns.md b/.claude/agent-memory/ux-designer/photo-annotator-patterns.md index bb25bf57c..387bb8dd3 100644 --- a/.claude/agent-memory/ux-designer/photo-annotator-patterns.md +++ b/.claude/agent-memory/ux-designer/photo-annotator-patterns.md @@ -8,9 +8,9 @@ metadata: Patterns for `client/src/components/photos/PhotoAnnotator/`. Consult before specifying or reviewing changes to this component. - Tool palette: `role="toolbar"` wrapper; each button `.toolButton` / `.toolButtonActive`; `min-width/height: 44px`; `aria-pressed`; inline SVG icons (24×24, `stroke="currentColor"`); HighlightIcon uses `fill="currentColor"` (established precedent) -- Annotator dark-surface rgba values: `rgba(0,0,0,0.6)`, `rgba(255,255,255,0.4)` etc. in PhotoAnnotator.module.css are intentional photo-overlay hardcodes (pre-existing pattern); do NOT flag as token violations +- Annotator dark-surface values are now real tokens (PR #1847, Issue #1815): `--color-photo-*` family in `tokens.css` (overlay-caption, control-bg/-hover, bar-bg, border, focus-ring, active-bg, action-primary-bg, action-danger-bg) — theme-invariant by design (no dark override), consumed by `PhotoAnnotator`, `PhotoViewer`, `PhotoCard`. If you see a raw rgba in this family in a future diff, it's a regression, not an accepted exception anymore. - Font-size radiogroup: `role="radiogroup"` + `role="radio"` + `aria-checked`; buttons use `.fontSizeButton`/`.fontSizeButtonActive`; hover inside `prefers-reduced-motion` block (consistent with toolButton + strokeButton pattern) -- Inline text input (Story #1476): `.inlineTextInput` positioned absolute over canvas; focus managed via `requestAnimationFrame`; `aria-label` via `t('editText'|'editCallout')`; `z-index: 1000` is pre-existing (should be `var(--z-modal)`, refinement item); inline style should NOT duplicate CSS module's `min-width`/`z-index` +- Inline text input (Story #1476): `.inlineTextInput` positioned absolute over canvas; focus managed via `requestAnimationFrame`; `aria-label` via `t('editText'|'editCallout')`; `z-index: 1000` fixed to `var(--z-modal)` in PR #1847; inline style should NOT duplicate CSS module's `min-width`/`z-index` - TextIcon uses SVG `` element (not stroked path) — inconsistent with stroke icon family; flag for polish pass - Annotation colors in `ANNOTATION_COLORS` are intentionally hardcoded hex (not tokens) — marks must be theme-invariant; document this in any spec touching that file - Draft shape visual: `stroke-dasharray: 6 4`, `opacity: 0.8`, `pointer-events: none` — use for ALL new shape types diff --git a/.claude/agent-memory/ux-designer/pr-review-findings.md b/.claude/agent-memory/ux-designer/pr-review-findings.md index 731d83bf3..e7ca4cf80 100644 --- a/.claude/agent-memory/ux-designer/pr-review-findings.md +++ b/.claude/agent-memory/ux-designer/pr-review-findings.md @@ -56,6 +56,24 @@ Verified mechanism of the disclosed "document dates fixed off-by-one-day UTC bug Confirmed `gh pr review --approve` (not just `--request-changes`) also fails with "Can not approve your own pull request" on own PRs — same workaround as request-changes: `gh api repos/.../issues/{N}/comments`. +## PR #1847 — Stylelint enforcement + ~20 new tokens 1:1 extraction (Issue #1815) (APPROVED via gh api comment, own PR) + +Full remediation of 370 latent stylelint violations, plus wiring `stylelint` into `npm run lint`/`lint:fix` and a new CI `Stylelint` step (`static-analysis` job). Also fixed `.stylelintrc.json`'s `tokens.css` override (missing `color-function-alias-notation: null`, was causing 116 of the 370 violations). Verified every claim in the PR description by direct diff inspection rather than trusting the summary: + +- **`declaration-property-value-disallowed-list`** in `.stylelintrc.json` bans raw integer `z-index` and `font-weight` values via regex `/^\d+$/` — this is *why* `--z-raised: 1` and the `--z-local` escape hatch exist. Useful to know before flagging a future PR for "why didn't they just use `z-index: 1`" — it's lint-enforced, not stylistic preference. +- **New `--z-local` pattern** (first use in `PhotoMetadataSidepanel.module.css`): `--z-local: ;` declared immediately before `z-index: var(--z-local);` inside the same selector. Sanctioned escape hatch for component-internal arbitrary z-index offsets that don't belong in the shared global z-scale (`--z-dropdown`/`--z-overlay`/`--z-sidebar`/`--z-modal`) and aren't reused elsewhere. Recommended this get written into the Style Guide as the standard pattern for any future stylelint-blocked one-off literal. +- **`--z-raised: 1`** fills the gap below `--z-dropdown: 10` for "lift element above local siblings" — swapped in 1:1 for every existing `z-index: 1` (and `calc(var(--z-raised) + 1)` for what was `z-index: 2`) across `CostBreakdownTable`, `GanttSidebar`, `DashboardPage`, `WorkItemDetailPage`, `BudgetOverviewPage`, `TimelinePage`, `calendar/CalendarView`. +- **`--color-photo-*` family** (9 tokens: overlay-caption, control-bg/-hover, bar-bg, border, focus-ring, active-bg, action-primary-bg, action-danger-bg): deliberately theme-invariant (no dark override) — consumed by `PhotoViewer`, `PhotoCard`, `PhotoAnnotator`, all always-dark lightbox/editor chrome. This resolves two items I'd previously flagged as acceptable-but-not-ideal in [photo-annotator-patterns.md](photo-annotator-patterns.md): the dark-surface rgba hardcodes are now real tokens, and `PhotoAnnotator`'s `z-index: 1000` is now `var(--z-modal)`. +- **`--color-tooltip-*` family** (7 tokens: subdued, bullet, separator, status-not-started/in-progress/completed/late-bg): extracted from what used to be *component-scoped* custom properties re-declared inline in `GanttTooltip.module.css` under `.tooltip` and `[data-theme='dark'] .tooltip`. Only flag from this review: the tokens.css section comment says "GANTT TOOLTIP TOKENS" and sits in the Gantt-prefixed region, but the token names themselves are generic `--color-tooltip-*` (not `--color-gantt-tooltip-*`) — inconsistent with the rest of that section's `--color-gantt-` prefix convention, though defensible since a separate shared `Tooltip` component uses the same `--color-bg-inverse` surface pattern and could reuse these later. Low severity, non-blocking, flagged as a wiki/comment cleanup follow-up. +- **`--color-warning-badge-bg`** matches the existing `--color-{semantic}-badge-bg`/`-badge-text`/`-badge-bg-alt` scheme (`success`, `primary` already had entries) — good precedent, no `-badge-text` companion needed since consumer uses `var(--color-warning)` directly. +- **`--shadow-text-overlay`** — clean 1:1 extraction from `LinkedDocumentCard`'s inline text-shadow-over-thumbnail exception comment, preserved verbatim. +- **Visual-parity math verified, not assumed**: `InvoicePipelineCard .itemOverdue` swapped `rgba(251,146,60,0.08)` for solid `--color-warning-bg` (`#fff7ed`) — flattened the old rgba over white by hand (≈`rgb(255,246,239)` vs `rgb(255,247,237)`), confirms imperceptible difference, not a silent regression. +- **Real bug fix confirmed independently**: `InvoiceBudgetLinesSection.module.css` had `@extend .td;` (Sass syntax) in a project with zero Sass/postcss-extend tooling (confirmed no `postcss.config.*` extend plugin, no other `@extend` usage anywhere in repo) — was silently inert, so `tdDescription`/`tdCategory`/`tdPlanned`/`tdItemized`/`tdActions` never got `.td`'s padding. Fix replaces with the literal `padding: var(--spacing-3)`, matching `.td` exactly. +- **Legacy-duplicate-class dedup pattern** (also seen in PR #1846's i18n key dedup): `HouseholdItemDetailPage.module.css` and `CostBreakdownTable.module.css` each had select classes (`.modalContent`/`.modalTitle`/`.modalActions`/`.section`/`.sectionTitle`/`.propertyGrid`/`.property`/`.propertyLabel`; `.rowSourceDetail .colName`/`.nameIndented`) defined **twice** — an untokenized legacy block followed later by a fully-tokenized one. CSS last-wins means only the second block ever rendered; deleting the first is dead-code removal with zero visual change. **Recurring verification method**: grep `^\.classname {` counts in old vs. new file, confirm survivor is the tokenized one, confirm TSX references still resolve. +- **`print.css`** correctly uses Layer-1 palette tokens (`--color-black`, `--color-gray-700/300`) directly instead of Layer-2 semantic tokens — this is the right call since print output is intentionally theme-invariant (always black-on-white paper), a legitimate exception to the usual "never use Layer 1 in `.module.css`" rule. Also modernized deprecated `page-break-*`/`clip: rect()` to `break-*`/`clip-path: inset(50%)`. + +Verification method worth reusing: `awk '/^\+/ && !/^\+\+\+/' | grep -Ei '#[0-9a-fA-F]{3,6}|rgba?\('` and equivalent greps for `z-index:\s*[0-9]`/`font-weight:\s*[0-9]` against the full diff (excluding `tokens.css`) — fast way to confirm zero hardcoded-value regressions slipped past stylelint across a large multi-file remediation PR, rather than spot-checking a handful of files. + ## Process notes - Cannot `--request-changes` on your own PRs — use `--comment` instead, and note this in the review body diff --git a/.claude/agent-memory/ux-designer/token-reference.md b/.claude/agent-memory/ux-designer/token-reference.md index 2cb7815e9..aacfecb03 100644 --- a/.claude/agent-memory/ux-designer/token-reference.md +++ b/.claude/agent-memory/ux-designer/token-reference.md @@ -28,6 +28,21 @@ metadata: - `--color-danger-text` = white (text ON danger bg) — NEVER use as border or text on `--color-danger-bg`; use `--color-danger-border` for border, `--color-danger-text-on-light` for red text on light bg - Budget bar, Gantt, and milestone tokens already exist in tokens.css — check before specifying new domain-specific colors +## Stylelint-enforced literal bans (as of PR #1847 / Issue #1815) + +- `.stylelintrc.json` `declaration-property-value-disallowed-list` bans raw integer `z-index` and `font-weight` values via regex — any `z-index: 8` or `font-weight: 600` in a `.module.css` fails CI (`Stylelint` step in `static-analysis` job, wired into `npm run lint`/`lint:fix`). +- Escape hatch for arbitrary component-internal values that don't belong in the shared z-scale: declare a local `--z-local: ;` custom property immediately before `z-index: var(--z-local);` in the same selector (see `PhotoMetadataSidepanel.module.css`). Use this instead of inventing a one-off global token nothing else will reuse. +- `--z-raised: 1` now exists to fill the gap below `--z-dropdown: 10` — use it (or `calc(var(--z-raised) + 1)` etc.) for "lift element above local unstacked siblings" instead of a raw `z-index: 1`/`2`. +- `tokens.css` itself is exempted from `color-no-hex`/`function-disallowed-list`/`declaration-property-value-disallowed-list` (and `color-function-alias-notation`) via a stylelintrc override — palette/raw values are expected there, only `.module.css` consumers must reference `var(--token)`. +- `print.css` (global, not a CSS Module) legitimately uses Layer-1 palette tokens (`--color-black`, `--color-gray-700`, etc.) directly rather than Layer-2 semantic tokens — print output is intentionally theme-invariant, so this is not a violation of the "Layer 2 only in `.module.css`" rule. + +## New token families (PR #1847) + +- `--color-photo-*` (9 tokens) — theme-invariant photo-viewer/annotator chrome, no dark override by design. See [photo-annotator-patterns.md](photo-annotator-patterns.md). +- `--color-tooltip-*` (7 tokens) — generic naming despite living in the Gantt-prefixed tokens.css section (only consumer today is `GanttTooltip`, but shared `Tooltip` component uses the same `--color-bg-inverse` surface and could adopt these later). Section comment says "GANTT TOOLTIP TOKENS" which is a minor mismatch vs. the generic names — flagged as a low-severity wiki/comment cleanup follow-up, not fixed yet. +- `--color-warning-badge-bg` — follows the existing `--color-{semantic}-badge-bg`/`-badge-text`/`-badge-bg-alt` scheme (`success`, `primary` already had entries). +- `--shadow-text-overlay` — text-shadow for legibility over image thumbnails (image-overlay exception pattern). + ## Common token-substitution mistakes (recurring across PRs) - `0.875rem` → `var(--font-size-sm)`; `0.75rem` → `var(--font-size-xs)`; `0.375rem` → `var(--radius-md)` diff --git a/client/src/components/Badge/Badge.module.css b/client/src/components/Badge/Badge.module.css index 2de092bd0..8263938f1 100644 --- a/client/src/components/Badge/Badge.module.css +++ b/client/src/components/Badge/Badge.module.css @@ -240,6 +240,59 @@ border: 1px solid var(--color-success-border); } +/* Budget health indicator variants */ +.budgetHealthOnBudget { + background-color: var(--color-success-badge-bg); + color: var(--color-success-badge-text); +} + +.budgetHealthAtRisk { + background-color: var(--color-status-not-started-bg); + color: var(--color-status-not-started-text); +} + +.budgetHealthOverBudget { + background-color: var(--color-danger-bg-strong); + color: var(--color-danger-text-on-light); +} + +/* Schedule health variants (milestones, critical path) */ +.scheduleOnTrack { + background-color: var(--color-success-bg); + color: var(--color-success-text-on-light); +} + +.scheduleWarning { + background-color: var(--color-warning-bg); + color: var(--color-text-secondary); +} + +.scheduleAtRisk { + background-color: var(--color-danger-bg); + color: var(--color-danger-text-on-light); +} + +/* Subsidy pipeline status variants */ +.subsidyEligible { + background-color: var(--color-status-not-started-bg); + color: var(--color-status-not-started-text); +} + +.subsidyApplied { + background-color: var(--color-status-in-progress-bg); + color: var(--color-status-in-progress-text); +} + +.subsidyApproved { + background-color: var(--color-success-badge-bg); + color: var(--color-success-badge-text); +} + +.subsidyRejected { + background-color: var(--color-danger-bg); + color: var(--color-danger-text-on-light); +} + /* Responsive */ @media (max-width: 767px) { .badge { diff --git a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.module.css b/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.module.css deleted file mode 100644 index 0e421176e..000000000 --- a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.module.css +++ /dev/null @@ -1,25 +0,0 @@ -.badge { - display: inline-flex; - align-items: center; - padding: var(--spacing-1) var(--spacing-3); - border-radius: var(--radius-full); - font-size: var(--font-size-sm); - font-weight: var(--font-weight-semibold); - line-height: 1; - white-space: nowrap; -} - -.onBudget { - background: var(--color-success-badge-bg); - color: var(--color-success-badge-text); -} - -.atRisk { - background: var(--color-status-not-started-bg); - color: var(--color-status-not-started-text); -} - -.overBudget { - background: var(--color-danger-bg-strong); - color: var(--color-danger-text-on-light); -} diff --git a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.test.tsx b/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.test.tsx index f196086e3..84225be41 100644 --- a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.test.tsx +++ b/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.test.tsx @@ -32,10 +32,12 @@ describe('BudgetHealthIndicator', () => { expect(screen.getByRole('status')).toHaveTextContent('On Budget'); }); - it('applies onBudget CSS class when margin > 10%', () => { + it('applies budgetHealthOnBudget CSS class to the nested Badge span when margin > 10%', () => { render(); - expect(screen.getByRole('status')).toHaveClass('onBudget'); + const status = screen.getByRole('status'); + const badge = status.querySelector('span'); + expect(badge).toHaveClass('budgetHealthOnBudget'); }); // ── At Risk ────────────────────────────────────────────────────────────── @@ -68,10 +70,12 @@ describe('BudgetHealthIndicator', () => { expect(screen.getByRole('status')).toHaveTextContent('At Risk'); }); - it('applies atRisk CSS class when margin <= 10%', () => { + it('applies budgetHealthAtRisk CSS class to the nested Badge span when margin <= 10%', () => { render(); - expect(screen.getByRole('status')).toHaveClass('atRisk'); + const status = screen.getByRole('status'); + const badge = status.querySelector('span'); + expect(badge).toHaveClass('budgetHealthAtRisk'); }); // ── Over Budget ────────────────────────────────────────────────────────── @@ -88,10 +92,12 @@ describe('BudgetHealthIndicator', () => { expect(screen.getByRole('status')).toHaveTextContent('Over Budget'); }); - it('applies overBudget CSS class when remaining is negative', () => { + it('applies budgetHealthOverBudget CSS class to the nested Badge span when remaining is negative', () => { render(); - expect(screen.getByRole('status')).toHaveClass('overBudget'); + const status = screen.getByRole('status'); + const badge = status.querySelector('span'); + expect(badge).toHaveClass('budgetHealthOverBudget'); }); // ── Edge cases ──────────────────────────────────────────────────────────── diff --git a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.tsx b/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.tsx index 85ba5259e..d94b6642a 100644 --- a/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.tsx +++ b/client/src/components/BudgetHealthIndicator/BudgetHealthIndicator.tsx @@ -1,5 +1,7 @@ +import { useMemo } from 'react'; import { useTranslation } from 'react-i18next'; -import styles from './BudgetHealthIndicator.module.css'; +import { Badge, type BadgeVariantMap } from '../Badge/Badge.js'; +import badgeStyles from '../Badge/Badge.module.css'; interface BudgetHealthIndicatorProps { remainingVsProjectedMax: number; @@ -8,45 +10,26 @@ interface BudgetHealthIndicatorProps { type HealthStatus = 'on-budget' | 'at-risk' | 'over-budget'; -interface HealthConfig { - status: HealthStatus; - label: string; - cssClass: string; -} - -function resolveHealth(remainingVsProjectedMax: number, availableFunds: number): HealthConfig { +function resolveHealthStatus( + remainingVsProjectedMax: number, + availableFunds: number, +): HealthStatus { if (remainingVsProjectedMax < 0) { - return { - status: 'over-budget', - label: 'health.overBudget', - cssClass: styles.overBudget!, - }; + return 'over-budget'; } // Special case: both are exactly zero — treat as at-risk if (availableFunds === 0) { - return { - status: 'at-risk', - label: 'health.atRisk', - cssClass: styles.atRisk!, - }; + return 'at-risk'; } const margin = remainingVsProjectedMax / availableFunds; if (margin > 0.1) { - return { - status: 'on-budget', - label: 'health.onBudget', - cssClass: styles.onBudget!, - }; + return 'on-budget'; } - return { - status: 'at-risk', - label: 'health.atRisk', - cssClass: styles.atRisk!, - }; + return 'at-risk'; } export function BudgetHealthIndicator({ @@ -54,11 +37,23 @@ export function BudgetHealthIndicator({ availableFunds, }: BudgetHealthIndicatorProps) { const { t } = useTranslation('budget'); - const { label, cssClass } = resolveHealth(remainingVsProjectedMax, availableFunds); + const status = resolveHealthStatus(remainingVsProjectedMax, availableFunds); + + const variants = useMemo( + (): BadgeVariantMap => ({ + 'on-budget': { label: t('health.onBudget'), className: badgeStyles.budgetHealthOnBudget! }, + 'at-risk': { label: t('health.atRisk'), className: badgeStyles.budgetHealthAtRisk! }, + 'over-budget': { + label: t('health.overBudget'), + className: badgeStyles.budgetHealthOverBudget!, + }, + }), + [t], + ); return ( - - {t(label)} + + ); } diff --git a/client/src/components/DataTable/DataTableColumnSettings.tsx b/client/src/components/DataTable/DataTableColumnSettings.tsx index d055163ca..4ad8c357e 100644 --- a/client/src/components/DataTable/DataTableColumnSettings.tsx +++ b/client/src/components/DataTable/DataTableColumnSettings.tsx @@ -1,5 +1,6 @@ import { useEffect, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; +import { useClickOutside } from '../../hooks/useClickOutside.js'; import type { ColumnDef } from './DataTable.js'; import styles from './DataTable.module.css'; @@ -37,31 +38,21 @@ export function DataTableColumnSettings({ const [dragOverState, setDragOverState] = useState(null); // Close on outside click + useClickOutside([popoverRef, triggerRef], () => setIsOpen(false), isOpen); + + // Close on Escape key useEffect(() => { if (!isOpen) return; - const handleClickOutside = (event: MouseEvent) => { - if ( - popoverRef.current && - !popoverRef.current.contains(event.target as Node) && - triggerRef.current && - !triggerRef.current.contains(event.target as Node) - ) { - setIsOpen(false); - } - }; - const handleEscape = (event: KeyboardEvent) => { if (event.key === 'Escape') { setIsOpen(false); } }; - document.addEventListener('mousedown', handleClickOutside); document.addEventListener('keydown', handleEscape); return () => { - document.removeEventListener('mousedown', handleClickOutside); document.removeEventListener('keydown', handleEscape); }; }, [isOpen]); diff --git a/client/src/components/HouseholdItemStatusBadge/HouseholdItemStatusBadge.test.tsx b/client/src/components/HouseholdItemStatusBadge/HouseholdItemStatusBadge.test.tsx deleted file mode 100644 index 8b2c96ba4..000000000 --- a/client/src/components/HouseholdItemStatusBadge/HouseholdItemStatusBadge.test.tsx +++ /dev/null @@ -1,92 +0,0 @@ -/** - * @jest-environment jsdom - */ -import { describe, it, expect } from '@jest/globals'; -import { render } from '@testing-library/react'; -import { Badge } from '../Badge/Badge.js'; -import badgeStyles from '../Badge/Badge.module.css'; - -// Variant map mirroring the production definition in HouseholdItemsPage.tsx -const HI_STATUS_VARIANTS = { - planned: { label: 'Planned', className: badgeStyles.planned! }, - purchased: { label: 'Purchased', className: badgeStyles.purchased! }, - scheduled: { label: 'Scheduled', className: badgeStyles.scheduled! }, - arrived: { label: 'Arrived', className: badgeStyles.arrived! }, -}; - -describe('Badge — household item status variants', () => { - // ─── Labels ──────────────────────────────────────────────────────────────── - - it('renders "Planned" for planned status', () => { - const { container } = render(); - expect(container.querySelector('span')?.textContent).toBe('Planned'); - }); - - it('renders "Purchased" for purchased status', () => { - const { container } = render(); - expect(container.querySelector('span')?.textContent).toBe('Purchased'); - }); - - it('renders "Scheduled" for scheduled status', () => { - const { container } = render(); - expect(container.querySelector('span')?.textContent).toBe('Scheduled'); - }); - - it('renders "Arrived" for arrived status', () => { - const { container } = render(); - expect(container.querySelector('span')?.textContent).toBe('Arrived'); - }); - - // ─── Base CSS class ───────────────────────────────────────────────────────── - - it('applies badge base CSS class for planned', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - it('applies badge base CSS class for purchased', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - it('applies badge base CSS class for scheduled', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - it('applies badge base CSS class for arrived', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - // ─── Variant CSS class ────────────────────────────────────────────────────── - - it('applies planned CSS class for planned status', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('planned'); - }); - - it('applies purchased CSS class for purchased status', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('purchased'); - }); - - it('applies scheduled CSS class for scheduled status', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('scheduled'); - }); - - it('applies arrived CSS class for arrived status', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('arrived'); - }); - - // ─── Element type ─────────────────────────────────────────────────────────── - - it('renders as a span element', () => { - const { container } = render(); - const span = container.querySelector('span'); - expect(span).toBeInTheDocument(); - expect(span?.tagName.toLowerCase()).toBe('span'); - }); -}); diff --git a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.module.css b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.module.css index c6436118c..a50df79c8 100644 --- a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.module.css +++ b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.module.css @@ -1,80 +1,3 @@ -/* Modal */ -.modal { - position: fixed; - top: 0; - left: 0; - right: 0; - bottom: 0; - z-index: var(--z-modal); - display: flex; - align-items: center; - justify-content: center; - padding: 1rem; -} - -.modalBackdrop { - position: absolute; - top: 0; - left: 0; - right: 0; - bottom: 0; - background-color: var(--color-overlay); -} - -.modalContent { - position: relative; - background-color: var(--color-bg-primary); - border-radius: 0.5rem; - padding: 0; - max-width: 32rem; - width: 100%; - box-shadow: var(--shadow-xl); - max-height: 80vh; - overflow: hidden; - display: flex; - flex-direction: column; -} - -.modalHeader { - display: flex; - justify-content: space-between; - align-items: center; - padding: 1.5rem 1.5rem 1rem 1.5rem; - border-bottom: 1px solid var(--color-border); -} - -.modalTitle { - font-size: 1.25rem; - font-weight: var(--font-weight-semibold); - color: var(--color-text-primary); - margin: 0; -} - -.closeButton { - background: none; - border: none; - color: var(--color-text-muted); - font-size: 2rem; - line-height: 1; - cursor: pointer; - padding: 0; - width: 2rem; - height: 2rem; - display: flex; - align-items: center; - justify-content: center; - border-radius: 0.25rem; - transition: - background-color 0.2s, - color 0.2s; -} - -.closeButton:hover { - background-color: var(--color-bg-tertiary); - color: var(--color-text-primary); -} - -/* Shortcuts Table */ .shortcutsTable { width: 100%; border-collapse: collapse; @@ -88,9 +11,9 @@ } .shortcutsTable th { - padding: 0.75rem 1.5rem; + padding: var(--spacing-3) var(--spacing-6); text-align: left; - font-size: 0.75rem; + font-size: var(--font-size-xs); font-weight: var(--font-weight-semibold); color: var(--color-text-muted); text-transform: uppercase; @@ -115,8 +38,8 @@ } .shortcutsTable td { - padding: 0.875rem 1.5rem; - font-size: 0.875rem; + padding: var(--spacing-4) var(--spacing-6); + font-size: var(--font-size-sm); } .keyCell { @@ -129,37 +52,23 @@ .kbd { display: inline-block; - padding: 0.25rem 0.5rem; + padding: var(--spacing-1) var(--spacing-2); font-family: ui-monospace, SFMono-Regular, Menlo, Monaco, Consolas, 'Liberation Mono', 'Courier New', monospace; - font-size: 0.75rem; + font-size: var(--font-size-xs); font-weight: var(--font-weight-semibold); line-height: 1; color: var(--color-bg-inverse); background-color: var(--color-bg-tertiary); border: 1px solid var(--color-border-strong); - border-radius: 0.25rem; + border-radius: var(--radius-sm); box-shadow: var(--shadow-kbd); } -/* Responsive */ @media (max-width: 767px) { - .modalContent { - max-width: 100%; - max-height: 90vh; - } - - .modalHeader { - padding: 1rem; - } - - .modalTitle { - font-size: 1.125rem; - } - .shortcutsTable th, .shortcutsTable td { - padding: 0.75rem 1rem; + padding: var(--spacing-3) var(--spacing-4); } } diff --git a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.test.tsx b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.test.tsx index 520d55532..2c4db0e4e 100644 --- a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.test.tsx +++ b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.test.tsx @@ -1,9 +1,18 @@ +/** + * @jest-environment jsdom + */ import { jest, describe, it, expect } from '@jest/globals'; import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { KeyboardShortcutsHelp } from './KeyboardShortcutsHelp.js'; import type { KeyboardShortcut } from '../../hooks/useKeyboardShortcuts.js'; +// KeyboardShortcutsHelp now renders via the shared Modal component, which uses +// createPortal into document.body — dialog role, aria-modal, backdrop, Escape, +// and focus trap are all inherited from Modal (covered generically by Modal.test.tsx). +// These tests verify KeyboardShortcutsHelp's own rendering (table/rows) plus that +// it correctly wires into Modal (onClose plumbing, dialog semantics). + describe('KeyboardShortcutsHelp', () => { const mockShortcuts: KeyboardShortcut[] = [ { key: 'n', handler: () => {}, description: 'New work item' }, @@ -11,12 +20,24 @@ describe('KeyboardShortcutsHelp', () => { { key: '?', handler: () => {}, description: 'Show keyboard shortcuts' }, ]; - it('should render modal with shortcuts list', () => { + it('renders inside a Modal with dialog role, aria-modal, and portal to document.body', () => { + const onClose = jest.fn<() => void>(); + const { baseElement } = render( + , + ); + + const dialog = screen.getByRole('dialog'); + expect(dialog).toBeInTheDocument(); + expect(dialog).toHaveAttribute('aria-modal', 'true'); + // baseElement is document.body — portal content should live there + expect(baseElement.querySelector('[role="dialog"]')).toBe(dialog); + }); + + it('should render modal with title and shortcuts list', () => { const onClose = jest.fn<() => void>(); render(); - expect(screen.getByRole('dialog')).toBeInTheDocument(); - expect(screen.getByText('Keyboard Shortcuts')).toBeInTheDocument(); + expect(screen.getByRole('heading', { name: 'Keyboard Shortcuts' })).toBeInTheDocument(); // Check that all shortcuts are displayed expect(screen.getByText('n')).toBeInTheDocument(); @@ -42,12 +63,11 @@ describe('KeyboardShortcutsHelp', () => { it('should call onClose when backdrop is clicked', async () => { const onClose = jest.fn<() => void>(); - const { container } = render( - , - ); + render(); - // Find the backdrop by className (since it doesn't have a role) - const backdrop = container.querySelector('[class*="modalBackdrop"]'); + // Modal portals into document.body, so the backdrop isn't inside the + // render() container — query the whole document instead. + const backdrop = document.querySelector('[class*="modalBackdrop"]'); expect(backdrop).toBeInTheDocument(); await userEvent.click(backdrop!); @@ -65,13 +85,34 @@ describe('KeyboardShortcutsHelp', () => { expect(onClose).toHaveBeenCalledTimes(1); }); + it('close button aria-label resolves via common:aria.closeDialog (not the removed keyboardShortcuts.closeAriaLabel key)', () => { + const onClose = jest.fn<() => void>(); + render(); + + // "Close dialog" is the en value of common:aria.closeDialog — the same key + // Modal uses for every consumer. KeyboardShortcutsHelp no longer defines its + // own close-button aria-label. + expect(screen.getByRole('button', { name: 'Close dialog' })).toBeInTheDocument(); + }); + + it('Escape key closes the dialog (inherited from Modal, not KeyboardShortcutsHelp logic)', () => { + const onClose = jest.fn<() => void>(); + render(); + + // Fire directly against document, matching Modal's own Escape-key effect + const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true }); + document.dispatchEvent(event); + + expect(onClose).toHaveBeenCalledTimes(1); + }); + it('should render all shortcuts with kbd elements', () => { const onClose = jest.fn<() => void>(); - const { container } = render( - , - ); + render(); - const kbdElements = container.querySelectorAll('kbd'); + // Modal portals into document.body, so query document instead of the + // render() container. + const kbdElements = document.querySelectorAll('kbd'); expect(kbdElements).toHaveLength(3); expect(kbdElements[0]!).toHaveTextContent('n'); @@ -96,4 +137,18 @@ describe('KeyboardShortcutsHelp', () => { // Only header row, no data rows expect(rows).toHaveLength(1); }); + + it('filters out shortcuts with no description', () => { + const onClose = jest.fn<() => void>(); + const shortcutsWithBlank: KeyboardShortcut[] = [ + ...mockShortcuts, + { key: 'x', handler: () => {}, description: '' }, + ]; + render(); + + const rows = screen.getAllByRole('row'); + // Header row + 3 data rows (the blank-description shortcut is filtered out) + expect(rows).toHaveLength(4); + expect(screen.queryByText('x')).not.toBeInTheDocument(); + }); }); diff --git a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.tsx b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.tsx index 63d3e2fbf..7b6b09f63 100644 --- a/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.tsx +++ b/client/src/components/KeyboardShortcutsHelp/KeyboardShortcutsHelp.tsx @@ -1,5 +1,6 @@ import { useTranslation } from 'react-i18next'; import type { KeyboardShortcut } from '../../hooks/useKeyboardShortcuts.js'; +import { Modal } from '../Modal/Modal.js'; import styles from './KeyboardShortcutsHelp.module.css'; interface KeyboardShortcutsHelpProps { @@ -10,42 +11,28 @@ interface KeyboardShortcutsHelpProps { export function KeyboardShortcutsHelp({ shortcuts, onClose }: KeyboardShortcutsHelpProps) { const { t } = useTranslation('common'); return ( -
-
-
-
-

{t('keyboardShortcuts.title')}

- -
- - - - - - - - - {shortcuts - .filter((shortcut) => shortcut.description) - .map((shortcut) => ( - - - - - ))} - -
{t('keyboardShortcuts.keyColumn')}{t('keyboardShortcuts.actionColumn')}
- {shortcut.key} - {shortcut.description}
-
-
+ + + + + + + + + + {shortcuts + .filter((shortcut) => shortcut.description) + .map((shortcut) => ( + + + + + ))} + +
{t('keyboardShortcuts.keyColumn')}{t('keyboardShortcuts.actionColumn')}
+ {shortcut.key} + {shortcut.description}
+ ); } diff --git a/client/src/components/Modal/Modal.test.tsx b/client/src/components/Modal/Modal.test.tsx index f22c7975b..245129bf5 100644 --- a/client/src/components/Modal/Modal.test.tsx +++ b/client/src/components/Modal/Modal.test.tsx @@ -256,4 +256,122 @@ describe('Modal', () => { // baseElement is document.body; portal content should be there expect(baseElement.querySelector('[data-testid="portal-content"]')).toBeTruthy(); }); + + // ── Focus trap (Tab-cycling) ────────────────────────────────────────────── + + it('Tab on the last focusable element wraps focus to the first', () => { + render( + + + , + ); + + // Focusables in DOM order: close button (header), then the input (body). + const closeButton = screen.getByRole('button', { name: 'Close dialog' }); + const input = screen.getByTestId('only-input'); + + // Close button is focused on mount; move focus to the last focusable (input). + input.focus(); + expect(document.activeElement).toBe(input); + + fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); + + expect(document.activeElement).toBe(closeButton); + }); + + it('Shift+Tab on the first focusable element wraps focus to the last', () => { + render( + + + , + ); + + const closeButton = screen.getByRole('button', { name: 'Close dialog' }); + const input = screen.getByTestId('only-input'); + + // Close button is the first focusable (and is focused on mount). + closeButton.focus(); + expect(document.activeElement).toBe(closeButton); + + fireEvent.keyDown(document, { key: 'Tab', shiftKey: true }); + + expect(document.activeElement).toBe(input); + }); + + it('Tab and Shift+Tab with a single focusable element keep focus on it', () => { + render( + +

No interactive elements in the body

+ , + ); + + // The only focusable element in the whole content panel is the close button. + const closeButton = screen.getByRole('button', { name: 'Close dialog' }); + expect(document.activeElement).toBe(closeButton); + + fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); + expect(document.activeElement).toBe(closeButton); + + fireEvent.keyDown(document, { key: 'Tab', shiftKey: true }); + expect(document.activeElement).toBe(closeButton); + }); + + it('Tab does not throw and does not move focus when the active element is neither first nor last', () => { + render( + + + + , + ); + + // Focusables in DOM order: close button, first-input, second-input. + // Focus the middle element — neither first (close button) nor last (second-input). + const middleInput = screen.getByTestId('first-input'); + middleInput.focus(); + expect(document.activeElement).toBe(middleInput); + + expect(() => { + fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); + }).not.toThrow(); + + // The trap only intervenes at the boundaries; focus is left untouched here + // (the browser's native Tab order would normally move focus, but jsdom + // doesn't simulate that — this test only verifies the trap doesn't hijack it). + expect(document.activeElement).toBe(middleInput); + }); + + it('Tab does not throw when contentRef has no element ref yet (defensive null guard)', () => { + // Covers the `!contentRef.current` guard in the handler — exercised naturally + // once the Modal unmounts and its ref is cleared, but the listener is also + // removed on unmount, so this asserts the unmounted (no-op) case doesn't throw. + const { unmount } = render( + + + , + ); + + unmount(); + + expect(() => { + fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); + }).not.toThrow(); + }); + + it('trap only affects focus within contentRef, not the whole document (non-Tab keys still ignored)', () => { + const onClose = jest.fn<() => void>(); + render( + + + , + ); + + const input = screen.getByTestId('only-input'); + input.focus(); + + // A non-Tab key must not trigger the wrap logic or onClose. + fireEvent.keyDown(document, { key: 'ArrowDown' }); + + expect(document.activeElement).toBe(input); + expect(onClose).not.toHaveBeenCalled(); + }); }); diff --git a/client/src/components/Modal/Modal.tsx b/client/src/components/Modal/Modal.tsx index 2b510b515..fb3475aa3 100644 --- a/client/src/components/Modal/Modal.tsx +++ b/client/src/components/Modal/Modal.tsx @@ -12,6 +12,14 @@ export interface ModalProps { className?: string; } +function getFocusableElements(container: HTMLElement): HTMLElement[] { + return Array.from( + container.querySelectorAll( + 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])', + ), + ).filter((el) => !el.hasAttribute('disabled')); +} + export function Modal({ title, onClose, children, footer, className }: ModalProps) { const contentRef = useRef(null); const titleId = useId(); @@ -32,14 +40,35 @@ export function Modal({ title, onClose, children, footer, className }: ModalProp // Focus management: focus first focusable element on mount useEffect(() => { if (contentRef.current) { - const focusableElements = contentRef.current.querySelectorAll( - 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])', - ); - const firstFocusable = focusableElements[0] as HTMLElement; + const [firstFocusable] = getFocusableElements(contentRef.current); firstFocusable?.focus(); } }, []); + // Focus trap: cycle Tab/Shift+Tab within the modal content + useEffect(() => { + const handleTabKey = (e: KeyboardEvent) => { + if (e.key !== 'Tab' || !contentRef.current) return; + + const focusable = getFocusableElements(contentRef.current); + if (focusable.length === 0) return; + + const first = focusable[0]!; + const last = focusable[focusable.length - 1]!; + + if (e.shiftKey && document.activeElement === first) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && document.activeElement === last) { + e.preventDefault(); + first.focus(); + } + }; + + document.addEventListener('keydown', handleTabKey); + return () => document.removeEventListener('keydown', handleTabKey); + }, []); + return createPortal(
diff --git a/client/src/components/OverflowMenu/OverflowMenu.tsx b/client/src/components/OverflowMenu/OverflowMenu.tsx index 6dc6fcca7..06278269a 100644 --- a/client/src/components/OverflowMenu/OverflowMenu.tsx +++ b/client/src/components/OverflowMenu/OverflowMenu.tsx @@ -1,5 +1,6 @@ import { useState, useRef, useEffect, useLayoutEffect, type ReactNode } from 'react'; import { createPortal } from 'react-dom'; +import { useClickOutside } from '../../hooks/useClickOutside.js'; import styles from './OverflowMenu.module.css'; export const SCROLL_CLOSE_THRESHOLD_PX = 8; @@ -43,21 +44,7 @@ export function OverflowMenu({ const menuRef = useRef(null); // Close menu on outside click - useEffect(() => { - if (!isOpen) return; - - const handleMouseDown = (e: MouseEvent) => { - if (usePortal && menuRef.current && menuRef.current.contains(e.target as Node)) { - return; - } - if (wrapperRef.current && !wrapperRef.current.contains(e.target as Node)) { - setIsOpen(false); - } - }; - - document.addEventListener('mousedown', handleMouseDown); - return () => document.removeEventListener('mousedown', handleMouseDown); - }, [isOpen, usePortal]); + useClickOutside([wrapperRef, menuRef], () => setIsOpen(false), isOpen); // Close menu on scroll and resize when using portal useEffect(() => { diff --git a/client/src/components/SearchPicker/SearchPicker.test.tsx b/client/src/components/SearchPicker/SearchPicker.test.tsx index 48e95d11a..a227cdb7c 100644 --- a/client/src/components/SearchPicker/SearchPicker.test.tsx +++ b/client/src/components/SearchPicker/SearchPicker.test.tsx @@ -2,7 +2,7 @@ * @jest-environment jsdom */ import { describe, it, expect, jest, beforeEach, afterEach } from '@jest/globals'; -import { render, screen, act, waitFor } from '@testing-library/react'; +import { render, screen, act, waitFor, fireEvent } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { SearchPicker } from './SearchPicker.js'; @@ -1001,6 +1001,35 @@ describe('portal rendering (Story #1600)', () => { }); }); + // ── Test 11b: click inside the portal dropdown is NOT treated as outside ── + // Regression test for the useClickOutside migration (#1816): the old + // implementation special-cased this via a + // `document.querySelector('[data-search-picker-dropdown]')` check inside its + // handler. useClickOutside instead includes `refs.floating` (the portal + // element) directly in its target list — this test locks in that the + // portal-aware behavior survived the migration. + + it('clicking inside the portalled dropdown does not close it', async () => { + const user = userEvent.setup(); + renderPicker({ showItemsOnFocus: true, placeholder: 'Search...' }); + + const input = screen.getByPlaceholderText('Search...'); + await user.click(input); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + const listbox = screen.getByRole('listbox'); + + // A mousedown directly on the listbox container itself (not on an option, + // to avoid also triggering a selection) must NOT be treated as "outside". + fireEvent.mouseDown(listbox); + + // The dropdown must remain open. + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + // ── Test 12: Escape inside portal closes dropdown ──────────────────────── it('pressing Escape inside the portalled dropdown closes the dropdown', async () => { diff --git a/client/src/components/SearchPicker/SearchPicker.tsx b/client/src/components/SearchPicker/SearchPicker.tsx index 889849034..9bb6a7532 100644 --- a/client/src/components/SearchPicker/SearchPicker.tsx +++ b/client/src/components/SearchPicker/SearchPicker.tsx @@ -10,6 +10,8 @@ import { size, FloatingPortal, } from '@floating-ui/react'; +import { useDebouncedCallback } from '../../hooks/useDebouncedCallback.js'; +import { useClickOutside } from '../../hooks/useClickOutside.js'; import styles from './SearchPicker.module.css'; @@ -88,7 +90,6 @@ export function SearchPicker({ const containerRef = useRef(null); const inputRef = useRef(null); - const debounceRef = useRef | null>(null); const { refs, floatingStyles, isPositioned } = useFloating({ open: isOpen, @@ -110,26 +111,7 @@ export function SearchPicker({ }); // Close dropdown on click outside - useEffect(() => { - function handleClickOutside(event: MouseEvent) { - if (containerRef.current && !containerRef.current.contains(event.target as Node)) { - // Also check if the click was inside the portal dropdown - const portalEl = document.querySelector('[data-search-picker-dropdown]'); - if (portalEl && portalEl.contains(event.target as Node)) { - return; - } - setIsOpen(false); - } - } - - if (isOpen) { - document.addEventListener('mousedown', handleClickOutside); - } - - return () => { - document.removeEventListener('mousedown', handleClickOutside); - }; - }, [isOpen]); + useClickOutside([containerRef, refs.floating], () => setIsOpen(false), isOpen); // Reset when value is cleared externally (e.g. after form submission) useEffect(() => { @@ -146,15 +128,6 @@ export function SearchPicker({ /* eslint-enable @eslint-react/set-state-in-effect */ }, [value, specialOptions]); - // Cleanup debounce on unmount - useEffect(() => { - return () => { - if (debounceRef.current) { - clearTimeout(debounceRef.current); - } - }; - }, []); - const fetchInitialResults = useCallback(async () => { setIsLoading(true); setError(null); @@ -193,17 +166,12 @@ export function SearchPicker({ [excludeIds, fetchInitialResults, searchFn, resolvedSearchError], ); + const debouncedSearch = useDebouncedCallback(performSearch, 300); + const handleInputChange = (inputValue: string) => { setSearchTerm(inputValue); setIsOpen(true); - - if (debounceRef.current) { - clearTimeout(debounceRef.current); - } - - debounceRef.current = setTimeout(() => { - performSearch(inputValue); - }, 300); + debouncedSearch.trigger(inputValue); }; const handleFocus = () => { diff --git a/client/src/components/StatusBadge/StatusBadge.test.tsx b/client/src/components/StatusBadge/StatusBadge.test.tsx deleted file mode 100644 index deadeac91..000000000 --- a/client/src/components/StatusBadge/StatusBadge.test.tsx +++ /dev/null @@ -1,90 +0,0 @@ -/** - * @jest-environment jsdom - */ -import { describe, it, expect } from '@jest/globals'; -import { render } from '@testing-library/react'; -import { Badge } from '../Badge/Badge.js'; -import badgeStyles from '../Badge/Badge.module.css'; - -// Variant map mirroring the production definition in WorkItemsPage.tsx -const WORK_ITEM_STATUS_VARIANTS = { - not_started: { label: 'Not Started', className: badgeStyles.not_started! }, - in_progress: { label: 'In Progress', className: badgeStyles.in_progress! }, - completed: { label: 'Completed', className: badgeStyles.completed! }, -}; - -describe('Badge — work item status variants', () => { - // ─── Labels ──────────────────────────────────────────────────────────────── - - it('renders "Not Started" for not_started status', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.textContent).toBe('Not Started'); - }); - - it('renders "In Progress" for in_progress status', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.textContent).toBe('In Progress'); - }); - - it('renders "Completed" for completed status', () => { - const { container } = render(); - expect(container.querySelector('span')?.textContent).toBe('Completed'); - }); - - // ─── Base CSS class ───────────────────────────────────────────────────────── - - it('applies the badge base CSS class for not_started', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - it('applies the badge base CSS class for in_progress', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - it('applies the badge base CSS class for completed', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('badge'); - }); - - // ─── Variant CSS class ────────────────────────────────────────────────────── - - it('applies not_started CSS class for not_started status', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('not_started'); - }); - - it('applies in_progress CSS class for in_progress status', () => { - const { container } = render( - , - ); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('in_progress'); - }); - - it('applies completed CSS class for completed status', () => { - const { container } = render(); - expect(container.querySelector('span')?.getAttribute('class') ?? '').toContain('completed'); - }); - - // ─── Element type ─────────────────────────────────────────────────────────── - - it('renders as a span element', () => { - const { container } = render( - , - ); - const span = container.querySelector('span'); - expect(span).toBeInTheDocument(); - expect(span?.tagName.toLowerCase()).toBe('span'); - }); -}); diff --git a/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.module.css b/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.module.css index 40f8cef91..938a20ec5 100644 --- a/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.module.css +++ b/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.module.css @@ -17,37 +17,6 @@ background-color: var(--color-bg-secondary); } -.badge { - display: inline-flex; - align-items: center; - padding: var(--spacing-0-5) var(--spacing-2); - border-radius: var(--radius-full); - font-size: var(--font-size-xs); - font-weight: var(--font-weight-medium); - white-space: nowrap; - flex-shrink: 0; -} - -.badgeGray { - background-color: var(--color-status-not-started-bg); - color: var(--color-status-not-started-text); -} - -.badgeBlue { - background-color: var(--color-status-in-progress-bg); - color: var(--color-status-in-progress-text); -} - -.badgeGreen { - background-color: var(--color-success-badge-bg); - color: var(--color-success-badge-text); -} - -.badgeRed { - background-color: var(--color-danger-bg); - color: var(--color-danger-text-on-light); -} - .groupCount { flex: 1; font-size: var(--font-size-sm); diff --git a/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.tsx b/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.tsx index f0617eb92..21dbf2383 100644 --- a/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.tsx +++ b/client/src/components/SubsidyPipelineCard/SubsidyPipelineCard.tsx @@ -1,7 +1,10 @@ +import { useMemo } from 'react'; import { Link } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; import type { SubsidyProgram } from '@cornerstone/shared'; import { useFormatters } from '../../lib/formatters.js'; +import { Badge, type BadgeVariantMap } from '../Badge/Badge.js'; +import badgeStyles from '../Badge/Badge.module.css'; import styles from './SubsidyPipelineCard.module.css'; interface SubsidyPipelineCardProps { @@ -10,17 +13,41 @@ interface SubsidyPipelineCardProps { interface StatusGroup { status: string; - label: string; count: number; totalFixedReduction: number; hasUpcomingDeadline: boolean; - badgeClass: string; } export function SubsidyPipelineCard({ subsidyPrograms }: SubsidyPipelineCardProps) { const { t } = useTranslation('dashboard'); const { formatCurrency } = useFormatters(); + const statusBadgeVariants = useMemo( + (): BadgeVariantMap => ({ + eligible: { + label: t('cards.subsidyPipeline.statuses.eligible'), + className: badgeStyles.subsidyEligible!, + }, + applied: { + label: t('cards.subsidyPipeline.statuses.applied'), + className: badgeStyles.subsidyApplied!, + }, + approved: { + label: t('cards.subsidyPipeline.statuses.approved'), + className: badgeStyles.subsidyApproved!, + }, + received: { + label: t('cards.subsidyPipeline.statuses.received'), + className: badgeStyles.subsidyApproved!, + }, + rejected: { + label: t('cards.subsidyPipeline.statuses.rejected'), + className: badgeStyles.subsidyRejected!, + }, + }), + [t], + ); + // Helper to check if deadline is within 14 days (inclusive) from today and >= 0 days in future const isUpcomingDeadline = (deadline: string | null): boolean => { if (!deadline) return false; @@ -48,27 +75,11 @@ export function SubsidyPipelineCard({ subsidyPrograms }: SubsidyPipelineCardProp const hasUpcomingDeadline = programs.some((p) => isUpcomingDeadline(p.applicationDeadline)); - const badgeClassMap: Record = { - eligible: styles.badgeGray!, - applied: styles.badgeBlue!, - approved: styles.badgeGreen!, - received: styles.badgeGreen!, - }; - - const labelMap: Record = { - eligible: t('cards.subsidyPipeline.statuses.eligible')!, - applied: t('cards.subsidyPipeline.statuses.applied')!, - approved: t('cards.subsidyPipeline.statuses.approved')!, - received: t('cards.subsidyPipeline.statuses.received')!, - }; - statusGroups.push({ status, - label: labelMap[status]!, count: programs.length, totalFixedReduction, hasUpcomingDeadline, - badgeClass: badgeClassMap[status]!, }); } } @@ -78,11 +89,9 @@ export function SubsidyPipelineCard({ subsidyPrograms }: SubsidyPipelineCardProp if (rejectedPrograms.length > 0) { statusGroups.push({ status: 'rejected', - label: t('cards.subsidyPipeline.statuses.rejected')!, count: rejectedPrograms.length, totalFixedReduction: 0, hasUpcomingDeadline: false, - badgeClass: styles.badgeRed!, }); } @@ -100,9 +109,7 @@ export function SubsidyPipelineCard({ subsidyPrograms }: SubsidyPipelineCardProp
    {statusGroups.map((group) => (
  • - - {group.label} - + {group.count}{' '} {t(`cards.subsidyPipeline.program_${group.count === 1 ? 'one' : 'other'}`)} diff --git a/client/src/components/TimelineStatusCards/CriticalPathCard.tsx b/client/src/components/TimelineStatusCards/CriticalPathCard.tsx index b692de49a..302df72f2 100644 --- a/client/src/components/TimelineStatusCards/CriticalPathCard.tsx +++ b/client/src/components/TimelineStatusCards/CriticalPathCard.tsx @@ -3,6 +3,8 @@ import { Link } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; import type { TimelineWorkItem } from '@cornerstone/shared'; import { useFormatters } from '../../lib/formatters.js'; +import { Badge, type BadgeVariantMap } from '../Badge/Badge.js'; +import badgeStyles from '../Badge/Badge.module.css'; import styles from './TimelineStatusCards.module.css'; interface CriticalPathCardProps { @@ -40,6 +42,39 @@ export function CriticalPathCard({ criticalPath, workItems }: CriticalPathCardPr return Math.ceil(diff / (1000 * 60 * 60 * 24)); }, [deadline]); + type HealthStatus = 'onTrack' | 'warning' | 'critical' | 'overdue'; + + let status: HealthStatus = 'onTrack'; + if (daysRemaining < 0) { + status = 'overdue'; + } else if (daysRemaining < 7) { + status = 'critical'; + } else if (daysRemaining <= 14) { + status = 'warning'; + } + + const healthVariants = useMemo( + (): BadgeVariantMap => ({ + onTrack: { + label: t('cards.criticalPath.health.onTrack'), + className: badgeStyles.scheduleOnTrack!, + }, + warning: { + label: t('cards.criticalPath.health.warning'), + className: badgeStyles.scheduleWarning!, + }, + critical: { + label: t('cards.criticalPath.health.critical'), + className: badgeStyles.scheduleAtRisk!, + }, + overdue: { + label: t('cards.criticalPath.health.overdue'), + className: badgeStyles.scheduleAtRisk!, + }, + }), + [t], + ); + if (criticalItems.length === 0) { return (

    @@ -56,20 +91,6 @@ export function CriticalPathCard({ criticalPath, workItems }: CriticalPathCardPr ); } - // Determine health indicator - let healthClass = styles.badgeGreen; // green >14 days - let healthLabel = t('cards.criticalPath.health.onTrack'); - if (daysRemaining < 0) { - healthClass = styles.badgeRed; // red: overdue - healthLabel = t('cards.criticalPath.health.overdue'); - } else if (daysRemaining < 7) { - healthClass = styles.badgeRed; // red <7 days - healthLabel = t('cards.criticalPath.health.critical'); - } else if (daysRemaining <= 14) { - healthClass = styles.badgeYellow; // yellow 7-14 days - healthLabel = t('cards.criticalPath.health.warning'); - } - return (

    @@ -110,9 +131,7 @@ export function CriticalPathCard({ criticalPath, workItems }: CriticalPathCardPr {daysRemaining}
    -
    - {healthLabel} -
    +
); diff --git a/client/src/components/TimelineStatusCards/TimelineStatusCards.module.css b/client/src/components/TimelineStatusCards/TimelineStatusCards.module.css index 311c3f4ab..b0a7a1ef3 100644 --- a/client/src/components/TimelineStatusCards/TimelineStatusCards.module.css +++ b/client/src/components/TimelineStatusCards/TimelineStatusCards.module.css @@ -42,27 +42,6 @@ color: var(--color-text-primary); } -.badge { - display: inline-flex; - align-items: center; - padding: var(--spacing-1) var(--spacing-2); - border-radius: var(--radius-full); - font-size: var(--font-size-xs); - font-weight: var(--font-weight-medium); - white-space: nowrap; - flex-shrink: 0; -} - -.badgeGreen { - background-color: var(--color-success-bg); - color: var(--color-success-text-on-light); -} - -.badgeRed { - background-color: var(--color-danger-bg); - color: var(--color-danger-text-on-light); -} - .emptyState { margin: 0; font-size: var(--font-size-sm); @@ -85,12 +64,6 @@ box-shadow: var(--shadow-focus); } -/* Badge variant for health indicator with dynamic background color */ -.badgeYellow { - background-color: var(--color-warning-bg); - color: var(--color-text-secondary); -} - /* Legend for donut chart */ .legend { display: flex; diff --git a/client/src/components/TimelineStatusCards/UpcomingMilestonesCard.tsx b/client/src/components/TimelineStatusCards/UpcomingMilestonesCard.tsx index c876757a2..7b3c64ef6 100644 --- a/client/src/components/TimelineStatusCards/UpcomingMilestonesCard.tsx +++ b/client/src/components/TimelineStatusCards/UpcomingMilestonesCard.tsx @@ -1,7 +1,10 @@ +import { useMemo } from 'react'; import { Link } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; import type { TimelineMilestone } from '@cornerstone/shared'; import { useFormatters } from '../../lib/formatters.js'; +import { Badge, type BadgeVariantMap } from '../Badge/Badge.js'; +import badgeStyles from '../Badge/Badge.module.css'; import styles from './TimelineStatusCards.module.css'; interface UpcomingMilestonesCardProps { @@ -18,6 +21,20 @@ export function UpcomingMilestonesCard({ milestones }: UpcomingMilestonesCardPro .sort((a, b) => a.targetDate.localeCompare(b.targetDate)) .slice(0, 5); + const healthVariants = useMemo( + (): BadgeVariantMap => ({ + onTrack: { + label: t('cards.upcomingMilestones.onTrack'), + className: badgeStyles.scheduleOnTrack!, + }, + delayed: { + label: t('cards.upcomingMilestones.delayed'), + className: badgeStyles.scheduleAtRisk!, + }, + }), + [t], + ); + if (upcoming.length === 0) { return (

@@ -33,9 +50,6 @@ export function UpcomingMilestonesCard({ milestones }: UpcomingMilestonesCardPro // Determine health: "On Track" if projectedDate <= targetDate or no projectedDate const isOnTrack = !milestone.projectedDate || milestone.projectedDate <= milestone.targetDate; - const healthText = isOnTrack - ? t('cards.upcomingMilestones.onTrack') - : t('cards.upcomingMilestones.delayed'); return (

  • @@ -46,12 +60,11 @@ export function UpcomingMilestonesCard({ milestones }: UpcomingMilestonesCardPro {formatDate(milestone.targetDate)} - - {healthText} - +
    • ); diff --git a/client/src/components/documents/DocumentBrowser.tsx b/client/src/components/documents/DocumentBrowser.tsx index ef8b94b34..3c4091ae1 100644 --- a/client/src/components/documents/DocumentBrowser.tsx +++ b/client/src/components/documents/DocumentBrowser.tsx @@ -2,6 +2,7 @@ import { useState, useRef, useEffect } from 'react'; import { useTranslation } from 'react-i18next'; import type { PaperlessDocumentSearchResult } from '@cornerstone/shared'; import { usePaperless } from '../../hooks/usePaperless.js'; +import { useDebounce } from '../../hooks/useDebounce.js'; import { DocumentCard } from './DocumentCard.js'; import { DocumentDetailPanel } from './DocumentDetailPanel.js'; import { DocumentSkeleton } from './DocumentSkeleton.js'; @@ -39,7 +40,6 @@ export function DocumentBrowser({ const [selectedDoc, setSelectedDoc] = useState(null); const [searchInput, setSearchInput] = useState(''); const [hideLinked, setHideLinked] = useState(defaultHideLinked); - const searchDebounceRef = useRef | null>(null); // Forward correspondent prop changes to the hook useEffect(() => { @@ -47,17 +47,18 @@ export function DocumentBrowser({ // eslint-disable-next-line @eslint-react/exhaustive-deps -- setCorrespondent is stable callback; only depend on correspondentId to trigger filter updates }, [correspondentId]); - // Debounced search — intentionally omits hook.search from dep array to prevent infinite loop + // Debounced search + const debouncedSearchInput = useDebounce(searchInput, 300); + const isFirstSearchRunRef = useRef(true); + useEffect(() => { - if (searchDebounceRef.current) clearTimeout(searchDebounceRef.current); - searchDebounceRef.current = setTimeout(() => { - hook.search(searchInput); - }, 300); - return () => { - if (searchDebounceRef.current) clearTimeout(searchDebounceRef.current); - }; + if (isFirstSearchRunRef.current) { + isFirstSearchRunRef.current = false; + return; + } + hook.search(debouncedSearchInput); // eslint-disable-next-line @eslint-react/exhaustive-deps -- hook is not directly used; it's queried via ref to avoid re-running on every hook change - }, [searchInput]); + }, [debouncedSearchInput]); const handleCardSelect = (doc: PaperlessDocumentSearchResult) => { if (onSelect) { diff --git a/client/src/components/photos/PhotoMetadataModal.test.tsx b/client/src/components/photos/PhotoMetadataModal.test.tsx index 237e2474f..c2baf2e87 100644 --- a/client/src/components/photos/PhotoMetadataModal.test.tsx +++ b/client/src/components/photos/PhotoMetadataModal.test.tsx @@ -484,77 +484,4 @@ describe('PhotoMetadataModal', () => { }); }); }); - - describe('focus trap (useEffect keyboard handler)', () => { - // The PhotoMetadataModal focus trap queries [role="dialog"] to find focusable elements. - // The real Modal (portal to document.body) renders role="dialog" correctly. - // With real Modal + only a textarea inside formBody, the textarea is focusable. - // When first === last, Tab and Shift+Tab wrap focus back to the same element. - - it('Tab on the only focusable element wraps focus back to itself', async () => { - renderModal(); - - // Wait for the modal to appear in the DOM - await waitFor(() => { - expect(document.querySelector('[role="dialog"]')).toBeInTheDocument(); - }); - - const textarea = document.getElementById('modal-photo-caption') as HTMLTextAreaElement; - textarea.focus(); - expect(document.activeElement).toBe(textarea); - - // With real Modal: [role="dialog"] contains the textarea + buttons. - // The focus trap inside PhotoMetadataModal queries [role="dialog"] selectors - // to find focusables. When there are multiple focusable elements (buttons etc.), - // Tab on the LAST element wraps to the first. Since textarea is not last here, - // the Tab event may not trigger the wrap branch. Just verify no error is thrown. - expect(() => { - fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); - }).not.toThrow(); - }); - - it('Shift+Tab wraps focus within the dialog without errors', async () => { - renderModal(); - - await waitFor(() => { - expect(document.querySelector('[role="dialog"]')).toBeInTheDocument(); - }); - - const textarea = document.getElementById('modal-photo-caption') as HTMLTextAreaElement; - textarea.focus(); - expect(document.activeElement).toBe(textarea); - - expect(() => { - fireEvent.keyDown(document, { key: 'Tab', shiftKey: true }); - }).not.toThrow(); - }); - - it('focus trap does not trigger on non-Tab keys', async () => { - renderModal(); - - await waitFor(() => { - expect(document.querySelector('[role="dialog"]')).toBeInTheDocument(); - }); - - const textarea = document.getElementById('modal-photo-caption') as HTMLTextAreaElement; - textarea.focus(); - - // Should not throw or change focus to a different element - expect(() => { - fireEvent.keyDown(document, { key: 'Enter', shiftKey: false }); - }).not.toThrow(); - }); - - it('focus trap does nothing when no focusable elements exist inside the dialog', async () => { - renderModal(); - - await waitFor(() => { - expect(document.querySelector('[role="dialog"]')).toBeInTheDocument(); - }); - - expect(() => { - fireEvent.keyDown(document, { key: 'Tab', shiftKey: false }); - }).not.toThrow(); - }); - }); }); diff --git a/client/src/components/photos/PhotoMetadataModal.tsx b/client/src/components/photos/PhotoMetadataModal.tsx index b1b2047ac..737b14290 100644 --- a/client/src/components/photos/PhotoMetadataModal.tsx +++ b/client/src/components/photos/PhotoMetadataModal.tsx @@ -38,36 +38,6 @@ export function PhotoMetadataModal({ return () => URL.revokeObjectURL(url); }, [file]); - // Focus trap: cycle Tab/Shift+Tab within the modal (form + footer buttons) - useEffect(() => { - const handleKeyDown = (e: KeyboardEvent) => { - if (e.key !== 'Tab') return; - - // Find all focusable elements in the entire modal (form + footer buttons) - const allFocusable = Array.from( - document.querySelectorAll( - '[role="dialog"] button, [role="dialog"] [href], [role="dialog"] input, [role="dialog"] select, [role="dialog"] textarea, [role="dialog"] [tabindex]:not([tabindex="-1"])', - ), - ).filter((el) => !el.hasAttribute('disabled')); - - if (allFocusable.length === 0) return; - - const first = allFocusable[0]!; - const last = allFocusable[allFocusable.length - 1]!; - - if (e.shiftKey && document.activeElement === first) { - e.preventDefault(); - last.focus(); - } else if (!e.shiftKey && document.activeElement === last) { - e.preventDefault(); - first.focus(); - } - }; - - document.addEventListener('keydown', handleKeyDown); - return () => document.removeEventListener('keydown', handleKeyDown); - }, []); - const handleSave = () => { onSave({ caption: caption || null, diff --git a/client/src/hooks/useClickOutside.test.ts b/client/src/hooks/useClickOutside.test.ts new file mode 100644 index 000000000..b4cd1ab8b --- /dev/null +++ b/client/src/hooks/useClickOutside.test.ts @@ -0,0 +1,164 @@ +/** + * @jest-environment jsdom + */ +import { jest, describe, it, expect, afterEach } from '@jest/globals'; +import { createElement, useRef } from 'react'; +import type { RefObject } from 'react'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import { useClickOutside } from './useClickOutside.js'; +import type { ClickOutsideTarget } from './useClickOutside.js'; + +interface HarnessProps { + extraTargets?: ClickOutsideTarget[]; + onClickOutside: (event: MouseEvent) => void; + enabled?: boolean; +} + +// No JSX here — this file keeps the `.test.ts` extension (matching the hook's +// own `.ts` extension), and `.ts` files cannot contain JSX syntax. Use +// React.createElement directly instead. +function Harness({ extraTargets = [], onClickOutside, enabled }: HarnessProps) { + const insideRef = useRef(null); + const secondInsideRef = useRef(null); + + useClickOutside([insideRef, secondInsideRef, ...extraTargets], onClickOutside, enabled); + + return createElement( + 'div', + null, + createElement('div', { 'data-testid': 'inside', ref: insideRef }, 'Inside'), + createElement('div', { 'data-testid': 'second-inside', ref: secondInsideRef }, 'Second Inside'), + createElement('div', { 'data-testid': 'outside' }, 'Outside'), + ); +} + +function renderHarness(props: HarnessProps) { + return render(createElement(Harness, props)); +} + +describe('useClickOutside', () => { + afterEach(() => { + cleanup(); + }); + + it('fires the handler on a mousedown outside all targets', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + renderHarness({ onClickOutside: handler }); + + fireEvent.mouseDown(screen.getByTestId('outside')); + + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('does not fire the handler on a mousedown inside one of multiple targets', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + renderHarness({ onClickOutside: handler }); + + fireEvent.mouseDown(screen.getByTestId('inside')); + expect(handler).not.toHaveBeenCalled(); + + fireEvent.mouseDown(screen.getByTestId('second-inside')); + expect(handler).not.toHaveBeenCalled(); + }); + + it('accepts both RefObject and raw HTMLElement targets in the same array', () => { + const raw = document.createElement('div'); + raw.setAttribute('data-testid', 'raw-target'); + document.body.appendChild(raw); + + const handler = jest.fn<(event: MouseEvent) => void>(); + renderHarness({ onClickOutside: handler, extraTargets: [raw] }); + + // Click on the raw (non-ref) element — should NOT count as outside. + fireEvent.mouseDown(raw); + expect(handler).not.toHaveBeenCalled(); + + // Click on the ref-based inside element — should also NOT count as outside. + fireEvent.mouseDown(screen.getByTestId('inside')); + expect(handler).not.toHaveBeenCalled(); + + // Click elsewhere — should count as outside. + fireEvent.mouseDown(screen.getByTestId('outside')); + expect(handler).toHaveBeenCalledTimes(1); + + document.body.removeChild(raw); + }); + + it('treats null/undefined targets in the array as inert (does not throw, does not block outside detection)', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + const nullRef: RefObject = { current: null }; + + renderHarness({ onClickOutside: handler, extraTargets: [nullRef, null, undefined] }); + + expect(() => { + fireEvent.mouseDown(screen.getByTestId('outside')); + }).not.toThrow(); + + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('enabled=false: no listener is attached — handler never fires even for an outside click', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + renderHarness({ onClickOutside: handler, enabled: false }); + + fireEvent.mouseDown(screen.getByTestId('outside')); + + expect(handler).not.toHaveBeenCalled(); + }); + + it('toggling enabled from false to true attaches the listener without needing a remount', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + const { rerender } = renderHarness({ onClickOutside: handler, enabled: false }); + + fireEvent.mouseDown(screen.getByTestId('outside')); + expect(handler).not.toHaveBeenCalled(); + + rerender(createElement(Harness, { onClickOutside: handler, enabled: true })); + + fireEvent.mouseDown(screen.getByTestId('outside')); + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('toggling enabled from true to false detaches the listener', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + const { rerender } = renderHarness({ onClickOutside: handler, enabled: true }); + + rerender(createElement(Harness, { onClickOutside: handler, enabled: false })); + + fireEvent.mouseDown(screen.getByTestId('outside')); + expect(handler).not.toHaveBeenCalled(); + }); + + it('defaults enabled to true when the parameter is omitted', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + renderHarness({ onClickOutside: handler }); + + fireEvent.mouseDown(screen.getByTestId('outside')); + + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('always invokes the latest handler passed in, even after a re-render with a new handler', () => { + const firstHandler = jest.fn<(event: MouseEvent) => void>(); + const secondHandler = jest.fn<(event: MouseEvent) => void>(); + const { rerender } = renderHarness({ onClickOutside: firstHandler }); + + rerender(createElement(Harness, { onClickOutside: secondHandler })); + + fireEvent.mouseDown(screen.getByTestId('outside')); + + expect(firstHandler).not.toHaveBeenCalled(); + expect(secondHandler).toHaveBeenCalledTimes(1); + }); + + it('cleans up the mousedown listener on unmount', () => { + const handler = jest.fn<(event: MouseEvent) => void>(); + const { unmount } = renderHarness({ onClickOutside: handler }); + + unmount(); + + fireEvent.mouseDown(document.body); + + expect(handler).not.toHaveBeenCalled(); + }); +}); diff --git a/client/src/hooks/useClickOutside.ts b/client/src/hooks/useClickOutside.ts new file mode 100644 index 000000000..2daa105f9 --- /dev/null +++ b/client/src/hooks/useClickOutside.ts @@ -0,0 +1,38 @@ +import { useEffect, useRef } from 'react'; +import type { RefObject } from 'react'; + +export type ClickOutsideTarget = RefObject | HTMLElement | null | undefined; + +/** + * Invokes `onClickOutside` on a `mousedown` that lands outside all of the given `targets`. + * Accepts a mix of ref objects and raw DOM elements so call sites with dynamically + * resolved elements (e.g. a floating-ui portal ref) can participate without extra wrapping. + */ +export function useClickOutside( + targets: ClickOutsideTarget[], + onClickOutside: (event: MouseEvent) => void, + enabled = true, +): void { + const targetsRef = useRef(targets); + targetsRef.current = targets; + const handlerRef = useRef(onClickOutside); + handlerRef.current = onClickOutside; + + useEffect(() => { + if (!enabled) return; + + function handleMouseDown(event: MouseEvent) { + const node = event.target as Node; + const isInside = targetsRef.current.some((target) => { + const el = target instanceof HTMLElement ? target : (target?.current ?? null); + return el ? el.contains(node) : false; + }); + if (!isInside) { + handlerRef.current(event); + } + } + + document.addEventListener('mousedown', handleMouseDown); + return () => document.removeEventListener('mousedown', handleMouseDown); + }, [enabled]); +} diff --git a/client/src/hooks/useDebounce.test.ts b/client/src/hooks/useDebounce.test.ts new file mode 100644 index 000000000..4c8652862 --- /dev/null +++ b/client/src/hooks/useDebounce.test.ts @@ -0,0 +1,126 @@ +/** + * @jest-environment jsdom + */ +import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; +import { renderHook, act } from '@testing-library/react'; +import { useDebounce } from './useDebounce.js'; + +describe('useDebounce', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('returns the initial value synchronously', () => { + const { result } = renderHook(() => useDebounce('initial', 300)); + + expect(result.current).toBe('initial'); + }); + + it('does not update the returned value before delayMs elapses', () => { + const { result, rerender } = renderHook(({ value }) => useDebounce(value, 300), { + initialProps: { value: 'initial' }, + }); + + rerender({ value: 'updated' }); + + act(() => { + jest.advanceTimersByTime(299); + }); + + expect(result.current).toBe('initial'); + }); + + it('updates to the new value after delayMs elapses', () => { + const { result, rerender } = renderHook(({ value }) => useDebounce(value, 300), { + initialProps: { value: 'initial' }, + }); + + rerender({ value: 'updated' }); + + act(() => { + jest.advanceTimersByTime(300); + }); + + expect(result.current).toBe('updated'); + }); + + it('resets the timer on rapid successive changes — only the last value wins', () => { + const { result, rerender } = renderHook(({ value }) => useDebounce(value, 300), { + initialProps: { value: 'a' }, + }); + + rerender({ value: 'b' }); + act(() => { + jest.advanceTimersByTime(200); + }); + // Not yet elapsed for 'b' + expect(result.current).toBe('a'); + + rerender({ value: 'c' }); + act(() => { + jest.advanceTimersByTime(200); + }); + // 'b'’s timer was cancelled; 'c'’s timer has only run 200ms of 300ms + expect(result.current).toBe('a'); + + act(() => { + jest.advanceTimersByTime(100); + }); + // 'c'’s full 300ms has now elapsed + expect(result.current).toBe('c'); + }); + + it('cleans up the pending timeout on unmount (no post-unmount state update)', () => { + const { rerender, unmount } = renderHook(({ value }) => useDebounce(value, 300), { + initialProps: { value: 'a' }, + }); + + rerender({ value: 'b' }); + unmount(); + + // Advancing timers after unmount must not throw or trigger an act() warning + // (React would warn if setState were called on an unmounted component). + expect(() => { + act(() => { + jest.advanceTimersByTime(300); + }); + }).not.toThrow(); + }); + + it('supports non-string generic values (numbers)', () => { + const { result, rerender } = renderHook(({ value }) => useDebounce(value, 100), { + initialProps: { value: 1 }, + }); + + rerender({ value: 2 }); + + act(() => { + jest.advanceTimersByTime(100); + }); + + expect(result.current).toBe(2); + }); + + it('restarts the timer when delayMs changes even if value stays the same', () => { + const { result, rerender } = renderHook(({ value, delay }) => useDebounce(value, delay), { + initialProps: { value: 'a', delay: 300 }, + }); + + rerender({ value: 'b', delay: 500 }); + + act(() => { + jest.advanceTimersByTime(300); + }); + // Delay is now 500ms, so 300ms is not enough + expect(result.current).toBe('a'); + + act(() => { + jest.advanceTimersByTime(200); + }); + expect(result.current).toBe('b'); + }); +}); diff --git a/client/src/hooks/useDebounce.ts b/client/src/hooks/useDebounce.ts new file mode 100644 index 000000000..ef016d905 --- /dev/null +++ b/client/src/hooks/useDebounce.ts @@ -0,0 +1,15 @@ +import { useState, useEffect } from 'react'; + +export function useDebounce(value: T, delayMs: number): T { + const [debouncedValue, setDebouncedValue] = useState(value); + + useEffect(() => { + const timeoutId = setTimeout(() => { + setDebouncedValue(value); + }, delayMs); + + return () => clearTimeout(timeoutId); + }, [value, delayMs]); + + return debouncedValue; +} diff --git a/client/src/hooks/useDebouncedCallback.test.ts b/client/src/hooks/useDebouncedCallback.test.ts new file mode 100644 index 000000000..f0ca68396 --- /dev/null +++ b/client/src/hooks/useDebouncedCallback.test.ts @@ -0,0 +1,179 @@ +/** + * @jest-environment jsdom + */ +import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; +import { renderHook, act } from '@testing-library/react'; +import { useDebouncedCallback } from './useDebouncedCallback.js'; + +describe('useDebouncedCallback', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('trigger() schedules callback after delayMs and does not call it immediately', () => { + const callback = jest.fn<(x: string) => void>(); + const { result } = renderHook(() => useDebouncedCallback(callback, 300)); + + act(() => { + result.current.trigger('hello'); + }); + + expect(callback).not.toHaveBeenCalled(); + + act(() => { + jest.advanceTimersByTime(300); + }); + + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith('hello'); + }); + + it('calling trigger() again before the delay elapses cancels and reschedules — only the last args win', () => { + const callback = jest.fn<(x: string) => void>(); + const { result } = renderHook(() => useDebouncedCallback(callback, 300)); + + act(() => { + result.current.trigger('first'); + }); + act(() => { + jest.advanceTimersByTime(200); + }); + expect(callback).not.toHaveBeenCalled(); + + act(() => { + result.current.trigger('second'); + }); + act(() => { + jest.advanceTimersByTime(200); + }); + // Only 200ms of the second call's 300ms window has elapsed + expect(callback).not.toHaveBeenCalled(); + + act(() => { + jest.advanceTimersByTime(100); + }); + + expect(callback).toHaveBeenCalledTimes(1); + expect(callback).toHaveBeenCalledWith('second'); + }); + + it('cancel() prevents a pending call from firing', () => { + const callback = jest.fn<() => void>(); + const { result } = renderHook(() => useDebouncedCallback(callback, 300)); + + act(() => { + result.current.trigger(); + }); + act(() => { + result.current.cancel(); + }); + act(() => { + jest.advanceTimersByTime(300); + }); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('cancel() is a no-op when there is no pending call', () => { + const callback = jest.fn<() => void>(); + const { result } = renderHook(() => useDebouncedCallback(callback, 300)); + + expect(() => { + act(() => { + result.current.cancel(); + }); + }).not.toThrow(); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('unmounting while a call is pending does not fire it', () => { + const callback = jest.fn<() => void>(); + const { result, unmount } = renderHook(() => useDebouncedCallback(callback, 300)); + + act(() => { + result.current.trigger(); + }); + + unmount(); + + act(() => { + jest.advanceTimersByTime(300); + }); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('trigger and cancel are referentially stable across re-renders with the same delayMs', () => { + const callback = jest.fn<() => void>(); + const { result, rerender } = renderHook( + ({ delay }: { delay: number }) => useDebouncedCallback(callback, delay), + { initialProps: { delay: 300 } }, + ); + + const firstTrigger = result.current.trigger; + const firstCancel = result.current.cancel; + + rerender({ delay: 300 }); + + expect(result.current.trigger).toBe(firstTrigger); + expect(result.current.cancel).toBe(firstCancel); + }); + + it('trigger changes reference when delayMs changes (cancel stays stable)', () => { + const callback = jest.fn<() => void>(); + const { result, rerender } = renderHook( + ({ delay }: { delay: number }) => useDebouncedCallback(callback, delay), + { initialProps: { delay: 300 } }, + ); + + const firstTrigger = result.current.trigger; + const firstCancel = result.current.cancel; + + rerender({ delay: 500 }); + + expect(result.current.trigger).not.toBe(firstTrigger); + expect(result.current.cancel).toBe(firstCancel); + }); + + it('always calls the latest callback even if the callback identity changes between trigger and fire', () => { + const firstCallback = jest.fn<() => void>(); + const secondCallback = jest.fn<() => void>(); + const { result, rerender } = renderHook( + ({ cb }: { cb: () => void }) => useDebouncedCallback(cb, 300), + { initialProps: { cb: firstCallback } }, + ); + + act(() => { + result.current.trigger(); + }); + + // Swap the callback before the timer fires (e.g. a closure over updated state) + rerender({ cb: secondCallback }); + + act(() => { + jest.advanceTimersByTime(300); + }); + + expect(firstCallback).not.toHaveBeenCalled(); + expect(secondCallback).toHaveBeenCalledTimes(1); + }); + + it('supports multiple arguments passed through to the callback', () => { + const callback = jest.fn<(a: number, b: string) => void>(); + const { result } = renderHook(() => useDebouncedCallback(callback, 100)); + + act(() => { + result.current.trigger(42, 'answer'); + }); + act(() => { + jest.advanceTimersByTime(100); + }); + + expect(callback).toHaveBeenCalledWith(42, 'answer'); + }); +}); diff --git a/client/src/hooks/useDebouncedCallback.ts b/client/src/hooks/useDebouncedCallback.ts new file mode 100644 index 000000000..c3cba1161 --- /dev/null +++ b/client/src/hooks/useDebouncedCallback.ts @@ -0,0 +1,31 @@ +import { useCallback, useEffect, useRef } from 'react'; + +export function useDebouncedCallback( + callback: (...args: Args) => void, + delayMs: number, +): { trigger: (...args: Args) => void; cancel: () => void } { + const callbackRef = useRef(callback); + callbackRef.current = callback; + const timeoutRef = useRef | null>(null); + + const cancel = useCallback(() => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + timeoutRef.current = null; + } + }, []); + + const trigger = useCallback( + (...args: Args) => { + cancel(); + timeoutRef.current = setTimeout(() => { + callbackRef.current(...args); + }, delayMs); + }, + [delayMs, cancel], + ); + + useEffect(() => cancel, [cancel]); + + return { trigger, cancel }; +} diff --git a/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.tsx b/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.tsx index 89b2dde4f..88b46cf08 100644 --- a/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.tsx +++ b/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.tsx @@ -5,6 +5,7 @@ import type { BudgetOverview, BudgetBreakdown, BudgetSource } from '@cornerstone import { fetchBudgetOverview, fetchBudgetBreakdown } from '../../lib/budgetOverviewApi.js'; import { fetchBudgetSources } from '../../lib/budgetSourcesApi.js'; import { ApiClientError } from '../../lib/apiClient.js'; +import { useDebouncedCallback } from '../../hooks/useDebouncedCallback.js'; import { PageLayout } from '../../components/PageLayout/PageLayout.js'; import { SubNav, type SubNavTab } from '../../components/SubNav/SubNav.js'; import { CostBreakdownTable } from '../../components/CostBreakdownTable/CostBreakdownTable.js'; @@ -40,9 +41,8 @@ export function BudgetOverviewPage() { const [isBreakdownRefetching, setIsBreakdownRefetching] = useState(false); const [breakdownError, setBreakdownError] = useState(''); - // Refs for debounce + AbortController + // Ref for AbortController const abortRef = useRef(null); - const debounceRef = useRef | null>(null); // Budget sources state const [_budgetSources, setBudgetSources] = useState([]); @@ -114,8 +114,8 @@ export function BudgetOverviewPage() { [setSearchParams], ); - // Standalone fetch function for debounced refetch - const fetchBreakdown = useCallback( + // Standalone fetch function + const fetchBreakdownImpl = useCallback( async (sourceIds: Set, signal?: AbortSignal) => { const deselectedArray = sourceIds.size > 0 ? [...sourceIds] : undefined; try { @@ -136,31 +136,28 @@ export function BudgetOverviewPage() { // Debounced refetch on deselectedSourceIds change const DEBOUNCE_MS = 50; - useEffect(() => { - // 1. Clear any pending debounced fetch - if (debounceRef.current) clearTimeout(debounceRef.current); + const scheduleFetchBreakdown = useDebouncedCallback((sourceIds: Set) => { + const controller = new AbortController(); + abortRef.current = controller; + void fetchBreakdownImpl(sourceIds, controller.signal); + }, DEBOUNCE_MS); - // 2. Abort any in-flight fetch + useEffect(() => { + // Abort any in-flight fetch if (abortRef.current) abortRef.current.abort(); // Only trigger refetch after initial load completes if (isLoading) return; - // 3. Schedule new fetch after debounce window /* eslint-disable @eslint-react/set-state-in-effect -- initializing refetch state for debounced operation */ setIsBreakdownRefetching(true); /* eslint-enable @eslint-react/set-state-in-effect */ - debounceRef.current = setTimeout(() => { - const controller = new AbortController(); - abortRef.current = controller; - void fetchBreakdown(deselectedSourceIds, controller.signal); - }, DEBOUNCE_MS); + scheduleFetchBreakdown.trigger(deselectedSourceIds); return () => { - if (debounceRef.current) clearTimeout(debounceRef.current); if (abortRef.current) abortRef.current.abort(); }; - }, [deselectedSourceIds, paymentStatus, isLoading, fetchBreakdown]); + }, [deselectedSourceIds, paymentStatus, isLoading, scheduleFetchBreakdown]); // Close dropdown on outside click useEffect(() => { diff --git a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx index 9d9ad537a..e03201eff 100644 --- a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx +++ b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx @@ -2,7 +2,7 @@ * @jest-environment jsdom */ import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; -import { render, screen, waitFor, fireEvent, within } from '@testing-library/react'; +import { render, screen, waitFor, fireEvent, within, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { MemoryRouter, Routes, Route, useLocation } from 'react-router-dom'; import type * as DiaryApiTypes from '../../lib/diaryApi.js'; @@ -44,13 +44,22 @@ jest.unstable_mockModule('../../hooks/usePhotos.js', () => ({ }), })); -// Mock PhotoUpload to capture its onUpload prop so tests can invoke it directly. -// The real PhotoUpload uses XHR/FormData which are not available in jsdom. +// Mock PhotoUpload to capture its onUpload and onUploadingCountChange props so +// tests can invoke them directly. The real PhotoUpload uses XHR/FormData which +// are not available in jsdom. let capturedOnUpload: ((photo: Photo) => void) | null = null; +let capturedOnUploadingCountChange: ((count: number) => void) | null = null; jest.unstable_mockModule('../../components/photos/PhotoUpload.js', () => ({ - PhotoUpload: ({ onUpload }: { onUpload: (photo: Photo) => void }) => { + PhotoUpload: ({ + onUpload, + onUploadingCountChange, + }: { + onUpload: (photo: Photo) => void; + onUploadingCountChange?: (count: number) => void; + }) => { capturedOnUpload = onUpload; + capturedOnUploadingCountChange = onUploadingCountChange ?? null; return
    ; }, })); @@ -786,6 +795,52 @@ describe('DiaryEntryEditPage', () => { jest.useRealTimers(); }); + it('Scenario 44b: uploadingCount change while an autosave is pending cancels the debounced save (scheduleAutoSave.cancel via the uploadingCount-keyed cleanup effect)', async () => { + jest.useFakeTimers(); + mockGetDiaryEntry.mockResolvedValueOnce(draftGeneralNoteEntry); + mockUpdateDiaryEntry.mockResolvedValue({ ...draftGeneralNoteEntry, body: 'updated' }); + renderEditPage('draft-1'); + + await waitFor(() => { + expect(screen.getByRole('textbox', { name: /^entry/i })).toBeInTheDocument(); + }); + + // PhotoUpload only renders once `entry` has loaded — by this point it has. + expect(capturedOnUploadingCountChange).not.toBeNull(); + + // NOTE: mounting a draft entry fires one immediate autosave call on its own + // (pre-existing `skipAutoSaveOnMountRef` behavior in the metadata-change + // effect, unrelated to this PR's debounce-hook migration — see CODE_BUG + // note in the PR description). Flush and discard that call so this test + // isolates the debounce-cancel behavior under test. + await jest.advanceTimersByTimeAsync(50); + mockUpdateDiaryEntry.mockClear(); + + const textarea = screen.getByRole('textbox', { name: /^entry/i }); + fireEvent.change(textarea, { target: { value: 'updated body' } }); + fireEvent.blur(textarea); + + // A debounced autosave (1000ms) is now pending. Advance partway — not + // enough to fire — then simulate a photo upload starting. The + // `uploadingCount`-keyed effect's cleanup calls `scheduleAutoSave.cancel()` + // every time `uploadingCount` changes (not just on unmount), which must + // cancel the pending debounced save. + await jest.advanceTimersByTimeAsync(500); + expect(mockUpdateDiaryEntry).not.toHaveBeenCalled(); + + act(() => { + capturedOnUploadingCountChange!(1); + }); + + // Advance well past the original 1000ms debounce window — if the pending + // save were NOT cancelled, updateDiaryEntry would have fired by now. + await jest.advanceTimersByTimeAsync(1100); + + expect(mockUpdateDiaryEntry).not.toHaveBeenCalled(); + + jest.useRealTimers(); + }); + it('Scenario 46: weather select change on draft → triggers immediate auto-save', async () => { jest.useFakeTimers(); const draftDailyLogEntry: DiaryEntryDetail = { diff --git a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.tsx b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.tsx index 11e852ae4..978d10ca3 100644 --- a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.tsx +++ b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.tsx @@ -22,6 +22,7 @@ import { promoteDiaryEntry, } from '../../lib/diaryApi.js'; import { ApiClientError } from '../../lib/apiClient.js'; +import { useDebouncedCallback } from '../../hooks/useDebouncedCallback.js'; import { useToast } from '../../components/Toast/ToastContext.js'; import { useAuth } from '../../contexts/AuthContext.js'; import { fetchVendors } from '../../lib/vendorsApi.js'; @@ -77,7 +78,6 @@ export default function DiaryEntryEditPage() { // Auto-save state const [saveStatus, setSaveStatus] = useState('idle'); const autoSaveAbortRef = useRef(null); - const autoSaveDebounceRef = useRef | null>(null); const [uploadingCount, setUploadingCount] = useState(0); // Photo state @@ -154,6 +154,42 @@ export default function DiaryEntryEditPage() { void loadEntry(); }, [id, navigate, showToast, t]); + // Hoisted auto-save functions to enable hook usage before use + const doSaveImpl = async () => { + if (autoSaveAbortRef.current) { + autoSaveAbortRef.current.abort(); + } + + const controller = new AbortController(); + autoSaveAbortRef.current = controller; + + setSaveStatus('saving'); + + try { + const metadata = buildMetadata(); + await updateDiaryEntry(entry!.id, { + entryDate: entryDate || undefined, + title: title.trim() || null, + body: body || undefined, + metadata, + }); + + if (!controller.signal.aborted) { + setSaveStatus('saved'); + setTimeout(() => setSaveStatus('idle'), 3000); + } + } catch (err) { + if (!controller.signal.aborted) { + setSaveStatus('error'); + console.error('Failed to auto-save:', err); + } + } + }; + + const scheduleAutoSave = useDebouncedCallback(() => { + void doSaveImpl(); + }, 1000); + // Auto-save on metadata field changes for drafts useEffect(() => { if (skipAutoSaveOnMountRef.current) { @@ -199,11 +235,9 @@ export default function DiaryEntryEditPage() { if (autoSaveAbortRef.current) { autoSaveAbortRef.current.abort(); } - if (autoSaveDebounceRef.current) { - clearTimeout(autoSaveDebounceRef.current); - } + scheduleAutoSave.cancel(); }; - }, [uploadingCount]); + }, [uploadingCount, scheduleAutoSave]); // Delete modal: focus trap and Escape key handler useEffect(() => { @@ -388,46 +422,10 @@ export default function DiaryEntryEditPage() { return; } - const doSave = async () => { - if (autoSaveAbortRef.current) { - autoSaveAbortRef.current.abort(); - } - - const controller = new AbortController(); - autoSaveAbortRef.current = controller; - - setSaveStatus('saving'); - - try { - const metadata = buildMetadata(); - await updateDiaryEntry(entry.id, { - entryDate: entryDate || undefined, - title: title.trim() || null, - body: body || undefined, - metadata, - }); - - if (!controller.signal.aborted) { - setSaveStatus('saved'); - setTimeout(() => setSaveStatus('idle'), 3000); - } - } catch (err) { - if (!controller.signal.aborted) { - setSaveStatus('error'); - console.error('Failed to auto-save:', err); - } - } - }; - if (immediate) { - void doSave(); + void doSaveImpl(); } else { - if (autoSaveDebounceRef.current) { - clearTimeout(autoSaveDebounceRef.current); - } - autoSaveDebounceRef.current = setTimeout(() => { - void doSave(); - }, 1000); + scheduleAutoSave.trigger(); } }; diff --git a/client/src/pages/DiaryPage/DiaryPage.test.tsx b/client/src/pages/DiaryPage/DiaryPage.test.tsx index 518dbbf47..7e19e7c48 100644 --- a/client/src/pages/DiaryPage/DiaryPage.test.tsx +++ b/client/src/pages/DiaryPage/DiaryPage.test.tsx @@ -294,6 +294,47 @@ describe('DiaryPage', () => { }); }); + // ─── useDebounce migration (#1816): page param not reset on mount ───────── + // Regression test for the `isFirstSearchSync` guard around the debounced + // search-sync effect. Without it, mounting with both `q` and `page` in the + // URL would fire the search-sync effect on mount (since useDebounce returns + // its initial value synchronously) and reset `page` back to '1', discarding + // the user's pagination position on page load/refresh. + + it('does not reset the page URL param to 1 on initial mount when the URL has both q and page', async () => { + // NOTE: mounting with a `page` URL param already produces two fetches by + // design, unrelated to this guard: `currentPage` state initializes to 1, + // then a separate effect syncs it from the `page` URL param once `urlPage` + // is read, triggering a second fetch. That's pre-existing behavior. What + // this test guards against is a THIRD, spurious fetch/URL-rewrite from the + // debounced-search-sync effect resetting `page` back to '1' on mount + // (since useDebounce returns its initial value synchronously, that effect + // would otherwise treat the initial `q` value as a "change"). + mockListDiaryEntries.mockResolvedValue({ + items: [makeSummary('de-1')], + pagination: { page: 3, pageSize: 25, totalPages: 5, totalItems: 120 }, + }); + + render( + + + , + ); + + // Final rendered state must reflect page 3, not a reset to page 1. + await waitFor(() => { + expect(screen.getByText('Page 3 of 5')).toBeInTheDocument(); + }); + + // The last API call must have requested page 3 with the search query intact + // — if the isFirstSearchSync guard were missing, the debounced-search-sync + // effect would have rewritten the URL's `page` param back to '1' on mount, + // and this final call/render would show page 1 instead. + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.page).toBe(3); + expect(lastCall?.[0]?.q).toBe('foo'); + }); + // ─── Filter mode changes call API ────────────────────────────────────────── // ─── New Entry button ───────────────────────────────────────────────────── diff --git a/client/src/pages/DiaryPage/DiaryPage.tsx b/client/src/pages/DiaryPage/DiaryPage.tsx index f5be6e032..68905dc69 100644 --- a/client/src/pages/DiaryPage/DiaryPage.tsx +++ b/client/src/pages/DiaryPage/DiaryPage.tsx @@ -9,6 +9,7 @@ import type { } from '@cornerstone/shared'; import { listDiaryEntries } from '../../lib/diaryApi.js'; import { ApiClientError } from '../../lib/apiClient.js'; +import { useDebounce } from '../../hooks/useDebounce.js'; import { DiaryFilterBar } from '../../components/diary/DiaryFilterBar/DiaryFilterBar.js'; import { DiaryDateGroup } from '../../components/diary/DiaryDateGroup/DiaryDateGroup.js'; import shared from '../../styles/shared.module.css'; @@ -54,7 +55,6 @@ export default function DiaryPage() { const urlPage = parseInt(searchParams.get('page') || '1', 10); const [searchInput, setSearchInput] = useState(searchQuery); - const searchDebounceRef = useRef | null>(null); const announcementRef = useRef(null); /* eslint-disable @eslint-react/set-state-in-effect -- synchronously sync URL page to component state on page param change */ @@ -63,22 +63,24 @@ export default function DiaryPage() { }, [urlPage, currentPage]); /* eslint-enable @eslint-react/set-state-in-effect */ + // Debounced search with URL sync + const debouncedSearchInput = useDebounce(searchInput, 300); + const isFirstSearchSyncRef = useRef(true); + useEffect(() => { - if (searchDebounceRef.current) clearTimeout(searchDebounceRef.current); - searchDebounceRef.current = setTimeout(() => { - const newParams = new URLSearchParams(searchParams); - if (searchInput) { - newParams.set('q', searchInput); - } else { - newParams.delete('q'); - } - newParams.set('page', '1'); - setSearchParams(newParams); - }, 300); - return () => { - if (searchDebounceRef.current) clearTimeout(searchDebounceRef.current); - }; - }, [searchInput, searchParams, setSearchParams]); + if (isFirstSearchSyncRef.current) { + isFirstSearchSyncRef.current = false; + return; + } + const newParams = new URLSearchParams(searchParams); + if (debouncedSearchInput) { + newParams.set('q', debouncedSearchInput); + } else { + newParams.delete('q'); + } + newParams.set('page', '1'); + setSearchParams(newParams); + }, [debouncedSearchInput, searchParams, setSearchParams]); useEffect(() => { void loadEntries(); diff --git a/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.test.tsx b/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.test.tsx index 5cc34bdf2..2eff27586 100644 --- a/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.test.tsx +++ b/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.test.tsx @@ -610,9 +610,13 @@ describe('WorkItemDetailPage', () => { }); describe('subtasks display', () => { - it('shows empty state when no subtasks exist', async () => { + it('shows empty state when no subtasks exist (via the workItems:detail.subtasks.noSubtasks i18n key, not a hardcoded literal)', async () => { renderPage(); + // This text is now sourced from the `detail.subtasks.noSubtasks` translation + // key (client/src/i18n/en/workItems.json) rather than a hardcoded JSX literal. + // The English wording is unchanged, so the assertion string stays the same, + // but it now exercises the real i18n lookup rather than a static string. await waitFor(() => { expect(screen.getByText('No subtasks yet. Add one above.')).toBeInTheDocument(); }); diff --git a/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx b/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx index 570025b76..2f1428e9e 100644 --- a/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx +++ b/client/src/pages/WorkItemDetailPage/WorkItemDetailPage.tsx @@ -1666,7 +1666,7 @@ export default function WorkItemDetailPage() {
    {subtasks.length === 0 && ( -
    No subtasks yet. Add one above.
    +
    {t('detail.subtasks.noSubtasks')}
    )} {subtasks.map((subtask, index) => (
    From ff1c71bd592213d0c2046ab3157e969e27b38c24 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Wed, 8 Jul 2026 00:28:32 +0200 Subject: [PATCH 2/2] fix(client): memoize useDebouncedCallback return object to prevent effect re-fire loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useDebouncedCallback returned a brand-new `{ trigger, cancel }` object literal on every render, even though `trigger`/`cancel` were each individually stable via useCallback. Any consumer effect that depended on the whole returned object (not just its members) saw that dependency "change" on every render, causing the effect to re-fire spuriously. This was the deterministic root cause of the E2E smoke failure on tests/budget/budget-overview-no-hero-card.spec.ts:328 (PR #1848): in BudgetOverviewPage, the debounce-refetch effect depends on `scheduleFetchBreakdown`, so every render — including the render caused by the effect's own setIsBreakdownRefetching(true) — re-triggered the effect, producing an unbounded refetch ping-pong. isBreakdownRefetching never settled to false, so the `.breakdownRefetching` (pointer-events: none) overlay stayed applied over the breakdown table, and Playwright's click on "Expand work item budget by area" timed out with the ancestor PageLayout content wrapper reported as intercepting pointer events. The same pattern also silently cancelled pending debounced autosaves in DiaryEntryEditPage on any unrelated re-render. Fix: wrap the hook's return value in `useMemo(() => ({ trigger, cancel }), [trigger, cancel])` so identity is stable whenever trigger/cancel haven't changed. No page-level code changes were needed — the effect dependency arrays were already correct per exhaustive-deps; they were only broken by the hook's instability. Adds a regression test for object-identity stability in useDebouncedCallback.test.ts (the existing test only checked trigger/cancel individually, not the containing object), plus regression tests in BudgetOverviewPage.test.tsx (refetch settles and stops re-triggering) and DiaryEntryEditPage.test.tsx (a pending debounced autosave survives an unrelated re-render). Co-Authored-By: Claude dev-team-lead (Sonnet 4.6) Co-Authored-By: Claude frontend-developer (Haiku 4.5) Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) --- .../issue-1815-stylelint-remediation.md | 1 + .../issue-1816-component-reuse-hooks.md | 49 +++++++++++++++-- .../ux-designer/pr-review-findings.md | 10 ++-- client/src/hooks/useDebouncedCallback.test.ts | 25 +++++++++ client/src/hooks/useDebouncedCallback.ts | 4 +- .../BudgetOverviewPage.test.tsx | 52 ++++++++++++++++++ .../DiaryEntryEditPage.test.tsx | 54 +++++++++++++++++++ 7 files changed, 185 insertions(+), 10 deletions(-) diff --git a/.claude/agent-memory/qa-integration-tester/issue-1815-stylelint-remediation.md b/.claude/agent-memory/qa-integration-tester/issue-1815-stylelint-remediation.md index 12c24b114..0e3d28e51 100644 --- a/.claude/agent-memory/qa-integration-tester/issue-1815-stylelint-remediation.md +++ b/.claude/agent-memory/qa-integration-tester/issue-1815-stylelint-remediation.md @@ -10,6 +10,7 @@ fix + `npm run stylelint` wired into root `lint`/`lint:fix` + CI `static-analysi no bugs found, no test assertion updates needed. **Verification approach that worked well for a CSS-only PR with no new test files:** + - Ran the 5 touched-component test dirs batched in one `jest` invocation (29 suites / 1365 tests / 6 todo, all green) rather than one invocation per directory — faster and the spec explicitly listed them as a batch. diff --git a/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md b/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md index 80060ba52..9aa223bb8 100644 --- a/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md +++ b/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md @@ -44,8 +44,8 @@ effect (`skipAutoSaveOnMountRef` guard, lines ~194-219) consumes its "skip" flag effect run — which happens on initial mount (before `entry` loads, all metadata state is initial values). When `loadEntry()` resolves and `entry?.status` flips from `undefined` to `'draft'`, the effect's dependency array changes value, so it re-runs — but the skip flag is already spent, so it -fires `triggerAutoSave(true)` → an immediate `updateDiaryEntry` call with the *just-loaded, -unedited* content. Confirmed via isolated probe test (mount only, no interaction → +fires `triggerAutoSave(true)` → an immediate `updateDiaryEntry` call with the _just-loaded, +unedited_ content. Confirmed via isolated probe test (mount only, no interaction → `mockUpdateDiaryEntry` called once). This predates #1816 — the PR only touched the debounce/cancel plumbing (`doSaveImpl`/`scheduleAutoSave`), not this skip-ref effect. Existing Scenario 44 test doesn't catch it because it asserts `toHaveBeenCalledWith(...)` not call count. Worked around in @@ -55,12 +55,55 @@ tracked — not in scope for me to fix (test-only agent). ## Reusable pattern: capturing an untested mock prop -When a component under test mocks a child (e.g. `PhotoUpload`) and only captures *some* of its +When a component under test mocks a child (e.g. `PhotoUpload`) and only captures _some_ of its props (e.g. `onUpload` but not `onUploadingCountChange`), and a new spec needs to exercise the uncaptured prop, extend the mock factory to also capture it — don't create a parallel mock. Wrap any resulting `setState`-triggering invocation in `act()` since it's called directly, not through `fireEvent`/`userEvent` (which auto-wrap). +## Follow-up regression (PR #1848): hook tests must assert the RETURNED OBJECT's identity, not just its members + +The `useDebouncedCallback` "referentially stable" test above (added for #1816) only asserted +`result.current.trigger` and `result.current.cancel` were stable individually — it did NOT assert +`result.current` itself was the same object across re-renders. That was the exact gap: the hook +returned a fresh `{ trigger, cancel }` object literal every render even though `trigger`/`cancel` +were each individually memoized via `useCallback`. Any consumer effect that put the *whole hook +return value* in its dependency array (not just `.trigger`/`.cancel`) saw that dependency change on +every render, causing the effect to re-fire spuriously: +- `BudgetOverviewPage.tsx`: `[deselectedSourceIds, paymentStatus, isLoading, scheduleFetchBreakdown]` + effect re-fired every time `isBreakdownRefetching` flipped after a fetch resolved → unbounded + false/true ping-pong, breakdown never stopped "refetching", `.breakdownRefetching` + (`pointer-events: none`) overlay stuck on. This was the deterministic E2E smoke failure. +- `DiaryEntryEditPage.tsx`: `[uploadingCount, scheduleAutoSave]` cleanup effect called + `scheduleAutoSave.cancel()` on every render (not just `uploadingCount` changes), silently + cancelling any pending debounced autosave the instant an unrelated field caused a re-render. + +Fix: wrap the hook's return in `useMemo(() => ({ trigger, cancel }), [trigger, cancel])`. + +**Lesson for future hook tests**: when a hook returns an object/array bundling multiple values, +always add a test asserting `result.current` (the whole thing) `.toBe()` its prior value across an +unrelated re-render — in addition to (not instead of) testing individual member stability. Member +stability does not imply container stability. + +**Regression tests added** (all mutation-checked: fail on the pre-`useMemo` hook, pass on the +fixed one — verified via `git stash` on just `useDebouncedCallback.ts`, not the test files): +- `useDebouncedCallback.test.ts`: `result.current` itself `.toBe()`s its prior value across + `rerender({ delay: 300 })` (same delay → no reason for identity to change). +- `BudgetOverviewPage.test.tsx` (new describe "Regression #1816/#1848: breakdown refetch loop"): + after initial load settles, wait ~150ms real time (wrapped in `act`) for the debounce-triggered + refetch to resolve, snapshot `mockFetchBudgetBreakdown.mock.calls.length`, wait another ~500ms + real time with zero external state change, assert the call count did NOT grow further and + `document.querySelector('.breakdownRefetching')` is null. Used real timers + `act(async () => { + await new Promise(resolve => setTimeout(resolve, N)); })` rather than fake-timer stepping — more + robust for proving an *absence* of runaway async activity than manually stepping fake timers, + and avoids "not wrapped in act" warnings for the background state updates under test. +- `DiaryEntryEditPage.test.tsx` (new test after Scenario 44b): blur body textarea to schedule a + debounced autosave, advance 400ms (fake timers), then `fireEvent.change` (NOT blur) the `title` + input to force an unrelated re-render via `setTitle` without invoking `onFieldBlur`, advance the + remaining 700ms, assert `updateDiaryEntry` fires with the original scheduled body. Reused the + existing "flush + discard the mount-time spurious autosave" workaround from Scenario 44b (see + CODE_BUG note below) before the real assertion. + ## `.test.ts` vs `.test.tsx` for hook tests that need DOM fixtures `useClickOutside` needed real rendered DOM elements (refs + a raw non-ref `HTMLElement`) to diff --git a/.claude/agent-memory/ux-designer/pr-review-findings.md b/.claude/agent-memory/ux-designer/pr-review-findings.md index e7ca4cf80..050d559a7 100644 --- a/.claude/agent-memory/ux-designer/pr-review-findings.md +++ b/.claude/agent-memory/ux-designer/pr-review-findings.md @@ -60,11 +60,11 @@ Confirmed `gh pr review --approve` (not just `--request-changes`) also fails wit Full remediation of 370 latent stylelint violations, plus wiring `stylelint` into `npm run lint`/`lint:fix` and a new CI `Stylelint` step (`static-analysis` job). Also fixed `.stylelintrc.json`'s `tokens.css` override (missing `color-function-alias-notation: null`, was causing 116 of the 370 violations). Verified every claim in the PR description by direct diff inspection rather than trusting the summary: -- **`declaration-property-value-disallowed-list`** in `.stylelintrc.json` bans raw integer `z-index` and `font-weight` values via regex `/^\d+$/` — this is *why* `--z-raised: 1` and the `--z-local` escape hatch exist. Useful to know before flagging a future PR for "why didn't they just use `z-index: 1`" — it's lint-enforced, not stylistic preference. +- **`declaration-property-value-disallowed-list`** in `.stylelintrc.json` bans raw integer `z-index` and `font-weight` values via regex `/^\d+$/` — this is _why_ `--z-raised: 1` and the `--z-local` escape hatch exist. Useful to know before flagging a future PR for "why didn't they just use `z-index: 1`" — it's lint-enforced, not stylistic preference. - **New `--z-local` pattern** (first use in `PhotoMetadataSidepanel.module.css`): `--z-local: ;` declared immediately before `z-index: var(--z-local);` inside the same selector. Sanctioned escape hatch for component-internal arbitrary z-index offsets that don't belong in the shared global z-scale (`--z-dropdown`/`--z-overlay`/`--z-sidebar`/`--z-modal`) and aren't reused elsewhere. Recommended this get written into the Style Guide as the standard pattern for any future stylelint-blocked one-off literal. - **`--z-raised: 1`** fills the gap below `--z-dropdown: 10` for "lift element above local siblings" — swapped in 1:1 for every existing `z-index: 1` (and `calc(var(--z-raised) + 1)` for what was `z-index: 2`) across `CostBreakdownTable`, `GanttSidebar`, `DashboardPage`, `WorkItemDetailPage`, `BudgetOverviewPage`, `TimelinePage`, `calendar/CalendarView`. - **`--color-photo-*` family** (9 tokens: overlay-caption, control-bg/-hover, bar-bg, border, focus-ring, active-bg, action-primary-bg, action-danger-bg): deliberately theme-invariant (no dark override) — consumed by `PhotoViewer`, `PhotoCard`, `PhotoAnnotator`, all always-dark lightbox/editor chrome. This resolves two items I'd previously flagged as acceptable-but-not-ideal in [photo-annotator-patterns.md](photo-annotator-patterns.md): the dark-surface rgba hardcodes are now real tokens, and `PhotoAnnotator`'s `z-index: 1000` is now `var(--z-modal)`. -- **`--color-tooltip-*` family** (7 tokens: subdued, bullet, separator, status-not-started/in-progress/completed/late-bg): extracted from what used to be *component-scoped* custom properties re-declared inline in `GanttTooltip.module.css` under `.tooltip` and `[data-theme='dark'] .tooltip`. Only flag from this review: the tokens.css section comment says "GANTT TOOLTIP TOKENS" and sits in the Gantt-prefixed region, but the token names themselves are generic `--color-tooltip-*` (not `--color-gantt-tooltip-*`) — inconsistent with the rest of that section's `--color-gantt-` prefix convention, though defensible since a separate shared `Tooltip` component uses the same `--color-bg-inverse` surface pattern and could reuse these later. Low severity, non-blocking, flagged as a wiki/comment cleanup follow-up. +- **`--color-tooltip-*` family** (7 tokens: subdued, bullet, separator, status-not-started/in-progress/completed/late-bg): extracted from what used to be _component-scoped_ custom properties re-declared inline in `GanttTooltip.module.css` under `.tooltip` and `[data-theme='dark'] .tooltip`. Only flag from this review: the tokens.css section comment says "GANTT TOOLTIP TOKENS" and sits in the Gantt-prefixed region, but the token names themselves are generic `--color-tooltip-*` (not `--color-gantt-tooltip-*`) — inconsistent with the rest of that section's `--color-gantt-` prefix convention, though defensible since a separate shared `Tooltip` component uses the same `--color-bg-inverse` surface pattern and could reuse these later. Low severity, non-blocking, flagged as a wiki/comment cleanup follow-up. - **`--color-warning-badge-bg`** matches the existing `--color-{semantic}-badge-bg`/`-badge-text`/`-badge-bg-alt` scheme (`success`, `primary` already had entries) — good precedent, no `-badge-text` companion needed since consumer uses `var(--color-warning)` directly. - **`--shadow-text-overlay`** — clean 1:1 extraction from `LinkedDocumentCard`'s inline text-shadow-over-thumbnail exception comment, preserved verbatim. - **Visual-parity math verified, not assumed**: `InvoicePipelineCard .itemOverdue` swapped `rgba(251,146,60,0.08)` for solid `--color-warning-bg` (`#fff7ed`) — flattened the old rgba over white by hand (≈`rgb(255,246,239)` vs `rgb(255,247,237)`), confirms imperceptible difference, not a silent regression. @@ -84,8 +84,8 @@ Verification method worth reusing: `awk '/^\+/ && !/^\+\+\+/' | grep -Ei Small surface: usePhotos hook errors now translated (2 new photoViewer keys `networkError`/`unexpectedError`, en+de), 5 orphaned photoAnnotator keys + dead CSS (`.resetButton`/`.modalActions`/`.confirmButton`) removed, 5 shadowed literal-duplicate JSON keys removed (diary/householdItems/schedule, both locales), new `i18n.parity.test.ts` guard (en/de key parity + raw-text duplicate-key scanner across all namespace files). -Script-verify method for duplicate-key removals: parse the pre-PR JSON with Python's `json.loads(..., object_pairs_hook=...)` tracking repeated keys per object scope — reproduces exactly what `JSON.parse`/last-wins resolves to (same semantics as the browser/webpack JSON loader). Then grep the consuming `.tsx` for the actual `t('ns:path...')` calls to confirm which of the two (shadowed vs. surviving) values was ever rendered. Found: in `schedule.json`, `milestones.detail` had two `edit` keys and two `view` keys as *direct siblings* (not nested — watch for this, easy to misread indentation and think one is nested inside the other); `MilestoneDetailPage.tsx` only ever consumes the surviving (last) `view` block's fields, the first (removed) `view` block's `linkedItems`/`workItem`/etc. were separately duplicated as sibling top-level `detail.*` keys the component actually uses — so the whole first `view` object was double-dead (both a duplicate AND functionally redundant with existing live keys). +Script-verify method for duplicate-key removals: parse the pre-PR JSON with Python's `json.loads(..., object_pairs_hook=...)` tracking repeated keys per object scope — reproduces exactly what `JSON.parse`/last-wins resolves to (same semantics as the browser/webpack JSON loader). Then grep the consuming `.tsx` for the actual `t('ns:path...')` calls to confirm which of the two (shadowed vs. surviving) values was ever rendered. Found: in `schedule.json`, `milestones.detail` had two `edit` keys and two `view` keys as _direct siblings_ (not nested — watch for this, easy to misread indentation and think one is nested inside the other); `MilestoneDetailPage.tsx` only ever consumes the surviving (last) `view` block's fields, the first (removed) `view` block's `linkedItems`/`workItem`/etc. were separately duplicated as sibling top-level `detail.*` keys the component actually uses — so the whole first `view` object was double-dead (both a duplicate AND functionally redundant with existing live keys). -Bonus finding (informational, not blocking, not caused by this PR): even after dedup, `schedule.json`'s surviving `detail.edit` object (`title`/`form.*`) and `diary.json`'s `create.title`/`edit.title` keys are themselves entirely unreferenced by any `t()` call — real page headings/field labels live under separate `createPage.*`/`editPage.*`/`entryForm.*`/`detail.view.*` keys. Orphan-key sweeps that target only *literal duplicates* (this PR) or only *known-dead feature keys* (the 5 photoAnnotator keys) can still leave a residue of never-referenced-at-all keys behind — worth a follow-up sweep that cross-references every namespace key against `grep -rn "t('"` call sites, not just against duplicate-key detection. +Bonus finding (informational, not blocking, not caused by this PR): even after dedup, `schedule.json`'s surviving `detail.edit` object (`title`/`form.*`) and `diary.json`'s `create.title`/`edit.title` keys are themselves entirely unreferenced by any `t()` call — real page headings/field labels live under separate `createPage.*`/`editPage.*`/`entryForm.*`/`detail.view.*` keys. Orphan-key sweeps that target only _literal duplicates_ (this PR) or only _known-dead feature keys_ (the 5 photoAnnotator keys) can still leave a residue of never-referenced-at-all keys behind — worth a follow-up sweep that cross-references every namespace key against `grep -rn "t('"` call sites, not just against duplicate-key detection. -For dead-CSS verification: CSS Modules are locally scoped, so a repo-wide grep for a removed class name (e.g. `modalActions`) will hit dozens of *unrelated* components' own same-named local classes — always narrow the grep to the specific component file that imports the CSS module being edited, not the whole repo. +For dead-CSS verification: CSS Modules are locally scoped, so a repo-wide grep for a removed class name (e.g. `modalActions`) will hit dozens of _unrelated_ components' own same-named local classes — always narrow the grep to the specific component file that imports the CSS module being edited, not the whole repo. diff --git a/client/src/hooks/useDebouncedCallback.test.ts b/client/src/hooks/useDebouncedCallback.test.ts index f0ca68396..c8938ee56 100644 --- a/client/src/hooks/useDebouncedCallback.test.ts +++ b/client/src/hooks/useDebouncedCallback.test.ts @@ -124,6 +124,31 @@ describe('useDebouncedCallback', () => { expect(result.current.cancel).toBe(firstCancel); }); + // Regression #1816/#1848: the hook used to return a brand-new `{ trigger, cancel }` + // object literal on every render, even when `trigger` and `cancel` were themselves + // referentially stable. Any consumer that put the *whole hook return value* in a + // `useEffect`/`useMemo` dependency array (e.g. `[uploadingCount, scheduleAutoSave]` + // in DiaryEntryEditPage, or `[..., scheduleFetchBreakdown]` in BudgetOverviewPage) + // saw that dependency change on every render, causing effects to re-fire + // spuriously. The fix wraps the return value in `useMemo(() => ({ trigger, cancel + // }), [trigger, cancel])`. This test asserts the identity of the returned object + // itself, not just its members — the previous "referentially stable" test above + // only checked `result.current.trigger` / `result.current.cancel` individually and + // did not catch this. + it('the returned object itself is referentially stable across re-renders with unchanged delayMs', () => { + const callback = jest.fn<() => void>(); + const { result, rerender } = renderHook( + ({ delay }: { delay: number }) => useDebouncedCallback(callback, delay), + { initialProps: { delay: 300 } }, + ); + + const firstResult = result.current; + + rerender({ delay: 300 }); + + expect(result.current).toBe(firstResult); + }); + it('trigger changes reference when delayMs changes (cancel stays stable)', () => { const callback = jest.fn<() => void>(); const { result, rerender } = renderHook( diff --git a/client/src/hooks/useDebouncedCallback.ts b/client/src/hooks/useDebouncedCallback.ts index c3cba1161..0ca84955a 100644 --- a/client/src/hooks/useDebouncedCallback.ts +++ b/client/src/hooks/useDebouncedCallback.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useRef } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; export function useDebouncedCallback( callback: (...args: Args) => void, @@ -27,5 +27,5 @@ export function useDebouncedCallback( useEffect(() => cancel, [cancel]); - return { trigger, cancel }; + return useMemo(() => ({ trigger, cancel }), [trigger, cancel]); } diff --git a/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.test.tsx b/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.test.tsx index 987d79c86..ea773ca99 100644 --- a/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.test.tsx +++ b/client/src/pages/BudgetOverviewPage/BudgetOverviewPage.test.tsx @@ -944,6 +944,58 @@ describe('BudgetOverviewPage', () => { }); }); + // ─── Regression #1816/#1848: breakdown refetch loop ──────────────────────── + // + // `useDebouncedCallback` used to return a brand-new `{ trigger, cancel }` object + // on every render, even though `trigger`/`cancel` were themselves referentially + // stable. The debounce-refetch effect here depends on the whole + // `scheduleFetchBreakdown` object (deps: [deselectedSourceIds, paymentStatus, + // isLoading, scheduleFetchBreakdown]), so every render — including the render + // caused by the effect's own `setIsBreakdownRefetching` calls — made that + // dependency "change", re-firing the effect. That produced an unbounded + // false/true/false/... ping-pong: each fetch resolving triggered a render with a + // new `scheduleFetchBreakdown` identity, which re-ran the effect, which set + // `isBreakdownRefetching` back to `true` and scheduled another fetch, forever. + // Symptom: the breakdown never stopped "refetching" and its + // `pointer-events: none` overlay class stayed applied, blocking interaction. The + // fix wraps the hook's return value in `useMemo` so it settles once nothing + // relevant has changed. + describe('Regression #1816/#1848: breakdown refetch loop', () => { + it('the breakdown refetch settles and does not keep re-triggering itself after the initial load', async () => { + mockFetchBudgetOverview.mockResolvedValueOnce(richOverview); + mockFetchBudgetBreakdown.mockResolvedValue(emptyBreakdown); + mockFetchBudgetSources.mockResolvedValueOnce({ budgetSources: [] }); + + renderPage(); + + // Wait for the page to finish loading (initial overview + breakdown fetch). + await waitFor(() => { + expect(screen.queryByText(/loading budget overview/i)).not.toBeInTheDocument(); + }); + await waitFor(() => { + expect(mockFetchBudgetBreakdown.mock.calls.length).toBeGreaterThan(0); + }); + + // Give the 50ms debounce-triggered post-load refetch time to fire and settle. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 150)); + }); + + const callsAfterSettle = mockFetchBudgetBreakdown.mock.calls.length; + + // Wait several more debounce windows with no external state change at all. + // Pre-fix, this window would keep growing the call count indefinitely. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 500)); + }); + + expect(mockFetchBudgetBreakdown.mock.calls.length).toBe(callsAfterSettle); + + // The refetching overlay (pointer-events: none via CSS) must not be stuck on. + expect(document.querySelector('.breakdownRefetching')).toBeNull(); + }); + }); + // ─── URL state: paymentStatus param (Scenarios 20-25, Issue #1786) ───────── describe('URL state: ?paymentStatus param (Scenarios 20-25, Issue #1786)', () => { diff --git a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx index e03201eff..1915eeb08 100644 --- a/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx +++ b/client/src/pages/DiaryEntryEditPage/DiaryEntryEditPage.test.tsx @@ -841,6 +841,60 @@ describe('DiaryEntryEditPage', () => { jest.useRealTimers(); }); + // Regression #1816/#1848: useDebouncedCallback used to return a brand-new + // `{trigger, cancel}` object on every render. The uploadingCount-keyed cleanup + // effect (lines ~222-240) depends on that whole object, so its cleanup — + // `scheduleAutoSave.cancel()` — used to re-run on *every* render, not just when + // `uploadingCount` actually changed. That silently cancelled any pending + // debounced autosave the instant an unrelated field caused a re-render. The fix + // wraps the hook's return value in `useMemo` so the object is referentially + // stable across renders that don't change `trigger`/`cancel` identity. + it('Regression #1816/#1848: a pending debounced autosave survives an unrelated re-render and fires after its full delay', async () => { + jest.useFakeTimers(); + mockGetDiaryEntry.mockResolvedValueOnce(draftGeneralNoteEntry); + mockUpdateDiaryEntry.mockResolvedValue({ ...draftGeneralNoteEntry, body: 'updated' }); + renderEditPage('draft-1'); + + await waitFor(() => { + expect(screen.getByRole('textbox', { name: /^entry/i })).toBeInTheDocument(); + }); + + // Flush and discard the pre-existing spurious mount-time autosave (documented + // CODE_BUG, unrelated to this regression) so it doesn't muddy the assertion. + await jest.advanceTimersByTimeAsync(50); + mockUpdateDiaryEntry.mockClear(); + + // Blur the body textarea to schedule a debounced autosave (1000ms). + const textarea = screen.getByRole('textbox', { name: /^entry/i }); + fireEvent.change(textarea, { target: { value: 'updated body' } }); + fireEvent.blur(textarea); + + // Advance partway — not enough to fire yet. + await jest.advanceTimersByTimeAsync(400); + expect(mockUpdateDiaryEntry).not.toHaveBeenCalled(); + + // Trigger a re-render that has nothing to do with autosave scheduling: change + // (not blur) the title field. This calls setTitle, forcing a re-render, but + // never touches `uploadingCount` or the autosave trigger/cancel path. + const titleInput = screen.getByLabelText(/^title$/i); + fireEvent.change(titleInput, { target: { value: 'An unrelated title edit' } }); + + // Advance the remaining time past the original 1000ms debounce window. If the + // unrelated re-render had cancelled the pending save (the pre-fix bug), + // updateDiaryEntry would never fire. + await jest.advanceTimersByTimeAsync(700); + + await waitFor(() => { + expect(mockUpdateDiaryEntry).toHaveBeenCalledTimes(1); + }); + expect(mockUpdateDiaryEntry).toHaveBeenCalledWith( + 'draft-1', + expect.objectContaining({ body: 'updated body' }), + ); + + jest.useRealTimers(); + }); + it('Scenario 46: weather select change on draft → triggers immediate auto-save', async () => { jest.useFakeTimers(); const draftDailyLogEntry: DiaryEntryDetail = {