chore(eslint): add cleanup for setTimeout/setInterval/addEventListener in useEffect (#1468)

[steilerDev]

Summary

Fixes @eslint-react/web-api-no-leaked-timeout warnings by adding proper cleanup in all useEffect hooks that create setTimeout calls without returning a cleanup function.

Changes

client/src/components/documents/LinkedDocumentsSection.tsx

• Focus picker modal on open (showPicker effect): Capture the setTimeout return value and clearTimeout it in the cleanup
• Focus Cancel button on unlink dialog open (unlinkTarget effect): Same pattern

client/src/pages/InvoiceDetailPage/InvoiceBudgetLinesSection.tsx

• Focus picker modal on open (showPicker effect): Same pattern as above
• Focus description field when create form opens (pickerState.showCreateForm effect): Same pattern

Investigation Notes

I audited every setTimeout, setInterval, addEventListener call in the codebase that appeared inside a useEffect. The vast majority were already correctly cleaned up:

• setTimeout in event handlers / callbacks (not in useEffect) — not flagged by the rule, already safe
• Debounce refs (SearchPicker, WorkItemSelector, BudgetOverviewPage, etc.) — all have a dedicated unmount useEffect that calls clearTimeout
• addEventListener calls — all 20+ instances already return a corresponding removeEventListener cleanup

The 4 genuine violations were all zero-delay focus management setTimeouts (delay: 0) used to defer focus into newly-rendered modal/picker elements. These are a well-known React pattern but need the cleanup return to avoid the rule warning and to be technically correct (the timer would fire even if the component unmounts before the next microtask tick).

Fixes #1468
Part of #1455

[steilerDev]

[ux-designer]

Design Review — PR #1643 chore(eslint): add cleanup for setTimeout/setInterval/addEventListener in useEffect

VERDICT: APPROVED ✅

(Self-approval blocked by GitHub — using --comment per instructions.)

---

Scope Verification

The diff touches exactly 2 TypeScript files (LinkedDocumentsSection.tsx, InvoiceBudgetLinesSection.tsx) with zero CSS/styling changes. No design tokens, CSS Modules, JSX structure, layout, color, spacing, or dark mode values are modified. This is a pure logic refactor.

---

Focus Behaviour Analysis

All four changed useEffect hooks use the identical pattern:

const timer = setTimeout(() => {
  ref.current?.focus();
}, 0);
return () => clearTimeout(timer);

Focus still lands on the correct element in all cases:

Effect	Trigger	Focus target	Behaviour preserved?
LinkedDocumentsSection picker modal	showPicker becomes true	pickerModalRef.current	✅ Yes
LinkedDocumentsSection unlink dialog	unlinkTarget becomes truthy	cancelButtonRef.current	✅ Yes
InvoiceBudgetLinesSection picker modal	showPicker becomes true	pickerModalRef.current	✅ Yes
InvoiceBudgetLinesSection create form	pickerState.showCreateForm becomes true	#budget-description	✅ Yes

The setTimeout(fn, 0) defers focus until after the browser has painted the newly rendered DOM — unchanged from before. The cleanup only runs when the component unmounts or the dependency flips back, i.e. the timer is cancelled before it fires, not after. In normal usage (state flips to true and the component stays mounted), the cleanup function is never called while the timer is still pending, so focus management is functionally identical to the pre-patch code.

---

Rapid Open/Close Regression Risk

Worst-case sequence: user opens picker → immediately closes it before the 0ms timer fires.

• Before patch: Timer fires after close; ref.current may point to an unmounted or hidden element; focus() is a no-op on a non-displayed element — harmless but slightly wasteful.
• After patch: Cleanup fires on state flip back to false, clearTimeout cancels the timer before it runs — strictly better. No focus theft, no residual side-effect.

The PR author's investigation note (audited all setTimeout/addEventListener calls in useEffect) is correct — the remaining setTimeout calls in closePicker and handleDocumentSelect are inside useCallback callbacks, not directly inside useEffect, so they are not subject to the ESLint rule and do not require cleanup.

---

Design System Checklist

• Token adherence — No CSS/token changes; N/A
• Visual consistency — No rendering changes; layout/appearance identical
• Dark mode — No color values touched; N/A
• Responsive / touch targets — Unchanged; focus targets retain existing touch target compliance
• Accessibility — Focus management behaviour preserved; clearTimeout in cleanup prevents unwanted focus theft on rapid state changes — a net accessibility improvement
• Shared pattern usage — No new components; N/A
• Component reuse — N/A
• Animation/transition — No animation code changed

No visual, dark mode, responsive, or accessibility regressions identified.

[steilerDev]

[frontend-developer] APPROVED

Frontend Engineering Review: APPROVED ✅

This PR adds proper cleanup for setTimeout(..., 0) patterns in 4 useEffect hooks. The implementation follows idiomatic React patterns and correctly handles timer cleanup.

Changes Verified

LinkedDocumentsSection.tsx (2 effects):

• Effect 1 (line 101-109): Focus picker modal on open — ✅ proper cleanup with const timer = setTimeout(...); return () => clearTimeout(timer);
• Effect 2 (line 111-119): Focus cancel button on unlink confirmation — ✅ proper cleanup pattern applied
• Dependency arrays: [showPicker] and [unlinkTarget] unchanged — ✅ correct

InvoiceBudgetLinesSection.tsx (2 effects):

• Effect 1 (line 161-168): Focus picker modal on open — ✅ proper cleanup pattern
• Effect 2 (line 558-566): Focus description field when create form opens — ✅ proper cleanup pattern
• Dependency arrays: [showPicker] and [pickerState.showCreateForm] unchanged — ✅ correct

Completeness Audit (Spot-Check)

Sampled other components to verify the audit claim:

• SearchPicker.tsx: Uses debounce with clearTimeout cleanup ✅
• WorkItemSelector.tsx: Proper cleanup for debounce timeout + addEventListener ✅
• DocumentBrowser.tsx: Proper cleanup for debounce + event listeners ✅
• GanttChart.tsx: Proper cleanup for addEventListener ✅
• Modal.tsx: No timer patterns (just keyboard handler with cleanup) ✅

The author's audit claim is accurate. These are the problematic patterns, and the fixes align with established patterns in the codebase.

Code Quality Assessment

• ✅ No closure issues — refs safely accessed with optional chaining
• ✅ Correct dependency arrays — minimal and unchanged from original
• ✅ Idiomatic pattern — matches established cleanup patterns elsewhere
• ✅ Deterministic cleanup — fires correctly on unmount or dependency change

Note

Several setTimeout calls exist outside useEffect hooks (in callbacks for announcement clearing/focus restoration). These are correctly not wrapped in effects since they're one-time actions — no cleanup needed.

Verdict: Ready to merge once conflicts are resolved.

[steilerDev]

[product-architect]

VERDICT: APPROVED (posted as comment — GitHub blocks self-approval)

Architecture review — no concerns.

Pattern consistency audit ✅

Spot-checked the author's claim that "only these 4 useEffect hooks were missing cleanup". Ran a static audit across all production .ts/.tsx files in client/src/ looking for useEffect blocks containing setTimeout/setInterval/addEventListener without a corresponding return () => clear*/removeEventListener cleanup. Zero other instances found. Audit confirmed.

In particular, the other setTimeout calls in the two touched files (LinkedDocumentsSection.tsx lines 124, 182, 192, 211, 218; InvoiceBudgetLinesSection.tsx lines 171, 355, 444, 871, 1049) are all inside useCallback event handlers — not useEffect — so they are correctly out of scope for this PR (fire-and-forget focus restoration after a user action).

The cleanup pattern used (const timer = setTimeout(...); return () => clearTimeout(timer);) matches the idiomatic pattern already used throughout the codebase (e.g. ToastContext.tsx, Tooltip.tsx, CalendarView.tsx, ref-based debouncers in pages).

Other dimensions

• API contract / schema: N/A (client-only refactor)
• Wiki/ADR: No implications — pure code hygiene, no architectural decision involved
• Scope: Minimal and surgical (+8/-4), addresses exactly the issue
• Tests: Behavior unchanged (zero-delay timers complete before any plausible unmount); no test changes needed

Ignoring the merge conflict per orchestrator instruction.

[steilerDev]

[dev-team-lead]

VERDICT: APPROVED

Code Review: PR #1643 — clearTimeout cleanup in focus-management useEffect hooks

Summary

The fix correctly addresses all 4 @eslint-react/web-api-no-leaked-timeout warnings. Both modified files are clean.

Pattern Assessment ✅

All 4 changes follow the idiomatic React cleanup pattern — assigning setTimeout to a named const timer, then returning () => clearTimeout(timer). Cleanup is correctly placed inside the conditional branch (only registered when the timer is created, so no spurious clearTimeout(undefined) call on the else path).

Audit Completeness ✅

Spot-checked all other setTimeout/setInterval/addEventListener usages across the codebase. All remaining instances fall into one of these safe categories:

1. useCallback / async handlers — e.g., closePicker, handleLinkDocument, handleUnlink in both files — not subject to @eslint-react/web-api-no-leaked-timeout
2. Ref-based debounce patterns with clearTimeout guards — e.g., DiaryPage, DocumentBrowser, useColumnPreferences, BudgetOverviewPage — already have return () => clearTimeout in their effects
3. Cleanup-only useEffect — e.g., InvoiceDepositsSection.tsx uses revertErrorTimerRef with a dedicated cleanup effect
4. addEventListener with paired removeEventListener — all useEffect blocks in both modified files have symmetric add/remove (verified programmatically)

The author's claim that only these 4 effects lacked cleanup is confirmed correct.

Functional Equivalence ✅

Zero-delay setTimeout(..., 0) focus timers typically fire before any meaningful unmount. The cleanup is defensive and technically correct; it adds no functional risk.

No New ESLint Warnings ✅

The fix eliminates the 4 flagged warnings. The const timer = ... variable is used immediately and does not trigger unused-variable linting warnings.

---

✅ Approved. Ready to rebase onto beta once #1640/#1642 land.

[steilerDev]

[orchestrator] Consolidated /review-pr 1643 verdict: ✅ APPROVED (all 4 reviewers)

Reviewer	Verdict	Highlights
dev-team-lead	APPROVED	All 4 clearTimeout cleanups follow idiomatic const timer = setTimeout(...); return () => clearTimeout(timer) pattern; verified all addEventListeners in both files have paired removeEventListener; audit claim confirmed across the client \u2014 other setTimeout usages are in useCallback/async handlers (out of scope) or already cleaned in debounce refs
product-architect	APPROVED	Ran static audit across all production client/src/**/*.{ts,tsx} \u2014 zero other useEffect blocks with leaked timers, author's claim confirmed. Cleanup pattern matches idiomatic usage in ToastContext, Tooltip, CalendarView. No wiki/ADR impact.
frontend-developer	APPROVED	Dependency arrays unchanged ([showPicker], [unlinkTarget], [showPicker], [pickerState.showCreateForm]); spot-checked SearchPicker, WorkItemSelector, DocumentBrowser, GanttChart, Modal \u2014 all properly cleaned; no closure issues
ux-designer	APPROVED	Pure logic refactor, zero CSS/styling changes. Focus behaviour identical in normal open path (cleanup only fires when state flips back); rapid open/close is strictly safer now (no stray focus on hidden element) \u2014 net accessibility improvement

Status

• Retargeted: main \u2192 beta (was incorrectly targeting main).
• Mergeability: CONFLICTING against beta \u2014 will need a rebase after chore(eslint): fix @eslint-react/purity warning — move new Date() to useMemo (#1471) #1640 and chore(eslint): fix miscellaneous @eslint-react structural errors (#1462) #1642 land.
• Blocked on CI infra: same systemic Quality Gates failure as the other PRs in this batch. Will be merged after rebase + WP-0 CI-infra fix lands on beta.

Closes #1468 \u00b7 Part of #1455