Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ix/issue-2044 * 'fix/issue-2044' of github.com:eryue0220/npmx.dev: fix: align weekly chart buckets from end to match npm downloads (npmx-dev#2052)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds internal alias import resolution across the codebase. Introduces new types and exports in server/utils/import-resolver.ts (InternalImportTarget, InternalImportsMap, resolveAliasToDir, resolveInternalImport, helpers) and extends createImportResolver to accept an optional internal imports map. Updates server/api/registry/file/[...pkg].get.ts to include Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![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
🧹 Nitpick comments (1)
test/unit/server/utils/import-resolver.spec.ts (1)
194-299: Great test expansion; add one#/canonicalisation regression case.Please add a case where the specifier is
#/app/...but the imports key is#app, to lock the slash-variant behaviour.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42e2b622-9ae9-4d7c-b79d-c8d2478854a5
📒 Files selected for processing (4)
server/api/registry/file/[...pkg].get.tsserver/utils/code-highlight.tsserver/utils/import-resolver.tstest/unit/server/utils/import-resolver.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/utils/import-resolver.ts (1)
264-269: Minor performance optimisation: move normalisation outside the loop.
normalizeAliasPrefix(specifier)is computed on every iteration but the result never changes. Consider hoisting it before the loop.♻️ Suggested refactor
function guessInternalImportTarget( imports: InternalImportsMap, specifier: string, files: FileSet, currentFile: string, ): string | null { + const normalizedSpecifier = normalizeAliasPrefix(specifier) for (const [key, value] of Object.entries(imports)) { - const normalizedSpecifier = normalizeAliasPrefix(specifier) const normalizedKey = normalizeAliasPrefix(key)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bda51135-cf70-4e3f-90aa-3fd46fb1d0a8
📒 Files selected for processing (2)
server/api/registry/file/[...pkg].get.tsserver/utils/import-resolver.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/utils/import-resolver.ts (1)
258-316: RefactorguessInternalImportTargetinto smaller helpers.This method currently handles matching, suffix derivation, probing, and return shaping in one block, which makes it harder to maintain and test safely.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c7399fb-e4e1-44c9-8cf8-a21c19a00fba
📒 Files selected for processing (1)
server/utils/import-resolver.ts
| // Support #app, #/app, ~app, ~/app, @app, @/app | ||
| const alias = aliasSpec.replace(/^[#~@]\/?/, '') | ||
| const segments = filePath.split('/') | ||
|
|
||
| let lastMatchIndex = -1 | ||
| for (let i = 0; i < segments.length; i++) { | ||
| if (segments[i] === alias) { | ||
| lastMatchIndex = i | ||
| } | ||
| } | ||
|
|
||
| if (lastMatchIndex === -1) { | ||
| return null | ||
| } | ||
|
|
||
| return segments.slice(0, lastMatchIndex + 1).join('/') | ||
| } |
There was a problem hiding this comment.
Handle empty alias roots (@, ~, #) explicitly.
After normalisation, aliases like @/~/ become empty, and this segment-based lookup can return null for valid targets such as ./src (no empty segment), so aliased links still fail in common setups.
Proposed fix
export function resolveAliasToDir(aliasSpec: string, filePath?: string | null): string | null {
if (
(!aliasSpec.startsWith('#') && !aliasSpec.startsWith('~') && !aliasSpec.startsWith('@')) ||
!filePath
) {
return null
}
// Support `#app`, `#/app`, ~app, ~/app, `@app`, `@/app`
const alias = aliasSpec.replace(/^[#~@]\/?/, '')
+ const normalizedFilePath = filePath.replace(/\/+$/, '')
+ if (!normalizedFilePath) {
+ return null
+ }
+ if (alias === '') {
+ return normalizedFilePath
+ }
- const segments = filePath.split('/')
+ const segments = normalizedFilePath.split('/')| const importTarget = normalizeInternalImportTarget(imports[cleanSpecifier]) | ||
| const target = | ||
| importTarget != null | ||
| ? importTarget | ||
| : guessInternalImportTarget(imports, cleanSpecifier, files, currentFile) | ||
|
|
||
| if (!target || !target.startsWith('./')) { | ||
| return null | ||
| } | ||
|
|
||
| const path = normalizePath(target) | ||
| if (!path || path.startsWith('..') || !files.has(path)) { | ||
| return null |
There was a problem hiding this comment.
Add extension/index fallback for exact internal targets.
Exact imports[specifier] targets are currently accepted only on direct files.has(path). Extensionless or directory targets (for example ./dist/app) will incorrectly resolve to null even when resolvable files exist.
Proposed fix
const path = normalizePath(target)
- if (!path || path.startsWith('..') || !files.has(path)) {
+ if (!path || path.startsWith('..')) {
return null
}
- return { path }
+ if (files.has(path)) {
+ return { path }
+ }
+
+ for (const extensions of getExtensionPriority(currentFile)) {
+ for (const ext of extensions) {
+ const candidate = `${path}${ext}`
+ if (files.has(candidate)) {
+ return { path: candidate }
+ }
+ }
+ }
+
+ for (const indexFile of getIndexExtensions(currentFile)) {
+ const candidate = `${path}/${indexFile}`
+ if (files.has(candidate)) {
+ return { path: candidate }
+ }
+ }
+
+ return null
}2 participants
🔗 Linked issue
resolve #2044
🧭 Context
support alias path like
~/app/components/Button.vue,@/app/components/Button.vueor#/app/components/Button.vue📚 Description