fix: fix flaky tests for use-project-picker-data hook

[tombogle]

Code Review Summary

Branch: fix-flaky-tests

Base: origin/main

Date: 2026-06-02

Review model: Claude Sonnet 4.6

Files changed: 1

Overview

This change fixes flaky tests in src/renderer/hooks/use-project-picker-data.hook.test.ts. The root cause was vi.clearAllMocks() in beforeEach, which only clears call history but leaves mockReturnValue/mockImplementation overrides intact — allowing one test's mock setup to bleed into subsequent tests. Replacing it with vi.resetAllMocks() fully resets both call history and mock implementations, ensuring proper test isolation.

Beyond the root-cause fix, the change consolidates shared default mock setup into beforeEach (reducing per-test boilerplate), hoists a stable DEFAULT_RECENT_IDS constant to module scope (preventing useMemo recomputation loops from referential instability), and wraps paired assertions inside single waitFor callbacks (the correct React Testing Library pattern to avoid asserting on intermediate async states).

API Changes

None. The only changed file is a test file — no public API surfaces were modified.

Findings

Critical — Must address before merge

None.

Important — Should address before merge

• use-project-picker-data.hook.test.ts:92 — Prettier formatting violation (fixed during review: expanded single-line destructuring to multi-line so the line fits within Prettier's printWidth)

Minor — Consider

• use-project-picker-data.hook.test.ts:183 — Redundant mock setup (fixed during review: removed projectLookupService.getMetadataForAllProjects.mockResolvedValue([]) and unused projectLookupService destructure binding — beforeEach already establishes this default after vi.resetAllMocks())

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• vi.clearAllMocks() → vi.resetAllMocks() with a clear inline comment explaining the distinction (clear resets call history only; reset also clears mockReturnValue/mockImplementation overrides) — makes the fix self-documenting.
• DEFAULT_RECENT_IDS hoisted to module scope with a comment explaining the infinite re-render risk it prevents — good defensive documentation for a subtle referential-equality concern.
• Default mock setup consolidated in beforeEach so individual tests only override what they specifically care about, making each test's intent immediately readable.
• Multiple related assertions correctly placed inside a single waitFor callback — the right React Testing Library pattern to avoid asserting on stale intermediate render states.
• getNetworkEvent moved into importMocks() so all tests get it through the same consistent path, rather than one test importing it separately out of order.
• useData mock factory simplified at the mock definition level now that the stable reference is handled at module scope.

Interview Notes

Author purpose: fix flaky tests in use-project-picker-data.hook.test.ts.

Both findings were addressed during the review at the author's request — no dismissals. The author did not volunteer additional context or flag any tradeoffs or uncertainties.

In-Review Quality Check

Two fixes applied during the review interview:

1. Prettier formatting — expanded the beforeEach destructuring from a single overlong line to multi-line. No other formatting changes to the test file.
2. Redundant mock removal — removed the no-op mockResolvedValue([]) call and cleaned up the now-unused projectLookupService binding.

Post-fix quality results:

• npm run typecheck — PASSED
• npm run lint — PASSED
• npm run format — Applied auto-formatting to two pre-existing .mdx files (keyboard-shortcuts.mdx, theming.mdx) with unrelated table column drift; test file itself was unchanged.
• vitest run (test file only) — PASSED (7/7 tests)

Suggested Review Focus

• Confirm the vi.resetAllMocks() change is the correct fix for the reported flakiness — verify the flaky tests were caused by mock implementation bleed-through rather than another source (e.g. async timing, module-level state).
• Verify the DEFAULT_RECENT_IDS module-scope constant is the right place for this — check whether other tests in the file rely on different useData return shapes that could interact with this default.

---

This change is 

[captaincrazybro]

LGTM! I'm gonna approve, but make sure you revert the changes review paratext did to the keyboard-shortcuts.mdx and themeing.mdx

[tombogle]

Grrr....