Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: MK (fengmk2) <fengmk2@gmail.com>

.github/workflows/nodejs.yml

@@ -116,7 +116,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node-version: [20, 22, 24]
+        node-version: [22, 24]
         os: [ubuntu-latest]
         # 0-based index
         shardIndex: [0, 1, 2]
@@ -212,7 +212,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node-version: [20, 22, 24]
+        node-version: [22, 24]
         os: [ubuntu-latest]
         # 0-based index
         shardIndex: [0, 1, 2]
@@ -270,7 +270,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        node-version: [20, 22]
+        node-version: [22, 24]
         os: [ubuntu-latest]
 
     concurrency:

app/core/service/BugVersionService.ts

@@ -2,6 +2,7 @@ import { AccessLevel, Inject, SingletonProto, Logger } from 'egg';
 import pMap from 'p-map';
 import { BugVersion } from '../entity/BugVersion.ts';
 import type {
+  AbbreviatedPackageJSONType,
   PackageJSONType,
   PackageRepository,
 } from '../../repository/PackageRepository.ts';
@@ -69,29 +70,41 @@ export class BugVersionService {
     );
   }
 
+  /**
+   * Fix package bug version with all versions
+   * @param bugVersion - The bug version
+   * @param fullname - The fullname of the package
+   * @param manifests - The manifests of the package
+   * @returns The versions of the fixed manifests
+   */
   async fixPackageBugVersions(
     bugVersion: BugVersion,
     fullname: string,
-    // oxlint-disable-next-line typescript-eslint/no-explicit-any
-    manifests: Record<string, any>
+    manifests: Record<string, PackageJSONType | AbbreviatedPackageJSONType | undefined>
   ) {
+    const fixedVersions: string[] = [];
     // If package all version unpublished(like pinyin-tool), versions is undefined
-    if (!manifests) return;
+    if (!manifests) {
+      return fixedVersions;
+    }
     for (const manifest of Object.values(manifests)) {
-      this.fixPackageBugVersionWithAllVersions(
+      const fixedVersion = this.fixPackageBugVersionWithAllVersions(
         fullname,
         bugVersion,
-        manifest,
-        manifests
+        manifest as PackageJSONType,
+        manifests as Record<string, PackageJSONType>
       );
+      if (fixedVersion) {
+        fixedVersions.push(fixedVersion);
+      }
     }
+    return fixedVersions;
   }
 
   async fixPackageBugVersion(
     bugVersion: BugVersion,
     fullname: string,
-    // oxlint-disable-next-line typescript-eslint/no-explicit-any
-    manifest: any
+    manifest: PackageJSONType
   ) {
     const advice = bugVersion.fixVersion(fullname, manifest.version);
     if (!advice) {
@@ -119,13 +132,19 @@ export class BugVersionService {
     return bugVersion.fixManifest(manifest, fixedManifest);
   }
 
+  /**
+   * Fix package bug version with all versions
+   * @param fullname - The fullname of the package
+   * @param bugVersion - The bug version
+   * @param manifest - The manifest of the package
+   * @param manifests - The manifests of the package
+   * @returns The version of the fixed manifest
+   */
   private fixPackageBugVersionWithAllVersions(
     fullname: string,
     bugVersion: BugVersion,
-    // oxlint-disable-next-line typescript-eslint/no-explicit-any
-    manifest: any,
-    // oxlint-disable-next-line typescript-eslint/no-explicit-any
-    manifests: Record<string, any>
+    manifest: PackageJSONType,
+    manifests: Record<string, PackageJSONType>
   ) {
     const advice = bugVersion.fixVersion(fullname, manifest.version);
     if (!advice) {
@@ -142,5 +161,7 @@ export class BugVersionService {
     }
     const newManifest = bugVersion.fixManifest(manifest, fixedManifest);
     manifests[manifest.version] = newManifest;
+
+    return manifest.version;
   }
 }

app/core/service/PackageManagerService.ts

@@ -1177,7 +1177,10 @@ export class PackageManagerService extends AbstractService {
     let etag = '';
     let blockReason = '';
     const pkg = await this.packageRepository.findPackage(scope, name);
-    if (!pkg) return { etag, data: null, blockReason };
+    if (!pkg) {
+      return { etag, data: null, blockReason };
+    }
+  
     const registry = await this.getSourceRegistry(pkg);
 
     const block = await this.packageVersionBlockRepository.findPackageBlock(
@@ -1199,25 +1202,34 @@ export class PackageManagerService extends AbstractService {
     if (dist?.distId) {
       etag = `"${dist.shasum}"`;
       const data = (await this.distRepository.readDistBytesToJSON(dist)) as T;
+      let needCalculateIntegrity = false;
       if (bugVersion) {
-        await this.bugVersionService.fixPackageBugVersions(
+        const fixedVersions = await this.bugVersionService.fixPackageBugVersions(
           bugVersion,
           fullname,
           data.versions
         );
+        if (fixedVersions.length > 0) {
+          // calculate integrity after fix bug version
+          needCalculateIntegrity = true;
+        }
       }
       // set _source_registry_name in full manifestDist
-      if (registry) {
-        data._source_registry_name = registry?.name;
+      if (registry?.name && data._source_registry_name !== registry.name) {
+        data._source_registry_name = registry.name;
+        // calculate integrity after set _source_registry_name
+        needCalculateIntegrity = true;
       }
 
-      const distBytes = Buffer.from(JSON.stringify(data));
-      const distIntegrity = await calculateIntegrity(distBytes);
-      etag = `"${distIntegrity.shasum}"`;
+      if (needCalculateIntegrity) {
+        const distBytes = Buffer.from(JSON.stringify(data));
+        const distIntegrity = await calculateIntegrity(distBytes);
+        etag = `"${distIntegrity.shasum}"`;
+      }
       return { etag, data, blockReason };
     }
 
-    // read from database
+    // read from database then update to dist, the next time will read from dist
     const fullManifests = isFullManifests
       ? await this._listPackageFullManifests(pkg)
       : null;
@@ -1228,25 +1240,28 @@ export class PackageManagerService extends AbstractService {
       // not exists
       return { etag, data: null, blockReason };
     }
+
+    // update to dist, the next time will read from dist
     await this._updatePackageManifestsToDists(
       pkg,
       fullManifests,
       abbreviatedManifests
     );
     const manifests = (fullManifests || abbreviatedManifests) as T;
-    /* c8 ignore next 5 */
     if (bugVersion) {
-      await this.bugVersionService.fixPackageBugVersions(
+      const fixedVersions = await this.bugVersionService.fixPackageBugVersions(
         bugVersion,
         fullname,
         manifests.versions
       );
-      const distBytes = Buffer.from(JSON.stringify(manifests));
-      const distIntegrity = await calculateIntegrity(distBytes);
-      etag = `"${distIntegrity.shasum}"`;
+      if (fixedVersions.length > 0) {
+        // calculate integrity after fix bug version
+        const distBytes = Buffer.from(JSON.stringify(manifests));
+        const distIntegrity = await calculateIntegrity(distBytes);
+        etag = `"${distIntegrity.shasum}"`;
+      }
     } else {
       dist = isFullManifests ? pkg.manifestsDist : pkg.abbreviatedsDist;
-      // oxlint-disable-next-line typescript-eslint/no-non-null-assertion
       etag = `"${dist!.shasum}"`;
     }
     return { etag, data: manifests, blockReason };

app/core/service/PackageSyncerService.ts

@@ -810,6 +810,7 @@ export class PackageSyncerService extends AbstractService {
       );
       specificVersions.push(distTags.latest);
     }
+    // Get the list of versions to sync this time
     const versions = specificVersions
       ? Object.values<PackageJSONType>(versionMap).filter(verItem =>
           specificVersions.includes(verItem.version)
@@ -858,6 +859,7 @@ export class PackageSyncerService extends AbstractService {
     let syncIndex = 0;
     for (const item of versions) {
       const version: string = item.version;
+      // Skip empty versions, handle abnormal data
       if (!version) continue;
       let existsItem: (typeof existsVersionMap)[string] | undefined =
         existsVersionMap[version];
@@ -945,10 +947,16 @@ export class PackageSyncerService extends AbstractService {
           diffMeta.readme = undefined;
         }
         if (!isEmpty(diffMeta)) {
+          // Differences found, need to sync the changed metadata
           differentMetas.push([existsItem, diffMeta]);
         }
+        
+        // Skip versions that have already been synced
+        // Avoid duplicate syncing
         continue;
       }
+
+      // New version found, start syncing
       syncIndex++;
       const description = item.description;
       // "dist": {

test/core/service/BugVersionService/fixPackageBugVersion.test.ts

@@ -4,7 +4,7 @@ import { app, mock } from '@eggjs/mock/bootstrap';
 import { TestUtil } from '../../../../test/TestUtil.ts';
 import { BugVersionService } from '../../../../app/core/service/BugVersionService.ts';
 import { DistRepository } from '../../../../app/repository/DistRepository.ts';
-import { PackageRepository } from '../../../../app/repository/PackageRepository.ts';
+import { PackageJSONType, PackageRepository } from '../../../../app/repository/PackageRepository.ts';
 import { BugVersion } from '../../../../app/core/entity/BugVersion.ts';
 import { Package } from '../../../../app/core/entity/Package.ts';
 import { PackageVersion } from '../../../../app/core/entity/PackageVersion.ts';
@@ -145,7 +145,7 @@ describe('test/core/service/BugVersionService/fixPackageBugVersion.test.ts', ()
     const newManifest = await bugVersionService.fixPackageBugVersion(
       bugVersion,
       'colors',
-      manifest
+      manifest as unknown as PackageJSONType
     );
     assert.deepStrictEqual(newManifest, {
       name: 'colors',
@@ -197,7 +197,7 @@ describe('test/core/service/BugVersionService/fixPackageBugVersion.test.ts', ()
     const newManifest = await bugVersionService.fixPackageBugVersion(
       bugVersion,
       'colors',
-      manifest
+      manifest as unknown as PackageJSONType
     );
     assert.ok(newManifest === manifest);
   });
@@ -227,7 +227,7 @@ describe('test/core/service/BugVersionService/fixPackageBugVersion.test.ts', ()
     const newManifest = await bugVersionService.fixPackageBugVersion(
       bugVersion,
       'colors',
-      manifest
+      manifest as unknown as PackageJSONType
     );
     assert.ok(newManifest === manifest);
   });

test/core/service/BugVersionService/fixPackageBugVersions.test.ts

@@ -1,9 +1,11 @@
 import assert from 'node:assert/strict';
+
 import { app, mock } from '@eggjs/mock/bootstrap';
 
 import { TestUtil } from '../../../../test/TestUtil.ts';
 import { BugVersionService } from '../../../../app/core/service/BugVersionService.ts';
 import { BugVersion } from '../../../../app/core/entity/BugVersion.ts';
+import { AbbreviatedPackageJSONType } from '../../../../app/repository/PackageRepository.ts';
 
 describe('test/core/service/BugVersionService/fixPackageBugVersions.test.ts', () => {
   let bugVersionService: BugVersionService;
@@ -89,7 +91,7 @@ describe('test/core/service/BugVersionService/fixPackageBugVersions.test.ts', ()
     await bugVersionService.fixPackageBugVersions(
       bugVersion,
       'colors',
-      manifests
+      manifests as unknown as Record<string, AbbreviatedPackageJSONType>
     );
     assert.deepStrictEqual(manifests, {
       '1.4.0': {