fix(theme): use withBase to resolve path for api.json #105
Conversation
WalkthroughThe pull request includes changes primarily focused on the configuration and usage of API-related properties in a VitePress setup. Key modifications involve updating API URLs in the VitePress configuration file, refining the use of the Changes
Possibly related PRs
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 (
|
Romakita
force-pushed
the
fix-api-json-doc-resolution
branch
2 times, most recently
from
71dbc68
to
4048b9f
Compare
Romakita
force-pushed
the
fix-api-json-doc-resolution
branch
from
4048b9f
to
6a7db75
Compare
![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
🧹 Outside diff range and nitpick comments (4)
docs/api.md (1)
15-15: LGTM! Improved prop passing pattern.
The explicit prop binding (:modules and :symbol-types) is a better approach than v-bind spread as it makes the component interface more predictable and maintainable.
Consider adding prop validation in the Api component to ensure type safety for these explicitly passed props.
packages/theme/composables/config/__mocks__/useThemeConfig.ts (1)
15-17: Consider enhancing the mock implementation of withBase
The current mock implementation of withBase simply returns the input value unchanged. This might not adequately test the path resolution behavior in the actual implementation. Consider adding basic path joining logic or at least documenting why this simplified mock is sufficient.
export function withBase(value: string) {
- return value;
+ // Mock implementation: Simplified version of path resolution
+ // Actual implementation would handle base path joining
+ return value.startsWith('http') ? value : `/base/${value}`;
}packages/theme/composables/api/useApiContent.ts (2)
10-10: Unnecessary mutable declaration.
The change from const to let seems unnecessary as the variable is not reassigned within the function scope. Consider keeping it as const to maintain immutability.
- let apiUrl = theme.value.apiUrl;
+ const apiUrl = theme.value.apiUrl;Line range hint 10-16: Missing path resolution with withBase.
According to the PR objective, the path for api.json should be resolved using withBase to support deployments in subdirectories. However, the current implementation doesn't handle this.
+ import { withBase } from 'vitepress'
export function useApiContent() {
const theme = useThemeConfig();
- let apiUrl = theme.value.apiUrl;
+ const apiUrl = withBase(theme.value.apiUrl);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- docs/.vitepress/config.mts (1 hunks)
- docs/api.md (1 hunks)
- packages/theme/composables/api/useApiContent.ts (1 hunks)
- packages/theme/composables/config/mocks/useThemeConfig.ts (1 hunks)
- packages/theme/composables/config/useThemeConfig.ts (1 hunks)
- packages/theme/package.json (1 hunks)
- test/vitepress.client.ts (1 hunks)
- vitest.config.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/package.json
🔇 Additional comments (6)
packages/theme/composables/config/useThemeConfig.ts (1)
1-1: LGTM!
Clean addition of the withBase import alongside the existing useData import from vitepress/client.
packages/theme/composables/config/__mocks__/useThemeConfig.ts (1)
Line range hint 1-13: Verify if mock URLs should be processed with withBase
Since the PR objective is to fix path resolution using withBase, consider if these hardcoded URLs in the mock should also be processed with withBase to better reflect the actual implementation.
✅ Verification successful
Mock URLs don't need withBase processing
The actual implementation only applies withBase to URLs that start with "/", but the mock's hardcoded URLs are already absolute ("https://..."). The mock correctly represents a post-processed state where URLs are already resolved, making the current implementation appropriate for testing scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of withBase with these URLs in the actual implementation
rg -l "withBase.*api\.json"
rg -l "withBase.*api-docs\.tsed\.io"
Length of output: 68
Script:
#!/bin/bash
# Let's search for the actual implementation of useThemeConfig to understand how these URLs are used
fd useThemeConfig.ts --exclude "__mocks__" --type f
# Also search for any usage of withBase in the theme package
rg "withBase" packages/theme/
Length of output: 449
Script:
#!/bin/bash
# Let's examine the actual implementation to see how URLs are processed
cat packages/theme/composables/config/useThemeConfig.ts
Length of output: 457
vitest.config.ts (1)
22-25: Coverage thresholds look reasonable.
The auto-updated coverage thresholds show minimal changes, with a slight decrease in branch coverage (79.58% from 80%). While these changes are acceptable, it's worth monitoring the coverage trends to ensure they don't continue to decrease over time.
Let's verify the coverage history:
✅ Verification successful
Based on the git history results, I can now generate the final response:
Coverage thresholds show minor fluctuations within acceptable range
The historical data confirms these are small variations in coverage metrics:
- Statements & Lines: 64.61% → 64.62% (+0.01%)
- Branches: 80% → 79.58% (-0.42%)
- Functions: stable at 67.92%
These minor changes are expected with auto-updating thresholds and don't indicate a concerning downward trend.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check git history for coverage threshold changes
# to identify any declining trends
git log -p vitest.config.ts | grep -A 4 "statements\|branches\|functions\|lines"
Length of output: 509
test/vitepress.client.ts (1)
Line range hint 1-78: Verify the impact on API URL resolution
The useData function contains hardcoded API URLs. With the introduction of withBase, we should verify if any of these URLs need to be resolved relative to the base path.
Let's check for other API URL usages that might need the same fix:
✅ Verification successful
Let me gather more context about how these URLs are being used in the related files.
The hardcoded URLs in useData don't require withBase resolution
The codebase analysis reveals that:
- The
withBasefunction is already being used inuseThemeConfig.tsspecifically for API URLs that start with "/", which is the correct approach - The hardcoded URLs in
useDataare all absolute URLs (starting with "https://"), not relative paths - The
useApiContent.tscorrectly consumes the already processedapiUrlfrom the theme config
Since all URLs in useData are absolute URLs and not relative paths, they don't need to be processed with withBase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API URL patterns in the codebase
rg -l "api\.json|apiUrl" --type ts
# Search for withBase usage patterns
rg "withBase\(" --type ts
Length of output: 856
Script:
#!/bin/bash
# Check how apiUrl is used in useThemeConfig.ts
rg -A 5 "apiUrl" packages/theme/composables/config/useThemeConfig.ts
# Check the implementation of withBase in the test file
rg -A 5 "withBase" test/vitepress.client.ts
# Look for API URL usage in useApiContent
rg -A 5 "apiUrl" packages/theme/composables/api/useApiContent.ts
Length of output: 540
docs/.vitepress/config.mts (2)
34-34: Clarify the purpose of empty apiRedirectUrl.
The apiRedirectUrl has been changed to an empty string. Please clarify:
- Is this intentional?
- How will API redirects be handled now?
- Are there any dependent features that might be affected by this change?
#!/bin/bash
# Search for usage of apiRedirectUrl in the codebase
rg "apiRedirectUrl" -A 333-33: Verify api.json location and withBase usage.
The change to a relative path /api.json aligns with the PR objective to use withBase. However, we should verify:
- That api.json exists in the public directory
- That the theme code properly uses withBase to resolve this path
✅ Verification successful
Let me check the content of the file that uses withBase to verify its implementation.
Configuration changes are properly implemented
The verification confirms:
- The
withBasefunction is correctly used to resolve theapiUrlpath inuseThemeConfig.ts - The
api.jsonfile exists at the correct location indocs/public/api.json - The empty
apiRedirectUrlis intentional as the API documentation is now served locally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if api.json exists in the public directory
fd -t f "api.json" docs/.vitepress/public/
# Check if withBase is used to resolve the apiUrl in theme code
rg -l "withBase.*apiUrl" packages/theme/
Length of output: 3247
Script:
#!/bin/bash
# Check the implementation of withBase with apiUrl
rg -A 5 "withBase.*apiUrl" packages/theme/composables/config/useThemeConfig.ts
# Let's also check if api.json exists in the docs directory
fd -t f "api.json" docs/
Length of output: 206
| if (theme.value?.apiUrl.startsWith("/")) { | ||
| theme.value.apiUrl = withBase(theme.value?.apiUrl); | ||
| } |
There was a problem hiding this comment.
Add null checks and consistent optional chaining.
While the logic is correct, there are potential safety improvements needed:
- The code assumes
theme.value?.apiUrlexists when assigning - There's inconsistent usage of optional chaining
Here's a safer implementation:
-if (theme.value?.apiUrl.startsWith("/")) {
- theme.value.apiUrl = withBase(theme.value?.apiUrl);
+if (theme.value?.apiUrl?.startsWith("/")) {
+ theme.value.apiUrl = withBase(theme.value.apiUrl);
}Or even better, with null safety:
-if (theme.value?.apiUrl.startsWith("/")) {
- theme.value.apiUrl = withBase(theme.value?.apiUrl);
+const apiUrl = theme.value?.apiUrl;
+if (apiUrl?.startsWith("/") && theme.value) {
+ theme.value.apiUrl = withBase(apiUrl);
}📝 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.
| if (theme.value?.apiUrl.startsWith("/")) { | |
| theme.value.apiUrl = withBase(theme.value?.apiUrl); | |
| } | |
| const apiUrl = theme.value?.apiUrl; | |
| if (apiUrl?.startsWith("/") && theme.value) { | |
| theme.value.apiUrl = withBase(apiUrl); | |
| } |
| export function withBase(url: string) { | ||
| return url; | ||
| } |
There was a problem hiding this comment.
Implementation doesn't fulfill the intended purpose
The current implementation of withBase simply returns the URL unchanged, which doesn't align with the PR's objective of properly resolving paths for api.json. This function should handle base path resolution to ensure URLs work correctly when the site is deployed to a subdirectory.
Consider implementing proper base path resolution. For example:
-export function withBase(url: string) {
- return url;
+export function withBase(url: string) {
+ if (!url || url.startsWith('http')) return url
+ const base = import.meta.env.BASE_URL?.replace(/\/$/, '')
+ return `${base}${url.startsWith('/') ? '' : '/'}${url}`
+}📝 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.
| export function withBase(url: string) { | |
| return url; | |
| } | |
| export function withBase(url: string) { | |
| if (!url || url.startsWith('http')) return url | |
| const base = import.meta.env.BASE_URL?.replace(/\/$/, '') | |
| return `${base}${url.startsWith('/') ? '' : '/'}${url}` | |
| } |
|
🎉 This PR is included in version 1.2.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
withBasefor better URL management.Bug Fixes
<Api>component.Chores
Documentation