fix: check the file tree before the timeline reports removed types#3000
fix: check the file tree before the timeline reports removed types#3000thribhuvan003 wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @thribhuvan003! 🚀 Here’s what will happen next:
|
|
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:
📝 WalkthroughWalkthroughThe timeline endpoint now re-checks missing ChangesTimeline hasTypes detection fix
Generated lexicon modules
Sequence Diagram(s)sequenceDiagram
participant TimelineEndpoint
participant PackageLookup
participant PackageAnalysis
TimelineEndpoint->>TimelineEndpoint: Sort versions and scan backwards
loop up to MAX_FILE_TREE_CHECKS
TimelineEndpoint->>PackageLookup: fetchPackageWithTypesAndFiles(package, version)
alt lookup succeeds
PackageLookup-->>TimelineEndpoint: types/files
TimelineEndpoint->>PackageAnalysis: analyzePackage(typesPackage, files)
PackageAnalysis-->>TimelineEndpoint: types included or not
TimelineEndpoint->>TimelineEndpoint: Update current.hasTypes
else lookup fails
PackageLookup-->>TimelineEndpoint: error
TimelineEndpoint->>TimelineEndpoint: Keep existing hasTypes
end
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api/registry/timeline/[...pkg].get.ts (1)
105-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the file-tree lookups
getPackageFileTreeaccepts a signal, but this handler never supplies one. AddAbortSignal.timeout(...)here — or wireevent.node.reqclose into anAbortControllerif you need disconnect cancellation — so a slow jsDelivr response cannot hold the request open across allMAX_FILE_TREE_CHECKS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/registry/timeline/`[...pkg].get.ts around lines 105 - 114, The file-tree lookup in the timeline handler is not bounded, so a slow getPackageFileTree call can keep the request open too long. Update the logic in the [...pkg].get handler to pass an AbortSignal to getPackageFileTree, preferably using AbortSignal.timeout(...), or wire event.node.req close into an AbortController if you want disconnect cancellation. Keep the existing try/catch behavior and make sure the signal is applied consistently for the MAX_FILE_TREE_CHECKS loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/api/registry/timeline/`[...pkg].get.ts:
- Around line 105-114: The file-tree lookup in the timeline handler is not
bounded, so a slow getPackageFileTree call can keep the request open too long.
Update the logic in the [...pkg].get handler to pass an AbortSignal to
getPackageFileTree, preferably using AbortSignal.timeout(...), or wire
event.node.req close into an AbortController if you want disconnect
cancellation. Keep the existing try/catch behavior and make sure the signal is
applied consistently for the MAX_FILE_TREE_CHECKS loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 365f9fa2-20ff-4bab-9df5-6313dc69d7b2
📒 Files selected for processing (2)
server/api/registry/timeline/[...pkg].get.tstest/unit/server/api/registry/timeline/pkg.get.spec.ts
ghostdevv
left a comment
There was a problem hiding this comment.
There seems to be a util we can re-use analyzePackage, for example:
npmx.dev/server/api/registry/analysis/[...pkg].get.ts
Lines 36 to 42 in 32e324a
(we don't need to pass createPackage here)
|
ah nice, missed that util — switched to fetchPackageWithTypesAndFiles + analyzePackage, much cleaner. thanks! |
|
oops, my local generate:lexicons run leaked into a commit — cleaned it up, diff is just the two files now |
The timeline computed hasTypes from packument fields only, so packages that ship declaration files next to their entry points (without a types field) showed a false "TypeScript types removed" event.
Now when a version would produce that event, the endpoint fetches its file tree and re-checks it with the same detectTypesStatus the package page uses, capped at 5 lookups per request so a timeline page never fans out. Added unit tests for the implicit-declarations case, a genuine removal, and a failed file tree lookup.
fixes #2791