fix(isMobile): performance #646
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough此次更改将移动端检测的实现方式由手动正则匹配用户代理字符串，替换为调用外部 Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/isMobile.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in ".eslintrc.js » /node_modules/.pnpm/@umijs+fabric@3.0.0/node_modules/@umijs/fabric/dist/eslint.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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
🧹 Nitpick comments (1)
src/isMobile.ts (1)
5-11: 命名导出可提升调试可读性匿名默认导出在 DevTools Stack Trace 中显示为
anonymous。
考虑改为具名函数以便排查问题，例如：export default function isMobileCached(ua?: string) { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/isMobile.ts(1 hunks)
🔇 Additional comments (1)
package.json (1)
40-43: 新增依赖需确认打包体积与类型支持
is-mobile会被打入浏览端 bundle，如果最终产物面向浏览器，请确认：
- 该库的 gzip 体积可接受，避免因一个简单检测函数增加过多 KB。
@types/is-mobile在 CI 中是否能自动解析，或是否需额外安装。
若项目tsconfig未开启esModuleInterop，import isMobile from 'is-mobile'也可能导致类型或运行时异常，需要验证。
| @@ -1,16 +1,11 @@ | |||
| import isMobile from 'is-mobile'; | |||
There was a problem hiding this comment.
CommonJS 默认导出可能导致类型/运行时问题
is-mobile 的类型声明采用 export = isMobile，在未开启 esModuleInterop 时应使用
import isMobile = require('is-mobile');或
import * as isMobile from 'is-mobile';否则编译阶段会出现 TS1192，运行时也可能拿到 { default: [Function] } 而非函数。
🤖 Prompt for AI Agents
In src/isMobile.ts at line 1, the import statement uses ES module default import
syntax for a CommonJS module, which can cause TypeScript error TS1192 and
runtime issues. Replace the current import with either "import isMobile =
require('is-mobile');" or "import * as isMobile from 'is-mobile';" to correctly
import the CommonJS default export without enabling esModuleInterop.
| let cached: boolean; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
缓存变量作用域过宽，SSR 场景下存在误判风险
cached 是模块级静态变量。在 Node SSR 中，多个 HTTP 请求会共享同一模块实例：
首个移动端请求若将 cached 置为 true，后续桌面请求也会错误地获得 true。
建议：
-export default () => {
- if (typeof cached === 'undefined') {
- cached = isMobile();
- }
- return cached;
-};
+export default (ua?: string) => {
+ // 若显式传入 UA，不走全局缓存
+ if (ua) {
+ return isMobile({ ua });
+ }
+ if (typeof cached === 'undefined') {
+ cached = isMobile();
+ }
+ return cached;
+};这样浏览端依旧命中缓存，而服务端可按请求 UA 调用。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let cached: boolean; | |
| let cached: boolean; | |
| export default (ua?: string) => { | |
| // 若显式传入 UA，不走全局缓存 | |
| if (ua) { | |
| return isMobile({ ua }); | |
| } | |
| if (typeof cached === 'undefined') { | |
| cached = isMobile(); | |
| } | |
| return cached; | |
| }; |
🤖 Prompt for AI Agents
In src/isMobile.ts at lines 3 to 4, the cached variable is declared at the
module level, causing shared state across requests in SSR environments and
leading to incorrect mobile detection. To fix this, remove the module-level
cached variable and instead implement caching scoped per request or per function
call, ensuring that server-side rendering does not share cached results between
different user agents while still allowing caching on the client side.
|
CI failed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/hooks.test.tsx (1)
516-523: 使用 try/finally 包裹断言并改用 mockReturnValue，可避免污染其它用例如果断言抛异常，
mockRestore()不会被执行，可能导致后续测试继承修改后的userAgent。同时，对 getter 的 spy 使用mockReturnValue语义更清晰。可考虑如下微调：- const navigatorSpy = jest - .spyOn(navigator, 'userAgent', 'get') - .mockImplementation(() => 'Android'); - - const { container } = render(<Demo />); - expect(container.textContent).toBe('mobile'); - - navigatorSpy.mockRestore(); + const navigatorSpy = jest + .spyOn(window.navigator, 'userAgent', 'get') + .mockReturnValue('Android'); + + try { + const { container } = render(<Demo />); + expect(container.textContent).toBe('mobile'); + } finally { + navigatorSpy.mockRestore(); + }[建议可选优化]
guoyunhe
force-pushed
the
is-mobile-performance
branch
from
9fbae28 to
e85ed91
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #646 +/- ##
==========================================
+ Coverage 90.11% 90.19% +0.08%
==========================================
Files 36 36
Lines 880 877 -3
Branches 288 299 +11
==========================================
- Hits 793 791 -2
+ Misses 85 84 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
新功能
优化
测试