perf: cache deviceOf() and isBot() per request via WeakMap#1088
perf: cache deviceOf() and isBot() per request via WeakMap#1088hugo-ccabral wants to merge 1 commit intomainfrom
Conversation
- Add WeakMap<Request, Device> cache for deviceOf() and WeakMap<Request, boolean> for isBot() - Avoid new URL() construction in deviceOf() when cf-device-type header is present (fast path) - Only parse URL search params as last resort (URLSearchParams instead of full URL object) - WeakMap ensures automatic cleanup when Request objects are GC'd - CPU profile showed ua-parser-js at 0.61 percent, called repeatedly for the same request across different loaders Co-authored-by: Cursor <cursoragent@cursor.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughRefactored Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
utils/userAgent.ts (2)
24-49: Multi-stage fallback logic is sound.One subtlety worth documenting: Cloudflare's
cf-device-typecan return"desktop", which is not a key inideviceToDevice. It still resolves correctly thanks to the?? "desktop"fallback on line 47, but it relies on the default rather than an explicit mapping. Consider addingdesktop: "desktop"toideviceToDevicefor clarity.Proposed change
const ideviceToDevice: Record<string, Device> = { mobile: "mobile", tablet: "tablet", + desktop: "desktop", console: "desktop", smarttv: "desktop", wearable: "desktop", embedded: "desktop", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/userAgent.ts` around lines 24 - 49, The cf-device-type value "desktop" can bypass the ideviceToDevice mapping and rely on the final "?? 'desktop'" fallback; update ideviceToDevice to include an explicit mapping for "desktop": "desktop" (so ideviceToDevice["desktop"]="desktop") and optionally add a brief code comment near the ideviceToDevice declaration explaining why cfDeviceHint (cf-device-type) needs this explicit mapping; no other logic changes in the cfDeviceHint/UAParser/device fallback or deviceCache usage are required.
62-77:UABotParser.setUA(ua)mutates a module-level singleton — safe today but fragile.The chained synchronous call
UABotParser.setUA(ua).getBrowser()is safe in a single-threaded JS runtime because no interleaving can occur within one expression. However, unlikedeviceOf(which creates a freshnew UAParser(ua)on line 32), this pattern silently breaks if anawaitis ever inserted betweensetUAandgetBrowser. Consider either: (a) adding a comment noting the synchronous-chain requirement, or (b) creating a newUAParser(Bots)instance per cache-miss to match the pattern used indeviceOf.Also, extracting the
user-agentheader once and reusing it across the KNOWN_BOTS check and the UAParser path would avoid redundantheaders.getcalls:Proposed refactor
let result = false; + const ua = req.headers.get("user-agent") || ""; const fromCloudFlare = req.headers.get("cf-verified-bot"); if (fromCloudFlare === "true") { result = true; } else if ( - KNOWN_BOTS.some((bot) => req.headers.get("user-agent")?.includes(bot)) + KNOWN_BOTS.some((bot) => ua.includes(bot)) ) { result = true; } else { - const ua = req.headers.get("user-agent") || ""; - const browser = UABotParser.setUA(ua).getBrowser() as unknown as { + // setUA mutates the singleton — keep this a single synchronous chain + const browser = new UAParser(ua, Bots).getBrowser() as unknown as { type: string; }; result = browser.type === "bot"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/userAgent.ts` around lines 62 - 77, Extract the user-agent once into a local variable (e.g., const ua = req.headers.get("user-agent") || "") and use that for the KNOWN_BOTS check instead of calling req.headers.get repeatedly; replace the singleton-mutating call UABotParser.setUA(ua).getBrowser() with creating a fresh parser instance (new UAParser(ua).getBrowser()) like deviceOf does, and set result = (browser.type === "bot"); this removes the fragile module-level mutation and avoids the synchronous-chain hazard while keeping KNOWN_BOTS, UABotParser.setUA, getBrowser, deviceOf and UAParser references to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@utils/userAgent.ts`:
- Around line 24-49: The cf-device-type value "desktop" can bypass the
ideviceToDevice mapping and rely on the final "?? 'desktop'" fallback; update
ideviceToDevice to include an explicit mapping for "desktop": "desktop" (so
ideviceToDevice["desktop"]="desktop") and optionally add a brief code comment
near the ideviceToDevice declaration explaining why cfDeviceHint
(cf-device-type) needs this explicit mapping; no other logic changes in the
cfDeviceHint/UAParser/device fallback or deviceCache usage are required.
- Around line 62-77: Extract the user-agent once into a local variable (e.g.,
const ua = req.headers.get("user-agent") || "") and use that for the KNOWN_BOTS
check instead of calling req.headers.get repeatedly; replace the
singleton-mutating call UABotParser.setUA(ua).getBrowser() with creating a fresh
parser instance (new UAParser(ua).getBrowser()) like deviceOf does, and set
result = (browser.type === "bot"); this removes the fragile module-level
mutation and avoids the synchronous-chain hazard while keeping KNOWN_BOTS,
UABotParser.setUA, getBrowser, deviceOf and UAParser references to locate the
change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="utils/userAgent.ts">
<violation number="1" location="utils/userAgent.ts:39">
P2: URLSearchParams does not strip hash fragments, meaning `deviceHint` may incorrectly include `#hash`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!device) { | ||
| const qIdx = request.url.indexOf("?"); | ||
| if (qIdx !== -1) { | ||
| const params = new URLSearchParams(request.url.slice(qIdx)); |
There was a problem hiding this comment.
P2: URLSearchParams does not strip hash fragments, meaning deviceHint may incorrectly include #hash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/userAgent.ts, line 39:
<comment>URLSearchParams does not strip hash fragments, meaning `deviceHint` may incorrectly include `#hash`.</comment>
<file context>
@@ -15,41 +15,67 @@ const ideviceToDevice: Record<string, Device> = {
+ if (!device) {
+ const qIdx = request.url.indexOf("?");
+ if (qIdx !== -1) {
+ const params = new URLSearchParams(request.url.slice(qIdx));
+ device = params.get("deviceHint") || "desktop";
+ } else {
</file context>
| const params = new URLSearchParams(request.url.slice(qIdx)); | |
| const hashIdx = request.url.indexOf("#", qIdx); | |
| const params = new URLSearchParams( | |
| request.url.slice(qIdx, hashIdx !== -1 ? hashIdx : undefined) | |
| ); |
1 participant
Summary
WeakMap<Request, Device>cache fordeviceOf()— same Request object returns cached result across multiple loadersWeakMap<Request, boolean>cache forisBot()— same patternnew URL()indeviceOf()whencf-device-typeheader is present (Cloudflare fast path). Previously always constructed a full URL object even when the header was availableURLSearchParamsinstead ofnew URL()as last resort fordeviceHintquery param — avoids full URL parsing when only the query string is neededContext
CPU profile showed ua-parser-js at 0.61% of total CPU (1,044 hits).
deviceOf()andisBot()are called via lazy getters infnContextFromHttpContext(), but a newFnContextis created for each loader (viaapplyProps), so the lazy cache doesn't persist across loaders for the same request. The WeakMap cache fixes this.Memory profile showed
isBot()at 3 MB (0.3%) anddeviceOf()at 2.3 MB (0.3%) in allocation timeline.Test plan
Summary by cubic
Caches deviceOf() and isBot() per Request using WeakMap to avoid repeated UA parsing and speed up loaders. Also skips full URL parsing by using the cf-device-type fast path and URLSearchParams for the deviceHint fallback.
Written for commit d074073. Summary will update on new commits.
Summary by CodeRabbit