feat: support global search in multiple environments and fix pipeline errors #2629
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
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 (
|
zzcr
changed the title
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
examples/sites/src/main.js (1)
52-55: Consider environment-specific search configurationsThe current implementation assumes default search behavior for non-open environments. Consider adding environment-specific configurations.
Consider creating a configuration file to manage environment-specific search settings:
// searchConfig.js export const getSearchConfig = (envTarget) => ({ open: { useGlobalSearch: false, // other open env specific settings }, default: { useGlobalSearch: true, // other env specific settings } }[envTarget] || config.default);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/sites/package.json(2 hunks)examples/sites/src/main.js(1 hunks)examples/sites/src/tools/docsearch.js(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
examples/sites/src/main.js
46-46: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| const findUrlLink = (target) => { | ||
| if (target?.nodeName?.toLocaleLowerCase?.() === 'a') { | ||
| return target.getAttribute('href') | ||
| } else if (target?.parentElement) { | ||
| return findUrlLink(target.parentElement) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for recursive DOM traversal
The findUrlLink function could potentially cause a stack overflow with deeply nested DOM structures. Consider adding a maximum depth limit.
-const findUrlLink = (target) => {
+const findUrlLink = (target, depth = 0) => {
+ const MAX_DEPTH = 10;
+ if (depth > MAX_DEPTH) return null;
if (target?.nodeName?.toLocaleLowerCase?.() === 'a') {
return target.getAttribute('href')
} else if (target?.parentElement) {
- return findUrlLink(target.parentElement)
+ return findUrlLink(target.parentElement, depth + 1)
}
}📝 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.
| const findUrlLink = (target) => { | |
| if (target?.nodeName?.toLocaleLowerCase?.() === 'a') { | |
| return target.getAttribute('href') | |
| } else if (target?.parentElement) { | |
| return findUrlLink(target.parentElement) | |
| } | |
| } | |
| const findUrlLink = (target, depth = 0) => { | |
| const MAX_DEPTH = 10; | |
| if (depth > MAX_DEPTH) return null; | |
| if (target?.nodeName?.toLocaleLowerCase?.() === 'a') { | |
| return target.getAttribute('href') | |
| } else if (target?.parentElement) { | |
| return findUrlLink(target.parentElement, depth + 1) | |
| } | |
| } |
| export const doSearhEverySite = () => { | ||
| window.addEventListener('click', (event) => { | ||
| const target = event.target | ||
| if (isAlgoliaHitDom(target)) { | ||
| const openUrl = findUrlLink(target) | ||
| if (openUrl) { | ||
| const urlObj = new URL(openUrl) | ||
| event.preventDefault() | ||
| window.location.href = openUrl.replace(urlObj.origin, window.location.origin) | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add security measures for URL handling
The URL manipulation could be vulnerable to open redirect attacks. Consider implementing URL validation and allowlisting.
export const doSearhEverySite = () => {
+ const isValidDomain = (url) => {
+ const allowedDomains = ['your-domain.com', 'other-domain.com'];
+ return allowedDomains.some(domain => url.includes(domain));
+ };
window.addEventListener('click', (event) => {
const target = event.target
if (isAlgoliaHitDom(target)) {
const openUrl = findUrlLink(target)
- if (openUrl) {
+ if (openUrl && isValidDomain(openUrl)) {
const urlObj = new URL(openUrl)
event.preventDefault()
window.location.href = openUrl.replace(urlObj.origin, window.location.origin)
+ } else {
+ console.warn('Invalid URL detected:', openUrl);
}
}
})
}Committable suggestion skipped: line range outside the PR's diff.
| docsearch({ | ||
| appId: 'AGPA5UXHMH', | ||
| apiKey: '5fa09fc20270efa61d68e2c2eb0f56df', | ||
| indexName: 'opentiny', | ||
| container: '.search-box', | ||
| debug: false | ||
| }) |
There was a problem hiding this comment.
Critical: Move API credentials to environment variables
The Algolia API credentials are exposed in client-side code, which poses a security risk. These should be moved to environment variables.
docsearch({
- appId: 'AGPA5UXHMH',
- apiKey: '5fa09fc20270efa61d68e2c2eb0f56df',
+ appId: import.meta.env.VITE_ALGOLIA_APP_ID,
+ apiKey: import.meta.env.VITE_ALGOLIA_API_KEY,
indexName: 'opentiny',
container: '.search-box',
debug: false
})📝 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.
| docsearch({ | |
| appId: 'AGPA5UXHMH', | |
| apiKey: '5fa09fc20270efa61d68e2c2eb0f56df', | |
| indexName: 'opentiny', | |
| container: '.search-box', | |
| debug: false | |
| }) | |
| docsearch({ | |
| appId: import.meta.env.VITE_ALGOLIA_APP_ID, | |
| apiKey: import.meta.env.VITE_ALGOLIA_API_KEY, | |
| indexName: 'opentiny', | |
| container: '.search-box', | |
| debug: false | |
| }) |
🧰 Tools
🪛 Gitleaks (8.21.2)
46-46: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| "@docsearch/css": "^3.8.0", | ||
| "@docsearch/react": "npm:@docsearch/css" |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
The @docsearch/react dependency configuration is incorrect and should be fixed
The codebase is using @docsearch/js and @docsearch/css directly, as evidenced in main.js:
import docsearch from '@docsearch/js'
import '@docsearch/css'The @docsearch/react dependency is incorrectly aliased to @docsearch/css and should be removed since:
- The project uses the vanilla JS version (
@docsearch/js), not the React version - CSS is already properly imported via the direct
@docsearch/cssdependency
"@docsearch/js": "^3.8.0",
"@docsearch/css": "^3.8.0",
- "@docsearch/react": "npm:@docsearch/css"🔗 Analysis chain
Verify @docsearch/react dependency configuration
The dependency configuration appears unusual:
- @docsearch/react is aliased to @docsearch/css
- @docsearch/css is also listed as a direct dependency
This might lead to duplicate CSS imports and potential version conflicts.
Consider using the proper @docsearch/react package instead of the CSS alias:
"@docsearch/js": "^3.8.0",
"@docsearch/css": "^3.8.0",
- "@docsearch/react": "npm:@docsearch/css"
+ "@docsearch/react": "^3.8.0"The previous commands didn't provide enough context. Let's search more broadly for docsearch usage and check package configurations to understand the dependency setup better.
Let's examine how docsearch is actually being used in the project to better understand the dependency configuration.
Let's examine how docsearch is actually implemented in the main.js file and if there are any other relevant files to understand the dependency setup better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential duplicate CSS imports
rg -l "from ['|\"]@docsearch/(css|react)['|\"]"
# Verify package resolution
npm ls @docsearch/css @docsearch/react
Length of output: 146
Script:
#!/bin/bash
# Let's check for any imports or usage patterns of @docsearch in the codebase
rg -l "@docsearch" --type js --type ts --type vue
# Check if there are any other package.json files that might be using @docsearch
fd package.json --type f --exec grep -l "@docsearch" {}
# Look for any CSS imports related to docsearch
rg -l "docsearch" --type css
Length of output: 223
Script:
#!/bin/bash
# Let's examine the content of style.css to understand how docsearch is being used
cat examples/sites/src/style.css
# Check the exact content around docsearch in package.json for more context
grep -A 5 -B 5 "@docsearch" examples/sites/package.json
# Look for any JavaScript/TypeScript files that might be using docsearch
rg -l "docsearch" --type js --type ts
Length of output: 1830
Script:
#!/bin/bash
# Let's check the main.js file to see how docsearch is being used
cat examples/sites/src/main.js
# Check if there are any other JS files that might contain docsearch configuration
rg -l "docsearch" --type js --type ts -g '!main.js'
# Look for any Vue components that might be using docsearch
fd -e vue -x grep -l "docsearch" {}
Length of output: 2682
examples/sites/src/main.js
Outdated
| @@ -33,21 +33,25 @@ import css from 'highlight.js/lib/languages/css' | |||
| import html from 'highlight.js/lib/languages/xml' | |||
| import docsearch from '@docsearch/js' | |||
| import '@docsearch/css' | |||
| import { doSearhEverySite } from './tools/docsearch' | |||
There was a problem hiding this comment.
| import { doSearhEverySite } from './tools/docsearch' | |
| import { doSearchEverySite } from './tools/docsearch' |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/sites/src/main.js (1)
52-54: Consider improving environment handling and error handlingWhile the functionality is correct, consider these improvements:
- Use a more explicit environment constant instead of the string literal 'open'
- Add error handling for the
doSearchEverySitefunction- Document supported environments in comments
+// Define supported environments +const ENVIRONMENTS = { + OPEN: 'open', + INTERNAL: 'internal', + LOCAL: 'local' +} -if (envTarget !== 'open') { +if (envTarget !== ENVIRONMENTS.OPEN) { // 支持本地开发和内网使用全局搜索 - doSearchEverySite() + try { + doSearchEverySite() + } catch (error) { + console.error('Failed to initialize global search:', error) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/sites/src/main.js(1 hunks)examples/sites/src/tools/docsearch.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/sites/src/tools/docsearch.js
🧰 Additional context used
🪛 Gitleaks (8.21.2)
examples/sites/src/main.js
46-46: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
examples/sites/src/main.js (3)
36-36: LGTM: Import statement is properly placed
The import of the new search functionality follows proper conventions.
44-50:
Critical: Move API credentials to environment variables
The Algolia API credentials are still exposed in client-side code, which poses a security risk. These should be moved to environment variables.
docsearch({
- appId: 'AGPA5UXHMH',
- apiKey: '5fa09fc20270efa61d68e2c2eb0f56df',
+ appId: import.meta.env.VITE_ALGOLIA_APP_ID,
+ apiKey: import.meta.env.VITE_ALGOLIA_API_KEY,
indexName: 'opentiny',
container: '.search-box',
debug: false
})🧰 Tools
🪛 Gitleaks (8.21.2)
46-46: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
36-36: Verify doSearchEverySite implementation
Let's verify the implementation of the new search functionality.
Also applies to: 52-54
✅ Verification successful
Implementation verified and looks correct
The doSearchEverySite function is properly implemented in the tools directory. It:
- Adds a click event listener to handle search result interactions
- Correctly identifies Algolia search hits using the
HIT_CLASSconstant - Handles URL transformation to maintain consistent origin across site navigation
- Implements proper DOM traversal to find anchor links
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of doSearchEverySite
echo "Checking doSearchEverySite implementation..."
fd docsearch.js --type f --exec cat {}
Length of output: 923
… errors (#2629) * feat: support global search in multiple environments and fix pipeline errors * feat: support global search in multiple environments and fix pipeline errors
… errors
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes