fix: should refresh package manifests to dists to avoid missing versions #946
Conversation
WalkthroughTwo occurrences of the publish command payload in PackageSyncerService.ts were modified to enable manifest refresh by setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
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 |
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 implements a critical fix to ensure the complete and accurate synchronization of package manifests. By enabling the refresh of package manifests, it resolves an issue where certain package versions might be missing from the distribution. The changes are validated with a new test case that simulates various synchronization scenarios, including the handling of large package versions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 addresses issue #943 by ensuring package manifests are refreshed incrementally during synchronization. This prevents missing versions in case of a partial sync failure. The change in PackageSyncerService.ts correctly sets skipRefreshPackageManifests to false to enable this behavior.
A comprehensive test case has been added to validate the fix, covering the scenario of a sync being blocked and later allowed. While the test is thorough, it contains significant code duplication. I've added a suggestion to refactor the test for improved readability and maintainability.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #946 +/- ##
==========================================
- Coverage 95.38% 95.26% -0.12%
==========================================
Files 197 197
Lines 22261 22262 +1
Branches 2457 2071 -386
==========================================
- Hits 21233 21209 -24
- Misses 1028 1053 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #943 where package versions could go missing from manifests during synchronization. The root cause was that package manifests weren't being refreshed after each version was published in the Packument-based sync flow, which could result in versions not appearing in the final manifest when some versions failed or were blocked during sync.
Key changes:
- Modified the Packument-based sync flow to refresh manifests immediately after each version is published, rather than deferring the refresh
- Added comprehensive test case that verifies all versions appear in manifests even when some versions are initially blocked
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/core/service/PackageSyncerService.ts | Changed skipRefreshPackageManifests from true to false in the syncPackageWithPackument method to ensure manifests are refreshed after each version publish, preventing missing versions |
| test/core/service/PackageSyncerService/executeTaskWithPackument.test.ts | Added test case issue-943: should block first then allow that validates all versions appear in manifests across multiple sync attempts, including scenarios where versions are initially blocked then later allowed |
| isPrivate: false, | ||
| publishTime, | ||
| skipRefreshPackageManifests: true, | ||
| // should refresh package manifests to dists to avoid missing versions |
There was a problem hiding this comment.
Changing skipRefreshPackageManifests to false will cause package manifests to be refreshed after each individual version publish, rather than batching all refreshes at the end. This is a performance trade-off: each of N versions will trigger a manifest refresh operation, which reads and writes manifest files from/to storage.
While this fixes the missing versions issue, for packages with many new versions to sync, this could significantly increase I/O operations and sync time. Consider whether there's an alternative approach that could batch the refreshes while still ensuring all versions are present in the manifests, or document this performance trade-off in the comment.
| // should refresh package manifests to dists to avoid missing versions | |
| // NOTE: | |
| // We intentionally disable manifest refresh batching here by setting | |
| // skipRefreshPackageManifests to false so that each synced version is | |
| // immediately reflected in the stored manifests. | |
| // | |
| // This avoids "missing version" issues when: | |
| // - some publishes fail midway through the loop, or | |
| // - a version already exists locally and publish() short‑circuits, | |
| // but we still need that version to appear in the manifests. | |
| // | |
| // Trade‑off: this causes one manifest refresh per version, which can | |
| // increase I/O and total sync time for packages with many new versions. | |
| // If this ever becomes a bottleneck, consider implementing an explicit | |
| // batching mechanism that refreshes manifests once after the loop while | |
| // still ensuring all versions in updateVersions are included. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/core/service/PackageSyncerService/executeTaskWithPackument.test.ts (1)
2267-2445: Comprehensive regression test for issue #943 looks solidThis test clearly drives the block-then-allow flow for large package versions, asserting the expected failure log first, then successful sync and final manifests containing all expected versions. It aligns well with the manifest-refresh fix and exercises both the error and success paths in a single scenario, which is desirable for this kind of regression. Based on learnings, this covers both failure and recovery behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/core/service/PackageSyncerService.tstest/core/service/PackageSyncerService/executeTaskWithPackument.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,tsx,jsx}: Use single quotes in JavaScript/TypeScript code (from Prettier configuration)
Use 2-space indentation (from Prettier configuration)
Maintain 120 character line width maximum (from Prettier configuration)
Use trailing commas in ES5 syntax (from Prettier configuration)
Avoid parentheses in arrow functions when possible (from Prettier configuration)
Maximum of 6 function parameters (from Oxlint configuration)
Warn on console usage (from Oxlint configuration)
Disallow anonymous default exports (from Oxlint configuration)
Use ES modules (import/export) syntax throughout
Files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.tsapp/core/service/PackageSyncerService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use strict TypeScript with comprehensive type definitions - avoidanytypes, use proper typing orunknown
Export types and interfaces for reusability in TypeScript
Files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.tsapp/core/service/PackageSyncerService.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
test/**/*.test.ts: Test files must use.test.tssuffix
Tests must use@eggjs/mockfor mocking and testing
Tests must useassertfromnode:assert/strictfor assertions
Test files should be organized intest/directory mirroring source structure
Mock external dependencies usingmock()from@eggjs/mockin tests
Use realistic test data created throughTestUtilhelper methods
Clean up after tests - database is reset between test files
Test both success and failure cases - error paths are equally important
Test files should follow naming pattern: describe('[HTTP_METHOD /api/path] functionName()', ...)
test/**/*.test.ts: Test files must use.test.tssuffix
Use@eggjs/mockfor mocking in test files
Useassertfromnode:assert/strictin test files
Test files must mirror source structure intest/directory and test both success and error cases
Files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript with proper typing - avoidanytypes, use proper typing orunknowninstead
Use ES modules withimport/exportsyntax throughout the codebase
Use single quotes (') for strings
Use 2-space indentation
Enforce 120 character line width
Use ES5 trailing commas
Limit functions to a maximum of 6 parameters
Do not use console statements - use logger instead
Files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.tsapp/core/service/PackageSyncerService.ts
app/core/service/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/core/service/**/*.ts: Services must implement core business logic without HTTP concerns or direct database access
Services must use@SingletonProto()for service lifecycle management
Services must orchestrate multiple repositories and entities, managing transactions and events
Files:
app/core/service/PackageSyncerService.ts
🧠 Learnings (2)
📚 Learning: 2025-11-29T15:42:37.586Z
Learnt from: CR
Repo: cnpm/cnpmcore PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T15:42:37.586Z
Learning: Applies to test/**/*.test.ts : Test both success and failure cases - error paths are equally important
Applied to files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.ts
📚 Learning: 2025-11-29T15:42:56.815Z
Learnt from: CR
Repo: cnpm/cnpmcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T15:42:56.815Z
Learning: Applies to test/**/*.test.ts : Test files must mirror source structure in `test/` directory and test both success and error cases
Applied to files:
test/core/service/PackageSyncerService/executeTaskWithPackument.test.ts
⏰ 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). (19)
- GitHub Check: test on mysql (node@24, shard@1/3, enableJSONBuilder@true)
- GitHub Check: test on mysql (node@22, shard@0/3, enableJSONBuilder@true)
- GitHub Check: test on postgresql (node@24, shard@2/3)
- GitHub Check: test on mysql (node@20, shard@0/3, enableJSONBuilder@false)
- GitHub Check: test on mysql (node@20, shard@1/3, enableJSONBuilder@true)
- GitHub Check: test on mysql (node@20, shard@1/3, enableJSONBuilder@false)
- GitHub Check: test on mysql (node@20, shard@2/3, enableJSONBuilder@true)
- GitHub Check: test on mysql (node@24, shard@2/3, enableJSONBuilder@false)
- GitHub Check: test on mysql (node@22, shard@0/3, enableJSONBuilder@false)
- GitHub Check: test on mysql (node@24, shard@0/3, enableJSONBuilder@true)
- GitHub Check: test on mysql (node@22, shard@2/3, enableJSONBuilder@true)
- GitHub Check: test on postgresql (node@22, shard@0/3)
- GitHub Check: test on postgresql (node@20, shard@2/3)
- GitHub Check: test on postgresql (node@22, shard@1/3)
- GitHub Check: test on postgresql (node@20, shard@1/3)
- GitHub Check: test on postgresql (node@24, shard@0/3)
- GitHub Check: test on postgresql (node@20, shard@0/3)
- GitHub Check: test on postgresql (node@22, shard@2/3)
- GitHub Check: build-and-push-image
🔇 Additional comments (1)
app/core/service/PackageSyncerService.ts (1)
1451-1466: Enabling manifest refresh in packument-based publish matches the PR’s intentSetting
skipRefreshPackageManifests: falsehere (with the clarifying comment) is consistent with fixing missing-version issues when syncing via packument. The extra refresh work on top of the laterrefreshPackageChangeVersionsToDistscall should be safe given these operations are designed to be idempotent.
[skip ci] ## <small>4.18.1 (2025-12-22)</small> * chore: replace @node-rs/crc32 with Node.js stdlib zlib.crc32 (#938) ([8a1b79f](8a1b79f)), closes [#938](#938) [#937](#937) * chore(deps): update dependency oxfmt to ^0.20.0 (#944) ([e6f9208](e6f9208)), closes [#944](#944) * chore(deps): update dependency oxlint-tsgolint to ^0.10.0 (#939) ([6f07c76](6f07c76)), closes [#939](#939) * chore(deps): update oxfmt and remove prettier (#923) ([9ab9d2a](9ab9d2a)), closes [#923](#923) * fix: should refresh package manifests to dists to avoid missing versions (#946) ([7d345b8](7d345b8)), closes [#946](#946) [hi#level](https://github.com/hi/issues/level)
|
🎉 This PR is included in version 4.18.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
close #943
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.