Conversation
📝 WalkthroughWalkthroughReplaces the custom memoize utility with ocache's defineCachedFunction across the codebase, removes src/utils/memoize.ts and its tests, and updates caching usage sites (package, replacement, vulnerability APIs and other helpers) to use ocache with CACHE_MAX_AGE_ONE_DAY. Renames CACHE_TTL_ONE_DAY → CACHE_MAX_AGE_ONE_DAY and changes units from milliseconds to seconds. Moves API modules from src/utils/api/* to src/api/* and updates imports. Refactors workspace caching to per-instance invalidation (invalidateDependencyInfo / invalidatedFolder) instead of direct cache deletion. Possibly related PRs
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/api/vulnerability.ts (1)
92-104:⚠️ Potential issue | 🟠 MajorMap 404 responses to no-result instead of throwing.
The
ofetchlibrary throws on non-2xx status codes by default. A 404 response from the vulnerabilities endpoint will currently abort with an error instead of allowing a no-result path, which is why the test "should not flag when API returns 404" is failing. Handle 404 responses explicitly before the error propagates, returningnullor a suitable no-result value to letcheckVulnerabilityhandle the absence of data gracefully. Rethrow only unexpected error responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c9e422a-fb8e-4ff5-b18e-86d58ab05600
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
package.jsonpnpm-workspace.yamlsrc/composables/workspace-context.tssrc/constants.tssrc/core/workspace.tssrc/providers/diagnostics/rules/vulnerability.tssrc/utils/api/package.tssrc/utils/api/replacement.tssrc/utils/api/vulnerability.tssrc/utils/memoize.tstests/utils/memoize.test.tstsdown.config.ts
💤 Files with no reviewable changes (2)
- tests/utils/memoize.test.ts
- src/utils/memoize.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/workspace.ts (1)
39-54:⚠️ Potential issue | 🟠 MajorInvalidate manifest caches when workspace catalogue data changes.
loadWorkspace()refreshes#catalogs, but the new invalidation path only marks the exact URI passed toinvalidateDependencyInfo(). Whensrc/composables/workspace-context.ts:17-30invalidates a workspace file and then reloads the workspace, any cachedloadPackageManifestInfo(uri)entries still keep the old catalogue-based resolutions until each manifest is invalidated separately.Possible fix
class WorkspaceContext { + `#workspaceGeneration` = 0 folder: WorkspaceFolder packageManager: PackageManager = 'npm' `#catalogs`?: PromiseWithResolvers<CatalogsInfo | undefined> `#invalidatedPaths` = new Set<string>() async loadWorkspace() { + this.#workspaceGeneration++ this.#catalogs = undefined this.packageManager = await detectPackageManager(this.folder) // ... } `#cacheOptions`: CacheOptions<any, [Uri]> = { - getKey: (uri) => uri.path, + getKey: (uri) => `${this.#workspaceGeneration}:${uri.path}`, maxAge: 0, } }Also applies to: 57-60, 62-65, 120-123, 148-150
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceab782a-0e95-4b18-b7e0-84cc8479583c
📒 Files selected for processing (13)
src/api/package.tssrc/api/replacement.tssrc/api/vulnerability.tssrc/core/workspace.tssrc/providers/diagnostics/rules/replacement.tssrc/providers/diagnostics/rules/upgrade.tssrc/providers/diagnostics/rules/vulnerability.tssrc/types/context.tssrc/utils/package.tstests/diagnostics/context.tstests/diagnostics/upgrade.test.tstests/utils/package.test.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tests/utils/package.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/vulnerability.ts (1)
96-98:⚠️ Potential issue | 🟠 MajorDo not cache non-2xx responses as vulnerability data.
ignoreResponseError: trueremoves the status gate here, and?? nullonly handlesnull/undefined. A 404/5xx payload can therefore be cached as if it were a validVulnerabilityTreeResult, which keeps bad data around for the full TTL. Only convert an expected 404 tonull; let the other failures bypass the cache.Proposed fix
-import { ofetch } from 'ofetch' +import { FetchError, ofetch } from 'ofetch' ... - const result = await ofetch(`${NPMX_DEV_API}/registry/vulnerabilities/${encodedName}/v/${version}`, { - ignoreResponseError: true, - }) ?? null - logger.info(`[vulnerability] fetched for ${name}`) - - return result + try { + const result = await ofetch<VulnerabilityTreeResult>(`${NPMX_DEV_API}/registry/vulnerabilities/${encodedName}/v/${version}`) + logger.info(`[vulnerability] fetched for ${name}`) + return result + } + catch (error) { + if (error instanceof FetchError && error.statusCode === 404) + return null + throw error + }In unjs/ofetch, what does `ignoreResponseError: true` do for HTTP 404/500 responses returned by `ofetch`? Does it resolve with the parsed response body rather than throwing?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32dbdc64-485a-4e27-aeae-52fa5c6fb988
📒 Files selected for processing (3)
src/api/replacement.tssrc/api/vulnerability.tssrc/core/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/replacement.ts
| #cacheOptions: CacheOptions<any, [Uri]> = { | ||
| getKey: (uri) => uri.path, | ||
| ttl: false, | ||
| maxSize: Number.POSITIVE_INFINITY, | ||
| fallbackToCachedOnError: false, | ||
| maxAge: 0, | ||
| swr: false, | ||
| staleMaxAge: 0, | ||
| shouldInvalidateCache: (uri) => this.#invalidatedPaths.delete(uri.path), | ||
| } | ||
|
|
||
| invalidateDependencyInfo(uri: Uri) { | ||
| const path = uri.path | ||
| this.#invalidatedPaths.add(path) | ||
| } |
There was a problem hiding this comment.
Invalidate manifest caches when the workspace catalogue changes.
loadPackageManifestInfo() bakes this.#catalogs!.promise into each cached manifest result, but invalidateDependencyInfo() only expires the exact URI that changed. If the workspace file changes, the catalogue is reloaded while existing manifest entries still serve dependency resolutions computed from the old catalogue.
Possible fix
class WorkspaceContext {
folder: WorkspaceFolder
packageManager: PackageManager = 'npm'
`#catalogs`?: PromiseWithResolvers<CatalogsInfo | undefined>
`#invalidatedPaths` = new Set<string>()
+ `#manifestCacheVersion` = 0
...
invalidateDependencyInfo(uri: Uri) {
const path = uri.path
this.#invalidatedPaths.add(path)
+ if (isWorkspaceFilePath(path))
+ this.#manifestCacheVersion++
}
...
loadPackageManifestInfo = defineCachedFunction<
WithResolvedDependencyInfo<PackageManifestInfo> | undefined,
[Uri]
>(async (uri) => {
const path = uri.path
if (!isPackageManifestPath(path))
return
...
return {
...info,
dependencies: info.dependencies.map((dep) => this.#createResolvedDependencyInfo(dep, catalogs)),
}
- }, this.#cacheOptions)
+ }, {
+ ...this.#cacheOptions,
+ getKey: (uri) => `${this.#manifestCacheVersion}:${uri.path}`,
+ })Also applies to: 97-123
1 participant
move away from manual memoize function, migrate to ocache