fix: 增加 window 使用前的判断 #767
Conversation
WalkthroughThe changes in this pull request involve modifications to several functions across multiple files to enhance the safety of property access on the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
plugin-packages/alipay-downgrade/src/native-log.ts (1)
Line range hint
31-33: Consider enhancing error logging detailsThe catch block could provide more context about why the native logging failed.
Consider this enhancement:
try { ap?.call('localLog', content); } catch (e) { - console.error(e); + console.error('[Native Log Failed]', { + reason: e, + environment: { + isAlipayMiniApp: isAlipayMiniApp(), + hasWindow: typeof window !== 'undefined', + hasAlipayBridge: typeof window?.AlipayJSBridge !== 'undefined', + }, + }); }packages/effects-core/src/utils/device.ts (1)
Add safety checks for browser globals across device detection functions
Several functions directly access browser globals without proper existence checks:
isIOS(): Directly usesnavigator.platformwithout checking ifnavigatorexistsisIOSByUA(): Usesnavigator.userAgentwithout safety checkisAndroid(): Haswindowcheck but directly accessesnavigator.userAgentisSimulatorCellPhone(): Similar toisAndroid()Suggested safety improvements:
- Add
typeof navigator !== 'undefined'checks before accessingnavigatorproperties- Ensure consistent window/document/screen existence checks across all functions
- Consider consolidating the safety checks into a shared utility function
🔗 Analysis chain
Line range hint
1-42: Verify safety checks in other browser-dependent functionsWhile we're adding window checks, other functions in this file also access browser globals and might benefit from similar safety improvements.
Let's check for other potential unsafe browser global access:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct access to browser globals without safety checks ast-grep --pattern 'navigator.$_' packages/effects-core/src/utils/device.ts ast-grep --pattern 'window.$_' packages/effects-core/src/utils/device.ts ast-grep --pattern 'document.$_' packages/effects-core/src/utils/device.ts ast-grep --pattern 'screen.$_' packages/effects-core/src/utils/device.tsLength of output: 1128
plugin-packages/alipay-downgrade/src/utils.ts (2)
32-32: LGTM! Consider additional browser API safety checks.The optional chaining for
window?.AlipayJSBridgeis a good addition for SSR safety. However, the function also uses other browser APIs likeperformance.now()that might need similar safety checks.Consider wrapping browser API calls with safety checks:
- const systemStartTime = performance.now(); + const systemStartTime = typeof performance !== 'undefined' ? performance.now() : Date.now();
Line range hint
89-97: Enhance safety checks for browser APIs.While the window check is good, the function still directly accesses
documentwithout similar safety checks. Consider making the browser API access more robust.Consider applying these improvements:
function registerEvent (options: DowngradeOptions) { const { autoPause } = options; - // SSR时window对象不存在 需要判断 - window && window.addEventListener('unload', () => { + // Check for browser environment + const isBrowser = typeof window !== 'undefined' && typeof document !== 'undefined'; + if (!isBrowser) { + return; + } + + window.addEventListener('unload', () => { getActivePlayers().forEach(player => player.dispose()); }); if (autoPause) { document.addEventListener('pause', pauseAllActivePlayers); document.addEventListener('resume', resumePausedPlayers); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/effects-core/src/utils/device.ts(2 hunks)plugin-packages/alipay-downgrade/src/native-log.ts(1 hunks)plugin-packages/alipay-downgrade/src/utils.ts(1 hunks)
🔇 Additional comments (2)
plugin-packages/alipay-downgrade/src/native-log.ts (1)
16-16: LGTM! The optional chaining addition improves safety.
The change prevents potential runtime errors when window is undefined in non-browser environments.
Let's verify the usage context of AlipayJSBridge across the codebase:
✅ Verification successful
Optional chaining is correctly applied and consistent with other usages
The verification shows that:
- In
alipay-downgrade/utils.ts, the same optional chaining pattern is already used:window?.AlipayJSBridge - In
orientation-transformer/device-orientation.ts, explicit undefined checks are used:typeof window.AlipayJSBridge !== 'undefined' - The type definition in
vendors.d.tscorrectly declaresAlipayJSBridgeas a window property
The change is safe and follows the established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AlipayJSBridge usage patterns to ensure consistent handling
# Search for other direct AlipayJSBridge accesses that might need similar safety checks
rg -l "AlipayJSBridge" --type ts
# Check for any existing optional chaining patterns with window
rg "window\?\." --type ts
Length of output: 558
Script:
#!/bin/bash
# Let's check the implementation details of AlipayJSBridge usage in other files
# Check utils.ts implementation
echo "=== alipay-downgrade/utils.ts ==="
rg -A 5 "AlipayJSBridge" plugin-packages/alipay-downgrade/src/utils.ts
# Check device-orientation.ts implementation
echo "=== orientation-transformer/device-orientation.ts ==="
rg -A 5 "AlipayJSBridge" plugin-packages/orientation-transformer/src/device-orientation.ts
# Check vendors.d.ts type definitions
echo "=== types/vendors.d.ts ==="
rg -A 5 "AlipayJSBridge" types/vendors.d.ts
Length of output: 1131
plugin-packages/alipay-downgrade/src/utils.ts (1)
32-32: Verify consistent window safety checks across the codebase.
Let's ensure that similar window safety checks are consistently implemented across all related files.
Also applies to: 89-97
✅ Verification successful
Based on the search results, I can now generate the final response:
Window access is consistently protected across the codebase
The review comment's concern about window safety checks has been verified. The codebase shows consistent patterns:
-
In the same package (
alipay-downgrade), window access is properly guarded:src/utils.ts:window?.AlipayJSBridgesrc/native-log.ts:window?.AlipayJSBridge
-
Other window accesses in the codebase are either:
- In browser-only contexts (demo/test files)
- Used with feature detection (e.g.,
typeof window.AlipayJSBridge !== 'undefined') - Or properly guarded with optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct window access patterns that might need safety checks
echo "Checking for potential unsafe window access patterns..."
rg -g '*.{ts,js}' 'window\.' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
# Search for AlipayJSBridge access patterns
echo "Checking AlipayJSBridge access patterns..."
rg -g '*.{ts,js}' 'AlipayJSBridge' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
# Look for other browser API usage that might need checks
echo "Checking other browser API usage..."
rg -g '*.{ts,js}' 'document\.|performance\.' --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}'
Length of output: 52171
| @@ -39,5 +39,5 @@ export function isAlipayMiniApp (): boolean { | |||
| } | |||
|
|
|||
| export function isWechatMiniApp () { | |||
| return window.__wxjs_environment === 'miniprogram'; | |||
| return window?.__wxjs_environment === 'miniprogram'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent window safety check pattern
While optional chaining helps, it doesn't prevent ReferenceError when window is undefined. Let's maintain consistency with other functions.
Apply this diff:
- return window?.__wxjs_environment === 'miniprogram';
+ return typeof window !== 'undefined' && window.__wxjs_environment === 'miniprogram';📝 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.
| return window?.__wxjs_environment === 'miniprogram'; | |
| return typeof window !== 'undefined' && window.__wxjs_environment === 'miniprogram'; |
| @@ -23,11 +23,11 @@ export function isIOSByUA () { | |||
| } | |||
|
|
|||
| export function isAndroid (): boolean { | |||
| return /\b[Aa]ndroid\b/.test(navigator.userAgent); | |||
| return window && /\b[Aa]ndroid\b/.test(navigator.userAgent); | |||
There was a problem hiding this comment.
Enhance window and navigator safety checks
The current window check is insufficient and could still throw a ReferenceError. Additionally, navigator should be checked before access.
Apply this diff to improve safety:
- return window && /\b[Aa]ndroid\b/.test(navigator.userAgent);
+ return typeof window !== 'undefined' && typeof navigator !== 'undefined' && /\b[Aa]ndroid\b/.test(navigator.userAgent);📝 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.
| return window && /\b[Aa]ndroid\b/.test(navigator.userAgent); | |
| return typeof window !== 'undefined' && typeof navigator !== 'undefined' && /\b[Aa]ndroid\b/.test(navigator.userAgent); |
| } | ||
|
|
||
| export function isSimulatorCellPhone (): boolean { | ||
| return isAndroid() || /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent); | ||
| return isAndroid() || (window && /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent)); |
There was a problem hiding this comment.
Optimize safety checks and reduce redundancy
The function has similar safety issues as isAndroid() and includes redundant checks through the isAndroid() call.
Apply this diff to improve safety and efficiency:
- return isAndroid() || (window && /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent));
+ return typeof window !== 'undefined' && typeof navigator !== 'undefined' &&
+ (/\b[Aa]ndroid\b/.test(navigator.userAgent) || /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent));📝 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.
| return isAndroid() || (window && /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent)); | |
| return typeof window !== 'undefined' && typeof navigator !== 'undefined' && | |
| (/\b[Aa]ndroid\b/.test(navigator.userAgent) || /\b(iPad|iPhone|iPod)\b/.test(navigator.userAgent)); |
Summary by CodeRabbit
Bug Fixes
windowobject access in multiple functions to prevent runtime errors in non-browser environments.AlipayJSBridge.Documentation
windowobject during server-side rendering (SSR).