Vitest for hook use-server-notifications - #2260
Conversation
Cover initial mount sync, empty fetch result, centreOpen refresh, visibility change sync and polling, polling interval lifecycle, and unmount cleanup.
|
Reviewed: purely additive vitest for use-server-notifications (a verified gap) - 11 cases covering mount fetch+merge, the full visibilitychange lifecycle including listener removal on unmount, polling start/clear/hidden-tab-no-poll, and centre-open refresh with the no-refetch-on-rerender negative. Cleanup-path coverage is the part hooks usually skip; this has it. Merging on green. |
📝 WalkthroughWalkthroughAdded comprehensive Vitest coverage for ChangesServer notification hook tests
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoAdd Vitest coverage for useServerNotifications hook
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
nemotron-super review VERDICT: No blocking issues found.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/hooks/use-server-notifications.test.ts`:
- Around line 212-226: Update the test using the renderHook result for the
useServerNotifications hook: call rerender() after the initial effects settle,
wait for the rerender’s effects to complete, then assert fetchSpy was called
exactly twice so repeated renders are verified not to trigger another fetch.
- Around line 27-31: Add vi.restoreAllMocks() to the afterEach cleanup alongside
fetchSpy.mockClear() and capturedListener reset, ensuring
document.addEventListener and timer spies are restored between tests rather than
only having their call history cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50f6fc27-1ccd-4187-8d11-b52951f5e742
📒 Files selected for processing (1)
desktop/src/hooks/use-server-notifications.test.ts
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| fetchSpy.mockClear(); | ||
| capturedListener = null; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching test name:\n'
fd -a 'use-server-notifications.test.ts' . || true
file="$(fd 'use-server-notifications.test.ts' . | head -n 1 || true)"
if [ -z "${file:-}" ]; then
echo "No matching file found"
exit 0
fi
echo
printf 'File: %s\n' "$file"
wc -l "$file"
echo
echo 'Outline:'
ast-grep outline "$file" --view expanded || true
echo
echo 'Relevant content:'
cat -n "$file"
echo
echo 'Search for addEventListener/reduceMocks usage in hooks/tests:'
rg -n "addEventListener|vi\.restoreAllMocks|mockClear|mockRestore|stubAllGlobals|unstubAllGlobals" desktop/src || trueRepository: jaylfc/taOS
Length of output: 47830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe for the Vitest mock concern: mockClear preserves the mock function implementation
# while clearing call history; restore only returns the original Spy implementation.
# This does not require running repository code.
cat >/tmp/verify-mock-behaviour.js <<'JS'
let originalCalled = 0;
let originalHandler = () => { originalCalled += 1; };
let callHistory = 0;
function mockHandler() { callHistory += 1; }
function replaceGlobal() {
globalThis.addEventListener = mockHandler;
return mockHandler;
}
function clearSpy(fn) {
callHistory = 0;
return fn;
}
function restoreSpy() {
globalThis.addEventListener = originalHandler;
}
const spy = replaceGlobal();
spy({ type: 'visibilitychange' }, originalHandler);
console.log('spy implementation preserved after spy call:', spy === mockHandler);
spy = clearSpy(spy);
console.log('spy history after mockClear:', callHistory);
console.log('spy implementation call after mockClear:', spy.toString().includes('callHistory') || Object.prototype.toString.call(spy) === '[object Function]' ? 'kept mock implementation through no-op clear behavior' : 'lost');
restoreSpy();
globalThis.addEventListener({ type: 'visibilitychange' }, originalHandler);
console.log('after restore global implementation:', globalThis.addEventListener === originalHandler);
console.log('original handler called after restore:', originalCalled);
JS
node /tmp/verify-mock-behaviour.jsRepository: jaylfc/taOS
Length of output: 837
🌐 Web query:
Vitest vi.spyOn mock.restore mockRestore mockClear API
💡 Result:
In Vitest, vi.spyOn is used to track calls to a method or getter/setter on an object, returning a mock function that can be controlled [1][2]. The mock function provides several methods to manage its state, with distinct behaviors for clearing, resetting, and restoring: mockClear: Clears the mock's call history (e.g., mock.calls, mock.instances) but leaves the mock's current implementation intact [1][3]. It is useful for cleaning up between tests without losing custom behaviors [1]. mockReset: Performs everything mockClear does and additionally resets the mock's implementation to undefined (or an empty function) [1][3]. This also clears any "once" implementations [1]. mockRestore: Performs everything mockReset does and restores the original implementation of the spied-on object [1][3]. When called on a spy created via vi.spyOn, it replaces the mock function with the original method, effectively "unspying" it [1][3]. If called on a plain vi.fn mock, it behaves identically to mockReset [1][3]. For managing multiple mocks, Vitest also provides global helpers: vi.clearAllMocks: Calls mockClear on all registered spies without affecting implementations [2]. vi.restoreAllMocks: Restores the original implementations for all spies created with vi.spyOn [2]. Note that this does not automatically clear mock history or reset implementations; it specifically focuses on restoring the original descriptor [2]. You can automate these actions by enabling clearMocks or restoreMocks in your Vitest configuration, or by using afterEach hooks [1][2][3]. Additionally, in environments supporting Explicit Resource Management, you can use the using keyword to automatically call mockRestore when a block is exited [2].
Citations:
- 1: https://vitest.dev/api/mock.html
- 2: https://vitest.dev/api/vi
- 3: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/mock.md
Restore DOM and timer spies after each test.
Several tests replace document.addEventListener and do not restore it. mockClear() only removes call history, so later tests can use the previous test’s mocked DOM API. Add vi.restoreAllMocks() to afterEach.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/hooks/use-server-notifications.test.ts` around lines 27 - 31, Add
vi.restoreAllMocks() to the afterEach cleanup alongside fetchSpy.mockClear() and
capturedListener reset, ensuring document.addEventListener and timer spies are
restored between tests rather than only having their call history cleared.
| it("does not re-fetch when centreOpen is already true and re-renders", async () => { | ||
| fetchSpy.mockClear(); | ||
| fetchSpy.mockResolvedValueOnce([]); | ||
|
|
||
| useNotificationStore.setState({ centreOpen: true }); | ||
|
|
||
| renderHook(() => useServerNotifications()); | ||
|
|
||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| await new Promise((r) => setTimeout(r, 0)); | ||
| }); | ||
|
|
||
| // mount effect fires once; second effect fires because centreOpen is true at mount | ||
| expect(fetchSpy).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Call rerender before asserting no re-fetch.
This test never re-renders the hook. It only verifies the two expected mount-time fetches. A regression that fetches again on every render would still pass.
Call rerender(), wait for effects, and keep the expected call count at two.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@desktop/src/hooks/use-server-notifications.test.ts` around lines 212 - 226,
Update the test using the renderHook result for the useServerNotifications hook:
call rerender() after the initial effects settle, wait for the rerender’s
effects to complete, then assert fetchSpy was called exactly twice so repeated
renders are verified not to trigger another fetch.
Code Review by Qodo
1. Leaky document spies
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| fetchSpy.mockClear(); | ||
| capturedListener = null; | ||
| }); |
There was a problem hiding this comment.
1. Leaky document spies 🐞 Bug ☼ Reliability
Several tests replace document.addEventListener via vi.spyOn(...).mockImplementation(...) but the suite afterEach never restores spies, so later tests may run with a mocked addEventListener or accumulated call history and become order-dependent.
Agent Prompt
## Issue description
Tests in `use-server-notifications.test.ts` create spies/mocks on `document.addEventListener` (and sometimes other globals), but `afterEach` only calls `vi.unstubAllGlobals()` and does not restore `vi.spyOn` replacements. This can leak mocked DOM APIs across tests.
## Issue Context
`useServerNotifications` registers a `visibilitychange` listener on mount and removes it on cleanup; if `document.addEventListener` stays mocked, other tests can stop exercising real behavior and/or assertions can be polluted by previous calls.
## Fix Focus Areas
- desktop/src/hooks/use-server-notifications.test.ts[27-31]
- desktop/src/hooks/use-server-notifications.test.ts[35-37]
## Suggested fix
- Add `vi.restoreAllMocks()` in `afterEach` (or restore the specific spies created in each test with `mockRestore()`), keeping `vi.unstubAllGlobals()` for `vi.stubGlobal` cleanups.
- If you use `vi.restoreAllMocks()`, ensure any needed mocks are re-established in `beforeEach`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Object.defineProperty(document, "hidden", { value: true, configurable: true }); | ||
| act(() => capturedListener!()); | ||
| expect(clearIntervalSpy).toHaveBeenCalled(); | ||
|
|
There was a problem hiding this comment.
2. Document.hidden not restored 🐞 Bug ☼ Reliability
Tests overwrite document.hidden using Object.defineProperty, but do not restore the original property descriptor, which can leak a changed hidden implementation into subsequent tests and make polling behavior assertions flaky.
Agent Prompt
## Issue description
The suite mutates `document.hidden` via `Object.defineProperty`, which can replace an accessor-based implementation with a plain value property. The tests set it back to `false` in one case, but they do not restore the original descriptor.
## Issue Context
`useServerNotifications` reads `document.hidden` to decide whether to start/stop polling and whether to sync when visibility changes.
## Fix Focus Areas
- desktop/src/hooks/use-server-notifications.test.ts[118-123]
- desktop/src/hooks/use-server-notifications.test.ts[174-188]
- desktop/src/hooks/use-server-notifications.ts[42-51]
## Suggested fix
Option A (descriptor restore):
- In `beforeEach`, capture `const hiddenDesc = Object.getOwnPropertyDescriptor(document, "hidden")` (or on `Document.prototype` if that’s where it lives).
- In `afterEach`, restore it with `Object.defineProperty(document, "hidden", hiddenDesc)` (or delete/redefine appropriately).
Option B (spy getter):
- If `hidden` is implemented as a getter, use `vi.spyOn(document, "hidden", "get").mockReturnValue(...)` and rely on `vi.restoreAllMocks()` in `afterEach`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| it("does not re-fetch when centreOpen is already true and re-renders", async () => { | ||
| fetchSpy.mockClear(); | ||
| fetchSpy.mockResolvedValueOnce([]); | ||
|
|
||
| useNotificationStore.setState({ centreOpen: true }); |
There was a problem hiding this comment.
3. Misnamed centreopen rerender test 🐞 Bug ⚙ Maintainability
The test claims it checks “does not re-fetch when centreOpen is already true and re-renders”, but it never triggers a rerender and instead asserts two mount-time fetches, so it doesn’t cover the behavior described by its name.
Agent Prompt
## Issue description
The test name and comment/assertions don’t match: it says it verifies no refetch on rerender, but it only mounts once and expects 2 fetches (from the mount effect + `centreOpen` effect).
## Issue Context
`useServerNotifications` has two effects: one runs on mount, and one runs whenever `centreOpen` is true (including initial mount if already true).
## Fix Focus Areas
- desktop/src/hooks/use-server-notifications.test.ts[212-227]
- desktop/src/hooks/use-server-notifications.ts[20-27]
- desktop/src/hooks/use-server-notifications.ts[59-69]
## Suggested fix
- Either rename the test to reflect actual behavior (e.g. “fetches twice when centreOpen is true at mount”),
- Or actually test rerender stability:
- `const { rerender } = renderHook(...)`
- wait for initial effects to settle
- `fetchSpy.mockClear()`
- call `rerender()` (with same inputs)
- assert `fetchSpy` was not called again.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-orB review VERDICT: Test file has fragile shared state, implementation-detail coupling, missing error-path coverage, and flaky async patterns.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-ultra-kilo review VERDICT: Needs improvement - several correctness risks, flaky patterns, and missing error-handling tests.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
1 participant
CARD TITLE (intent, not commit subject): Vitest for hook use-server-notifications
Autonomous build of board card tsk-ebg57j.
Cover initial mount sync, empty fetch result, centreOpen refresh,
visibility change sync and polling, polling interval lifecycle,
and unmount cleanup.
Files:
desktop/src/hooks/use-server-notifications.test.ts | 228 +++++++++++++++++++++
1 file changed, 228 insertions(+)
Summary by CodeRabbit