fix(cdn): exclude packages from CDN resolver including transitive deps#36
Conversation
The CDN resolver lacked exclude support entirely. When using generate() with the exclude option, excluded packages still appeared in the output import map if they were transitive dependencies of non-excluded packages. Add WithExclude builder to CDN resolver, filtering excluded packages from both direct dependencies in ResolvePackageJSON and transitive dependencies in resolvePackage. Wire exclude through WASM generate() and update TypeScript GenerateOptions type. Also fix a pre-existing race condition in PackageCache.GetOrLoad where the fast path could read entry.pkg/entry.err while another goroutine's sync.Once loader was still running. Store the loader function in the cache entry so any goroutine calling entry.load() uses the correct loader via sync.Once synchronization. Closes #32 Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request implements package exclusion support for CDN-based import map generation. The changes add an Changes
Sequence DiagramsequenceDiagram
actor Client
participant WASM as WASM Layer
participant Resolver as Resolver
participant Cache as PackageCache
participant Registry as Registry
Client->>WASM: generate(deps, {exclude: [...]})
WASM->>WASM: parseGenerateOptions(exclude)
WASM->>Resolver: WithExclude(packages)
Resolver->>Resolver: store excludePackages
WASM->>Resolver: ResolvePackageJSON(deps)
Resolver->>Resolver: filter direct deps (remove excluded)
Resolver->>Resolver: resolvePackage(each dep, transitive)
alt Package in transitive path?
Resolver->>Resolver: check slices.Contains(excludePackages)
Resolver->>Resolver: skip if excluded
else Package not excluded
Resolver->>Cache: GetOrLoad(pkg)
Cache->>Registry: fetch if needed
Registry-->>Cache: version data
Cache-->>Resolver: return pkg info
end
Resolver->>Resolver: build im.Imports & im.Scopes
Resolver-->>WASM: ImportMap (no excluded pkgs)
WASM-->>Client: ImportMap
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 53 minutes and 50 seconds.Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Build Artifacts
Built from a62e59e @ fix/cdn-exclude-transitive-deps |
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)
cdn/cache.go (1)
138-148:⚠️ Potential issue | 🔴 CriticalFix eviction order in
GetOrLoadto avoid under-capacity behavior and edge panic.At Line 143, eviction runs after inserting the new entry with
>=. This can evict too early (effective capacity becomesmaxSize-1) and can panic at Line 144 whenmaxSize == 1andorderis still empty.🐛 Proposed fix
- // Create new entry with loader - entry = &cacheEntry{loader: loader} - c.entries[key] = entry - - // Evict oldest if at capacity - if len(c.entries) >= c.maxSize { + // Evict oldest if at capacity + if len(c.entries) >= c.maxSize { oldest := c.order[0] c.order = c.order[1:] delete(c.entries, oldest) } + // Create new entry with loader + entry = &cacheEntry{loader: loader} + c.entries[key] = entry c.order = append(c.order, key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cdn/cache.go` around lines 138 - 148, The eviction logic in GetOrLoad currently runs after inserting the new entry and uses >= which can underflow capacity and panic; move the eviction block to run before creating/inserting the new entry and change the check to compare equality (if len(c.entries) == c.maxSize) so you evict exactly one oldest key from c.order (oldest := c.order[0]; c.order = c.order[1:]; delete(c.entries, oldest)) prior to setting c.entries[key] = entry, then append the new key to c.order; reference symbols: GetOrLoad, c.entries, c.order, c.maxSize, key, entry.
🧹 Nitpick comments (2)
resolve/cdn/cdn.go (1)
189-201: Consider copyingpackagesinWithExcludeto keep resolver instances immutable.Right now
excludePackagespoints at the caller-provided slice. A later mutation by the caller can alter resolver behavior unexpectedly.♻️ Proposed hardening
func (r *Resolver) WithExclude(packages []string) *Resolver { + excluded := append([]string(nil), packages...) return &Resolver{ fetcher: r.fetcher, provider: r.provider, registry: r.registry, template: r.template, cache: r.cache, logger: r.logger, conditions: r.conditions, includeDev: r.includeDev, - excludePackages: packages, + excludePackages: excluded, maxDepth: r.maxDepth, resolveScope: r.resolveScope, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resolve/cdn/cdn.go` around lines 189 - 201, WithExclude currently assigns the caller's slice directly to the Resolver.excludePackages field which preserves aliasing; change WithExclude to allocate a new slice and copy the elements into it before storing (e.g., create make([]string, len(packages)); copy(...)) so the returned Resolver owns an independent slice and cannot be mutated by the caller; update the return construction in WithExclude to use that copied slice (reference symbols: WithExclude, excludePackages, Resolver).resolve/cdn/cdn_test.go (1)
251-256: Move inlinePackageJSONinputs to fixtures for consistency with repo test style.These inline inputs work, but this repo’s test guidance prefers fixture-based inputs/goldens in
testdata/for maintainability and reuse.As per coding guidelines, "Don't inline source code in tests; use fixture files instead with input files and expected output".
Also applies to: 273-277, 312-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resolve/cdn/cdn_test.go` around lines 251 - 256, The test currently constructs packagejson.PackageJSON inline (pkg := &packagejson.PackageJSON{...}) which violates the repo style; instead create JSON fixture files under testdata (one per case covering the instances at the noted locations) containing the same Dependencies map, then update the tests to read and unmarshal those fixtures into a packagejson.PackageJSON (e.g., os.ReadFile + json.Unmarshal into a variable named pkg) before asserting; replace the three inline occurrences (the pkg := &packagejson.PackageJSON blocks at the referenced spots) with code that loads the corresponding fixture file and unmarshals it into packagejson.PackageJSON so tests use testdata fixtures consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resolve/cdn/cdn_test.go`:
- Around line 304-307: The test's exclusion assertions on im.Imports (and
similarly on im.Scopes) are too narrow — they only check exact keys "lib-b" and
"lib-b/helpers" and miss other subpaths; update the checks in the loop over
im.Imports (and the loop over im.Scopes around lines 326-336) to reject any key
that equals "lib-b" or has the prefix "lib-b/" (e.g., use a prefix check such as
strings.HasPrefix(k, "lib-b") or strings.HasPrefix(k, "lib-b/") to fail the test
on any subpath), and apply the same prefix-based logic to the scopes loop to
ensure no "lib-b" subpaths are emitted.
---
Outside diff comments:
In `@cdn/cache.go`:
- Around line 138-148: The eviction logic in GetOrLoad currently runs after
inserting the new entry and uses >= which can underflow capacity and panic; move
the eviction block to run before creating/inserting the new entry and change the
check to compare equality (if len(c.entries) == c.maxSize) so you evict exactly
one oldest key from c.order (oldest := c.order[0]; c.order = c.order[1:];
delete(c.entries, oldest)) prior to setting c.entries[key] = entry, then append
the new key to c.order; reference symbols: GetOrLoad, c.entries, c.order,
c.maxSize, key, entry.
---
Nitpick comments:
In `@resolve/cdn/cdn_test.go`:
- Around line 251-256: The test currently constructs packagejson.PackageJSON
inline (pkg := &packagejson.PackageJSON{...}) which violates the repo style;
instead create JSON fixture files under testdata (one per case covering the
instances at the noted locations) containing the same Dependencies map, then
update the tests to read and unmarshal those fixtures into a
packagejson.PackageJSON (e.g., os.ReadFile + json.Unmarshal into a variable
named pkg) before asserting; replace the three inline occurrences (the pkg :=
&packagejson.PackageJSON blocks at the referenced spots) with code that loads
the corresponding fixture file and unmarshals it into packagejson.PackageJSON so
tests use testdata fixtures consistently.
In `@resolve/cdn/cdn.go`:
- Around line 189-201: WithExclude currently assigns the caller's slice directly
to the Resolver.excludePackages field which preserves aliasing; change
WithExclude to allocate a new slice and copy the elements into it before storing
(e.g., create make([]string, len(packages)); copy(...)) so the returned Resolver
owns an independent slice and cannot be mutated by the caller; update the return
construction in WithExclude to use that copied slice (reference symbols:
WithExclude, excludePackages, Resolver).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39257635-07df-439a-a751-e7cd4a26e535
📒 Files selected for processing (9)
cdn/cache.gonpm/src/mappa.tsresolve/cdn/cdn.goresolve/cdn/cdn_test.goresolve/cdn/testdata/lib-a-package/package.jsonresolve/cdn/testdata/lib-a-registry/response.jsonresolve/cdn/testdata/lib-b-package/package.jsonresolve/cdn/testdata/lib-b-registry/response.jsonwasm/main.go
Use strings.HasPrefix for lib-b exclusion checks instead of exact key matching, so any subpath is caught. Move eviction before insertion in GetOrLoad to match Set() ordering and prevent panic on empty c.order with small maxSize. Change >= to == since the check now runs before the new entry is added. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
WithExcludeto CDN resolver, filtering excluded packages from both direct and transitive dependenciesPackageCache.GetOrLoadwhere fast path read entry fields whilesync.Onceloader was still runningexcludethrough WASMgenerate()API and update TypeScriptGenerateOptionstypeDetails
The CDN resolver lacked exclude support entirely. When using
generate()with theexcludeoption, excluded packages still appeared in the output import map as transitive dependencies of non-excluded packages. This broke monorepo workflows where workspace packages should be served locally rather than from CDN.The race fix stores the loader function in
cacheEntryso any goroutine'sentry.load()call uses the correct loader viasync.Oncesynchronization, rather than the old pattern where the fast path could readentry.pkgbefore the loader finished.Closes #32
Test plan
TestResolverWithExclude/without_exclude_both_present-- baseline, both packages resolvedTestResolverWithExclude/transitive_deps_produce_scopes-- proves lib-b appears in scopes before exclusionTestResolverWithExclude/exclude_direct_dependency-- lib-b removed from importsTestResolverWithExclude/exclude_transitive_dependency-- lib-b removed from imports and scopes when only lib-a is a direct depmake lintpasses (0 issues)make testpasses (331 tests,-raceenabled)Summary by CodeRabbit
New Features
Tests
Refactor