fix: login expired modal z-index #5145
Conversation
|
|
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
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vueOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue(2 hunks)
🔇 Additional comments (1)
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue (1)
Line range hint 31-58: Verify modal visibility across different scenarios
While the implementation ensures highest z-index, please verify:
- Modal visibility when multiple modals/overlays are present
- Behavior with dynamically added elements
- Interaction with third-party components that might manipulate z-index
✅ Verification successful
Let me gather more information about z-index usage and modal implementations.
Based on the comprehensive search results, I can now generate the final response:
Implementation of z-index calculation is appropriate for login expired modal
The current implementation ensures the login expired modal appears on top by:
- Using
getMaxZIndex()to dynamically calculate the highest z-index in the DOM - Being used consistently across different UI frameworks (Antd, Element, Naive)
- Working in conjunction with the app's
loginExpiredModesetting which can be configured as 'modal' or 'page'
The z-index calculation is particularly important because:
- The modal needs to appear above all other UI elements when login expires
- The app has multiple layers with different z-index values (layout header: 200, sidebar: varies, other modals: 1000)
- The implementation handles dynamic z-index values from third-party components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might compete for z-index
rg -l "z-index|zIndex" --type vue
# Look for other modal implementations
ast-grep --pattern 'Modal'
Length of output: 488
Script:
#!/bin/bash
# Search for z-index related code with context
rg "z-index|zIndex" -A 3 -B 3 --no-ignore-vcs
# Look for modal component implementations with context
ast-grep --pattern $'const Modal = defineComponent({
$$$
})'
# Search for other login-expired or session-related modals
rg -i "login.*expired|session.*expired" -A 3 -B 3
Length of output: 59185
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue
Outdated
Show resolved
Hide resolved
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue (1)
38-40: Use English for code commentsFor better maintainability and international collaboration, consider translating the Chinese comment to English:
-/** - * 获取最大的zIndex值 - */ +/** + * Get the maximum z-index value from the document + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue(3 hunks)
🔇 Additional comments (6)
packages/effects/common-ui/src/ui/authentication/login-expired-modal.vue (6)
4-4: LGTM: Required imports added correctly
The necessary Vue imports are properly included.
11-11: LGTM: Props interface properly extended
The zIndex property is correctly typed as an optional number.
18-21: LGTM: Appropriate default value set
The default z-index of 0 is a sensible choice.
34-36: Consider using cached computed property for better performance
The current implementation may impact performance as noted in previous reviews.
64-64: LGTM: Proper z-index binding
The z-index is correctly bound to the computed property.
41-52: 🛠️ Refactor suggestion
Optimize z-index calculation performance
The current implementation queries and processes all DOM elements, which is inefficient.
Consider this optimized version:
function calcZIndex() {
let maxZ = 0;
- const elements = document.querySelectorAll('*');
- [...elements].forEach((element) => {
+ const elements = document.querySelectorAll('div[style*="z-index"], .modal, .popup, .overlay');
+ elements.forEach((element) => {
const style = window.getComputedStyle(element);
const zIndex = style.getPropertyValue('z-index');
if (zIndex && !Number.isNaN(Number.parseInt(zIndex))) {
maxZ = Math.max(maxZ, Number.parseInt(zIndex));
}
});
return maxZ + 1;
}Likely invalid or redundant comment.
Description
确保令牌过期登录弹窗的层级最高。
fix #5089
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Documentation