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-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 new file mode 100644 index 000000000..9aa223bb8 --- /dev/null +++ b/.claude/agent-memory/qa-integration-tester/issue-1816-component-reuse-hooks.md @@ -0,0 +1,114 @@ +# 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). + +## 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 +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..050d559a7 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 @@ -66,8 +84,8 @@ Confirmed `gh pr review --approve` (not just `--request-changes`) also fails wit 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/.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..c8938ee56 --- /dev/null +++ b/client/src/hooks/useDebouncedCallback.test.ts @@ -0,0 +1,204 @@ +/** + * @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); + }); + + // 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( + ({ 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..0ca84955a --- /dev/null +++ b/client/src/hooks/useDebouncedCallback.ts @@ -0,0 +1,31 @@ +import { useCallback, useEffect, useMemo, 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 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/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..1915eeb08 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,106 @@ 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(); + }); + + // 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 = { 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) => (