Conversation
….json closes #116 - Remove useFileContent hook, use manifest.versions directly - Add version query parameter to dependency links - Remove unnecessary loading state 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRefactors the Deps component to retrieve dependencies directly from the manifest prop instead of remote file content, simplifying loading state handling and enabling version-aware dependency links with a unified data transformation approach. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fengmk2
changed the title
Summary of ChangesHello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the dependency display page by changing how dependency data is sourced. Instead of making an additional call to fetch Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring. It simplifies the dependencies page by removing an unnecessary data fetch and loading state, instead using the manifest data that's already available. This improves performance and makes the code easier to understand. The addition of the version spec to the dependency links is also a nice enhancement. I have one suggestion to improve the robustness and reduce code duplication in the useMemo hook.
| const depsInfo = React.useMemo(() => { | ||
| if (isLoading) return undefined; | ||
| const targetVersion = JSON.parse(versionManifest || '{}'); | ||
| const deps = ['dependencies', 'devDependencies'] as const; | ||
| const res: Record<string, { package: string; spec: string }[]> = { | ||
| dependencies: [], | ||
| devDependencies: [], | ||
| }; | ||
| deps.forEach((k) => { | ||
| if (targetVersion?.[k]) { | ||
| res[k] = Object.keys(targetVersion[k] || {}).map((pkg) => ({ | ||
| package: pkg, | ||
| spec: targetVersion[k][pkg], | ||
| })); | ||
| } | ||
| }); | ||
| return res; | ||
| }, [versionManifest, isLoading]); | ||
| const versionData = manifest.versions?.[version!]; | ||
| if (!versionData) return { dependencies: [], devDependencies: [] }; | ||
|
|
||
| const deps = versionData.dependencies || {}; | ||
| const devDeps = versionData.devDependencies || {}; | ||
|
|
||
| const loading = depsInfo === undefined; | ||
| return { | ||
| dependencies: Object.entries(deps).map(([pkg, spec]) => ({ | ||
| package: pkg, | ||
| spec, | ||
| })), | ||
| devDependencies: Object.entries(devDeps).map(([pkg, spec]) => ({ | ||
| package: pkg, | ||
| spec, | ||
| })), | ||
| }; | ||
| }, [manifest, version]); |
There was a problem hiding this comment.
This useMemo block can be made more robust and concise.
- Avoid Non-Null Assertion: Using the non-null assertion operator (
!) onversioncan be risky if theversionprop is not guaranteed to be defined. It's safer to add a guard clause to handle cases whereversionmight benullorundefined. - Reduce Duplication: The logic to transform the
dependenciesanddevDependenciesobjects into arrays is identical. This can be extracted into a small helper function within the hook to adhere to the DRY (Don't Repeat Yourself) principle.
Here's a suggested refactoring that addresses both points.
| const depsInfo = React.useMemo(() => { | |
| if (isLoading) return undefined; | |
| const targetVersion = JSON.parse(versionManifest || '{}'); | |
| const deps = ['dependencies', 'devDependencies'] as const; | |
| const res: Record<string, { package: string; spec: string }[]> = { | |
| dependencies: [], | |
| devDependencies: [], | |
| }; | |
| deps.forEach((k) => { | |
| if (targetVersion?.[k]) { | |
| res[k] = Object.keys(targetVersion[k] || {}).map((pkg) => ({ | |
| package: pkg, | |
| spec: targetVersion[k][pkg], | |
| })); | |
| } | |
| }); | |
| return res; | |
| }, [versionManifest, isLoading]); | |
| const versionData = manifest.versions?.[version!]; | |
| if (!versionData) return { dependencies: [], devDependencies: [] }; | |
| const deps = versionData.dependencies || {}; | |
| const devDeps = versionData.devDependencies || {}; | |
| const loading = depsInfo === undefined; | |
| return { | |
| dependencies: Object.entries(deps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| devDependencies: Object.entries(devDeps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| }; | |
| }, [manifest, version]); | |
| const depsInfo = React.useMemo(() => { | |
| if (!version) { | |
| return { dependencies: [], devDependencies: [] }; | |
| } | |
| const versionData = manifest.versions?.[version]; | |
| if (!versionData) { | |
| return { dependencies: [], devDependencies: [] }; | |
| } | |
| const transformDeps = (deps: Record<string, string> = {}) => | |
| Object.entries(deps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })); | |
| return { | |
| dependencies: transformDeps(versionData.dependencies), | |
| devDependencies: transformDeps(versionData.devDependencies), | |
| }; | |
| }, [manifest, version]); |
There was a problem hiding this comment.
Pull request overview
This PR refactors the dependencies page to use manifest data directly instead of fetching package.json separately, improving performance by eliminating an unnecessary HTTP request.
- Removes the
useFileContenthook and accesses dependency data directly frommanifest.versions - Adds TypeScript interface
DepRecordfor better type safety - Adds version query parameters to dependency links for better navigation
- Removes loading state that is no longer needed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/slugs/deps/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/slugs/deps/index.tsx (2)
src/pages/package/[...slug]/index.tsx (1)
PageProps(22-26)src/components/SizeContainer.tsx (1)
SizeContainer(11-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Agent
🔇 Additional comments (3)
src/slugs/deps/index.tsx (3)
9-12: LGTM! Clean type definition.The
DepRecordinterface provides clear typing for the transformed dependency data structure.
58-58: LGTM! Clean static count display.The static dependency counts improve UX by eliminating loading states, which aligns well with the refactor to use manifest data directly instead of remote fetching.
Also applies to: 68-68
18-24: Version ranges are intentionally supported as query parameters.The code is designed to pass version ranges (like "^1.0.0") from package.json directly to the registry API, which resolves them to specific versions. The package page automatically redirects to the resolved specific version, normalizing the URL. This is a supported pattern used consistently throughout the codebase, not a bug.
| export default function Deps({ manifest, version }: PageProps) { | ||
| const depsInfo = React.useMemo(() => { | ||
| if (isLoading) return undefined; | ||
| const targetVersion = JSON.parse(versionManifest || '{}'); | ||
| const deps = ['dependencies', 'devDependencies'] as const; | ||
| const res: Record<string, { package: string; spec: string }[]> = { | ||
| dependencies: [], | ||
| devDependencies: [], | ||
| }; | ||
| deps.forEach((k) => { | ||
| if (targetVersion?.[k]) { | ||
| res[k] = Object.keys(targetVersion[k] || {}).map((pkg) => ({ | ||
| package: pkg, | ||
| spec: targetVersion[k][pkg], | ||
| })); | ||
| } | ||
| }); | ||
| return res; | ||
| }, [versionManifest, isLoading]); | ||
| const versionData = manifest.versions?.[version!]; | ||
| if (!versionData) return { dependencies: [], devDependencies: [] }; | ||
|
|
||
| const deps = versionData.dependencies || {}; | ||
| const devDeps = versionData.devDependencies || {}; | ||
|
|
||
| const loading = depsInfo === undefined; | ||
| return { | ||
| dependencies: Object.entries(deps).map(([pkg, spec]) => ({ | ||
| package: pkg, | ||
| spec, | ||
| })), | ||
| devDependencies: Object.entries(devDeps).map(([pkg, spec]) => ({ | ||
| package: pkg, | ||
| spec, | ||
| })), | ||
| }; | ||
| }, [manifest, version]); |
There was a problem hiding this comment.
Remove unsafe non-null assertion on optional parameter.
Line 34 uses a non-null assertion (version!) on an optional parameter. While the fallback on line 35 handles missing versionData, the non-null assertion is unsafe and could mask issues.
Apply this diff to handle the optional version safely:
const depsInfo = React.useMemo(() => {
- const versionData = manifest.versions?.[version!];
+ if (!version) return { dependencies: [], devDependencies: [] };
+ const versionData = manifest.versions?.[version];
if (!versionData) return { dependencies: [], devDependencies: [] };📝 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 default function Deps({ manifest, version }: PageProps) { | |
| const depsInfo = React.useMemo(() => { | |
| if (isLoading) return undefined; | |
| const targetVersion = JSON.parse(versionManifest || '{}'); | |
| const deps = ['dependencies', 'devDependencies'] as const; | |
| const res: Record<string, { package: string; spec: string }[]> = { | |
| dependencies: [], | |
| devDependencies: [], | |
| }; | |
| deps.forEach((k) => { | |
| if (targetVersion?.[k]) { | |
| res[k] = Object.keys(targetVersion[k] || {}).map((pkg) => ({ | |
| package: pkg, | |
| spec: targetVersion[k][pkg], | |
| })); | |
| } | |
| }); | |
| return res; | |
| }, [versionManifest, isLoading]); | |
| const versionData = manifest.versions?.[version!]; | |
| if (!versionData) return { dependencies: [], devDependencies: [] }; | |
| const deps = versionData.dependencies || {}; | |
| const devDeps = versionData.devDependencies || {}; | |
| const loading = depsInfo === undefined; | |
| return { | |
| dependencies: Object.entries(deps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| devDependencies: Object.entries(devDeps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| }; | |
| }, [manifest, version]); | |
| export default function Deps({ manifest, version }: PageProps) { | |
| const depsInfo = React.useMemo(() => { | |
| if (!version) return { dependencies: [], devDependencies: [] }; | |
| const versionData = manifest.versions?.[version]; | |
| if (!versionData) return { dependencies: [], devDependencies: [] }; | |
| const deps = versionData.dependencies || {}; | |
| const devDeps = versionData.devDependencies || {}; | |
| return { | |
| dependencies: Object.entries(deps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| devDependencies: Object.entries(devDeps).map(([pkg, spec]) => ({ | |
| package: pkg, | |
| spec, | |
| })), | |
| }; | |
| }, [manifest, version]); |
🤖 Prompt for AI Agents
In src/slugs/deps/index.tsx around lines 32 to 50, remove the unsafe non-null
assertion on the optional parameter by replacing manifest.versions?.[version!]
with a safe access manifest.versions?.[version] (no '!') and handle the
undefined case already covered by the existing early return; ensure TypeScript
types permit indexing with string | undefined (or narrow version into a const
versionKey = version ?? undefined before indexing) so no non-null assertions are
used.
closes #116
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.