Prevent setTimeout from firing if we can't match devtools user agent - #13349
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 789eedbf85d2779c21efa047 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
🦋 Changeset detectedLatest commit: 588ab4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughApolloClient now computes the browser user agent and devtools extension URL before scheduling the delayed suggestion. The suggestion log is gated by top-level browsing context, supported protocols, and URL availability. ChangesDevtools suggestion control flow
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/ApolloClient.ts (1)
1172-1199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the scheduling gate.
Test an unsupported/jsdom user agent to verify that no timeout is registered, then cover Chrome and Firefox user agents to verify the delayed suggestion still logs correctly when the hook is absent.
🤖 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 `@src/core/ApolloClient.ts` around lines 1172 - 1199, Add regression tests around the scheduling gate in the Apollo client devtools suggestion logic: use an unsupported or jsdom user agent to assert no timeout is registered, then test Chrome and Firefox user agents with the devtools hook absent to assert the delayed suggestion logs correctly. Reuse the existing timer and logging test utilities, and cover both supported browser branches without changing the implementation.
🤖 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 `@src/core/ApolloClient.ts`:
- Around line 1173-1195: Harden the ApolloClient devtools detection flow around
the user-agent and protocol checks: guard navigator, document, location, and
window/top/self access before dereferencing them, and ensure the delayed
setTimeout callback safely handles window teardown before reading the devtools
hook. Preserve the existing Chrome/Firefox URL selection and devtools warning
behavior when all browser members are available.
---
Nitpick comments:
In `@src/core/ApolloClient.ts`:
- Around line 1172-1199: Add regression tests around the scheduling gate in the
Apollo client devtools suggestion logic: use an unsupported or jsdom user agent
to assert no timeout is registered, then test Chrome and Firefox user agents
with the devtools hook absent to assert the delayed suggestion logs correctly.
Reuse the existing timer and logging test utilities, and cover both supported
browser branches without changing the implementation.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e24b4c32-f71d-4c0b-ab1f-b9a28d7e4801
📒 Files selected for processing (1)
src/core/ApolloClient.ts
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/client@4.2.8 ### Patch Changes - [#13349](#13349) [`501a33b`](501a33b) Thanks [@jerelmiller](https://github.com/jerelmiller)! - Prevent the `setTimeout` in `connectToDevtools` that shows the devtools suggestion from firing when the user agent does not match Chrome or Firefox. This check was previously done inside the `setTimeout` which meant the timer was scheduled for environments where we'd never show the message anyways. For test environments, this could cause flaky tests when that `setTimeout` outlived the tests and ran after any virtual DOM was torn down and removed. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #13344
The
connectToDevtoolsmethod insideApolloClientruns asetTimeoutthen checks thewindow.navigator.userAgentto see if it matches a supported platform before logging the message. As #13344 points out, this can be problematic in test environments since thatsetTimeoutcan linger. Even worse is when thatsetTimeoutoutlives the virtual DOM construction sowindowdoesn't exist anymore which could cause crashes.This fix checks
window.navigator.userAgentoutside thesetTimeoutand prevents it from scheduling if we're in a non Chrome/Firefox environment.Summary by CodeRabbit