test(fetch-client): restore globalThis.fetch in afterEach

[ymc9]

Summary

• Save the original globalThis.fetch at describe scope and restore it in afterEach so the mocked fetch does not leak into other test files.

Test plan

• pnpm vitest run test/fetch-client.test.ts in packages/clients/fetch-client passes (47/47).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved test cleanup to properly preserve and restore the original fetch implementation between test runs, enhancing test reliability and preventing potential test interference.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0aef9f82-2382-438f-bda3-9be2ea38932b

📥 Commits

Reviewing files that changed from the base of the PR and between 1a4de21 and c42a154.

📒 Files selected for processing (1)

• packages/clients/fetch-client/test/fetch-client.test.ts

---

📝 Walkthrough

Walkthrough

This PR improves test isolation in the fetch-client test suite by properly preserving and restoring the original globalThis.fetch function. The test now captures the existing fetch before each test run and restores it afterward, replacing a previous approach that relied solely on vi.resetAllMocks() without explicit restoration.

Changes

Fetch Mock Test Isolation

Layer / File(s)	Summary
Fetch mock setup and teardown packages/clients/fetch-client/test/fetch-client.test.ts	Test suite stores original globalThis.fetch, installs Vitest mock in beforeEach, and restores original in afterEach for proper cleanup and test isolation.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A mock, a store, a restore divine,
Fetch functions now play nicely in line,
Before and after, pristine and clean,
No test pollution in between!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: restoring globalThis.fetch in the afterEach cleanup hook to prevent test pollution.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/fetch-client-test-restore-global-fetch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

✅ PR Review Complete

Todo List:

• Read the changed test file to understand the implementation
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage impact
• Provide comprehensive feedback

Summary

This is a clean and well-executed fix for preventing test pollution in the fetch-client test suite.

Code Quality & Best Practices ✅

• Excellent approach: Storing the original globalThis.fetch at the describe scope (line 21) and restoring it in afterEach (line 29) is the correct pattern for test isolation
• Proper timing: The restoration happens in afterEach after vi.resetAllMocks(), ensuring complete cleanup
• Consistent style: Follows the existing test structure and naming conventions

Potential Bugs/Issues ✅

• No issues identified: The implementation correctly handles the fetch restoration
• Edge case coverage: Works properly even if globalThis.fetch was undefined initially (though unlikely in this environment)
• Order dependency: The cleanup order is correct - mocks are reset first, then fetch is restored

Performance Considerations ✅

• Minimal overhead: Only adds one variable declaration and one assignment per test run
• No memory leaks: Proper cleanup prevents accumulation of mock references

Security Concerns ✅

• Safe implementation: No security implications - only restoring a legitimate browser API
• Test isolation: Prevents one test's mocks from affecting others, which is a security best practice in testing

Test Coverage Impact ✅

• Improves reliability: Prevents flaky tests caused by fetch mock leakage between test files
• Maintains existing coverage: All 47 tests still pass as confirmed in the test plan
• Better isolation: Each test file now starts with a clean fetch implementation

Specific Code Review

packages/clients/fetch-client/test/fetch-client.test.ts:21

const originalFetch = globalThis.fetch;

✅ Good: Captured at the right scope (describe block) to be available for cleanup

packages/clients/fetch-client/test/fetch-client.test.ts:29

globalThis.fetch = originalFetch;

✅ Good: Properly restores the original fetch in afterEach, ensuring test isolation

Recommendation

Approved - This is a textbook example of proper test cleanup. The changes are minimal, focused, and solve a real problem with test isolation. No changes needed.

---