fix: spec compliance, test coverage, and scope optimization#28
Merged
Conversation
- packagejson: handle null export targets (package encapsulation) - packagejson: handle array fallback exports - packagejson: handle null in conditional export conditions - importmap: add Validate() for WHATWG trailing-slash and URL checks - importmap: export SimplifyEntries() for scope-level simplification - resolve/local: simplify scope entries before merging (removes explicit exports covered by trailing-slash keys from wildcards) - inject: add unit test coverage (was 0) - Add test fixtures for null exports, array fallbacks, null conditions, validation, simplify edge cases, and wildcard scope simplification Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds import-map validation and exported simplification utilities, derives import-map entries from package.json (imports/exports) with new resolution paths, refactors resolvers/CDN/generator to use those entries (optionally simplifying via a new optimize flag), and adds extensive tests and fixtures for importmap, package.json, inject, resolve, and trace behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmd/generate)
participant Resolver as Resolver (local/cdn)
participant Package as PackageJSON
participant ImportMap as ImportMap
participant Output as Renderer
CLI->>Resolver: request import map generation (opts, optimizeLevel)
Resolver->>Package: ImportMapEntries(opts)
Package-->>Resolver: []ImportMapEntry(key,path)
Resolver->>ImportMap: merge entries into imports/scopes
Resolver->>ImportMap: SimplifyEntries(imports/scopes)
CLI->>ImportMap: Validate() (warnings/errors)
alt optimizeLevel >= 1
Resolver->>ImportMap: use simplified map
else
Resolver->>ImportMap: use unsimplified map
end
ImportMap->>Output: render final import map (json/html)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. Comment |
Build Artifacts
Built from 79be580 @ spec-coverage-optimization |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
inject/inject_test.go (1)
29-36: Move test HTML/package fixtures intotestdata/.These tests inline HTML,
package.json, and JS source fixtures. Please move them underinject/testdata/and load them via the project fixture filesystem helper instead of constructing source files in test bodies/helpers. As per coding guidelines,**/*_test.go: “Practice TDD and use the fixture/golden patterns for testing with fixtures intestdata/directories and goldens as expected output files”, “Always usetestutil.NewFixtureFSfor tests”, and “Don't inline source code in tests; use fixture files instead with input files and expected output”.Also applies to: 64-76, 105-108, 245-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inject/inject_test.go` around lines 29 - 36, The tests inline HTML, package.json and JS sources (the html variable and other inline fixtures) — move those files into a package-level testdata directory and update tests to load them using testutil.NewFixtureFS instead of constructing source bytes in the test body; replace direct uses of the html byte slice and inline package/app.js content with reads from the fixture FS and update any helpers in inject_test.go that create files to use the fixture loader so tests use input files and golden outputs from testdata rather than inline strings.importmap/importmap_test.go (1)
287-294: Move these import-map cases into fixtures/goldens.These are import-map input/expected-output cases, so keeping them under
testdata/importmap/...would match the rest of this suite and make the new edge cases easier to extend. As per coding guidelines, “Practice TDD and use the fixture/golden patterns for testing with fixtures intestdata/directories and goldens as expected output files” and “Don't inline source code in tests; use fixture files instead with input files and expected output”.Also applies to: 387-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@importmap/importmap_test.go` around lines 287 - 294, The inline "scope validation" test case (t.Run "scope validation") that constructs an importmap.ImportMap with Scopes should be converted into test fixtures: move the input import-map JSON/YAML into testdata/importmap/<name>.input and the expected normalized output into a matching goldens file (e.g. testdata/importmap/<name>.golden); update the test to load these fixture files and assert equality rather than inlining the ImportMap struct. Do the same refactor for the other inline cases mentioned (the block around the later scope-related cases), ensuring the test harness reads from testdata/importmap files and uses the goldens for expected output so new edge cases can be added as files rather than code changes.packagejson/packagejson_test.go (1)
509-516: Also assert allowed entries are still enumerated.This only verifies blocked subpaths are absent. If
ExportEntries(nil)accidentally dropped all entries, this section would still pass despite losing valid exports.Suggested test strengthening
entries := pkg.ExportEntries(nil) + found := make(map[string]bool, len(entries)) for _, e := range entries { + found[e.Subpath] = true for _, blocked := range expected.Blocked { if e.Subpath == blocked { t.Errorf("ExportEntries should not include blocked subpath %q", blocked) } } } + for subpath := range expected.Exports { + if !found[subpath] { + t.Errorf("ExportEntries should include exported subpath %q", subpath) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packagejson/packagejson_test.go` around lines 509 - 516, The test currently only checks that blocked subpaths from expected.Blocked are absent; add assertions that expected.Allowed entries are present by calling pkg.ExportEntries(nil) (same entries variable) and verifying for each allowed subpath in expected.Allowed there exists an entry with e.Subpath == allowed; use t.Errorf (or t.Fatalf) if any expected allowed subpath is missing to ensure ExportEntries(nil) still enumerates valid exports. Reference ExportEntries, entries, expected.Blocked, expected.Allowed and e.Subpath when locating where to add these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importmap/importmap.go`:
- Around line 249-253: The current simplifyImports used by SimplifyEntries
removes entries like "lit/foo.js" whenever a "lit/" key exists regardless of
targets, which silently drops explicit overrides; update simplifyImports so it
only elides a more-specific key when the more-general key's target plus the
key's suffix equals the specific key's target (i.e., for a base key "P/" with
target T, only remove "P + S" if its target == join(T, S)). Locate
simplifyImports and the logic that scans trailing-slash keys and change the
condition to compare the constructed target (baseTarget + suffix) to the
explicit target before deleting the specific entry, leaving non-matching
overrides intact.
- Around line 50-54: The scope keys in im.Scopes are URL prefixes and currently
aren’t validated; before calling validateSpecifierMap(imports, scope) loop, run
the same validation used for specifier values on the scope string and append any
returned *ValidationError to errs (i.e., validate the scope prefix as a URL
using the existing specifier validation logic/function used elsewhere), then
continue to validate the inner map with validateSpecifierMap; update im.Scopes
handling to push scope validation errors into errs prior to validating the
imports.
- Around line 83-95: The isValidSpecifierValue function currently uses prefix
checks; replace that with proper URL parsing: keep the quick acceptance for
relative paths starting with "/", "./", or "../", otherwise parse the value with
net/url.Parse and ensure parsing succeeded and that the parsed URL has a
non-empty Scheme and also contains meaningful content after the scheme (i.e.
u.Opaque != "" OR u.Host != "" OR u.Path starts with "/"), which will accept
schemes like data: and blob: but reject bare scheme-only strings like
"https://". Update the validation in isValidSpecifierValue to implement this
logic and remove the ad-hoc "http(s)://" prefix checks.
In `@inject/inject_test.go`:
- Around line 217-225: The test currently iterates over the results channel from
InjectBatch but doesn't fail if the channel emits nothing; update the test that
calls InjectBatch (variable results) to first assert that at least one result is
received (e.g., perform a blocking read with a timeout or read a first value
with select and fail the test if none arrives), then continue to validate that
each received result has a non-empty Error; apply the same change to the other
similar block that checks error-paths so the test fails if InjectBatch returns
no results.
---
Nitpick comments:
In `@importmap/importmap_test.go`:
- Around line 287-294: The inline "scope validation" test case (t.Run "scope
validation") that constructs an importmap.ImportMap with Scopes should be
converted into test fixtures: move the input import-map JSON/YAML into
testdata/importmap/<name>.input and the expected normalized output into a
matching goldens file (e.g. testdata/importmap/<name>.golden); update the test
to load these fixture files and assert equality rather than inlining the
ImportMap struct. Do the same refactor for the other inline cases mentioned (the
block around the later scope-related cases), ensuring the test harness reads
from testdata/importmap files and uses the goldens for expected output so new
edge cases can be added as files rather than code changes.
In `@inject/inject_test.go`:
- Around line 29-36: The tests inline HTML, package.json and JS sources (the
html variable and other inline fixtures) — move those files into a package-level
testdata directory and update tests to load them using testutil.NewFixtureFS
instead of constructing source bytes in the test body; replace direct uses of
the html byte slice and inline package/app.js content with reads from the
fixture FS and update any helpers in inject_test.go that create files to use the
fixture loader so tests use input files and golden outputs from testdata rather
than inline strings.
In `@packagejson/packagejson_test.go`:
- Around line 509-516: The test currently only checks that blocked subpaths from
expected.Blocked are absent; add assertions that expected.Allowed entries are
present by calling pkg.ExportEntries(nil) (same entries variable) and verifying
for each allowed subpath in expected.Allowed there exists an entry with
e.Subpath == allowed; use t.Errorf (or t.Fatalf) if any expected allowed subpath
is missing to ensure ExportEntries(nil) still enumerates valid exports.
Reference ExportEntries, entries, expected.Blocked, expected.Allowed and
e.Subpath when locating where to add these checks.
🪄 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: af0e006a-93bd-4b4e-b4d5-e8568a50484b
⛔ Files ignored due to path filters (2)
testdata/resolve/with-wildcard-scopes/node_modules/dep-with-wildcards/package.jsonis excluded by!**/node_modules/**testdata/resolve/with-wildcard-scopes/node_modules/parent-pkg/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (20)
importmap/importmap.goimportmap/importmap_test.goinject/inject_test.gopackagejson/packagejson.gopackagejson/packagejson_test.goresolve/local/local.goresolve/local/local_test.gotestdata/importmap/simplify-keeps-bare-specifier/expected.jsontestdata/importmap/simplify-keeps-bare-specifier/input.jsontestdata/importmap/validate-invalid-values/input.jsontestdata/importmap/validate-trailing-slash/input.jsontestdata/importmap/validate-valid/input.jsontestdata/packagejson/array-fallback-exports/expected.jsontestdata/packagejson/array-fallback-exports/package.jsontestdata/packagejson/mixed-null-and-valid/expected.jsontestdata/packagejson/mixed-null-and-valid/package.jsontestdata/packagejson/null-exports/expected.jsontestdata/packagejson/null-exports/package.jsontestdata/resolve/with-wildcard-scopes/expected.jsontestdata/resolve/with-wildcard-scopes/package.json
Coverage: - Add circular dependency test (resolver + trace module graph) - Add nested node_modules resolution with test fixture - Add peer/optional dependency support in scopes - Add malformed JSON error path tests (packagejson + importmap) - Add namespace re-export test (export * as ns from) - Add trace circular module graph test Spec: - Implement `#imports` subpath resolution (ResolveImport method) with wildcard, conditional, and null target support - Parse PeerDependencies and OptionalDependencies fields Optimization: - Deduplicate scope entries matching top-level imports in Simplify() - Add `--optimize` / `-O` flag to generate command (0=none, 1=default) Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- simplifyImports: only elide entries when the trailing-slash expansion matches the explicit target, preserving intentional overrides - Validate: check scope keys as URL prefixes - isValidSpecifierValue: use net/url.Parse for proper URL validation (accepts data:, blob: schemes; rejects bare strings) - inject tests: assert at least one result from InjectBatch channels - inject tests: move inline HTML to testdata fixtures - packagejson tests: verify allowed subpaths are present in ExportEntries - importmap tests: move inline scope validation to fixture file - Update with-wildcard-scopes expected.json to reflect correct behavior: explicit exports with different targets are preserved alongside trailing-slash keys Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract resolve.PackageName() and replace parsePackageName (local.go) and getPackageName (graph.go) with calls to the shared function - Add PackageJSON.ImportMapEntries() that encapsulates the repeated ExportEntries + WildcardExports + trailing-slash pattern - Refactor 5 call sites in local.go and 1 in cdn.go to use ImportMapEntries, removing ~100 lines of duplicated logic - Remove unused "path" import from trace/graph.go Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resolve/local/local.go (1)
702-719:⚠️ Potential issue | 🟠 MajorRecurse using the resolved dependency path, not the hoisted package name.
After
findDependencyPathfinds a nested copy, line 719 still recurses withnodeModulesPath+depName, so the next frame reads the hoisted package and builds the wrong scope. Keyvisitedby the resolved package path/version and passdepPathinto the recursive processing path so nested copies get their own dependency scopes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resolve/local/local.go` around lines 702 - 719, The recursion is using the hoisted package name instead of the resolved filesystem path, causing nested copies to reuse the wrong scope; change the recursive call to processPackageDependenciesParallelWithGraph to pass the resolved depPath (and use a visited key based on the resolved package path/version or depPath) instead of nodeModulesPath+depName so nested packages are processed separately. Specifically, after findDependencyPath/parsePackageJSON/expandDepURL, call r.processPackageDependenciesParallelWithGraph(im, mu, visited, depPath, depName, rootDir, graph) and ensure the visited map keys are derived from the resolved package path/version (e.g., depPath or depPkg identifier) rather than the original depName.
🧹 Nitpick comments (5)
trace/trace_test.go (1)
529-548: Move the expected graph shape into the fixture golden.This test already uses a fixture FS, but the expected module and entrypoint counts are still hard-coded. Consider adding
/test/expected.jsonfortrace/circular-modulesand loading it here, matching the surrounding tests so fixture changes and expectations stay together.♻️ Suggested direction
func TestTraceHTML_CircularModules(t *testing.T) { mfs := testutil.NewFixtureFS(t, "trace/circular-modules", "/test") tracer := NewTracer(mfs, "/test") graph, err := tracer.TraceHTML("/test/index.html") if err != nil { t.Fatalf("TraceHTML with circular modules failed: %v", err) } + + expectedBytes, err := mfs.ReadFile("/test/expected.json") + if err != nil { + t.Fatalf("Failed to read expected.json: %v", err) + } + + var expected struct { + Entrypoints []string `json:"entrypoints"` + Modules []string `json:"modules"` + } + if err := json.Unmarshal(expectedBytes, &expected); err != nil { + t.Fatalf("Failed to parse expected.json: %v", err) + } - if len(graph.Modules) != 2 { - t.Errorf("Expected 2 modules (a.js, b.js), got %d", len(graph.Modules)) + if len(graph.Modules) != len(expected.Modules) { + t.Errorf("Expected %d modules, got %d", len(expected.Modules), len(graph.Modules)) for path := range graph.Modules { t.Logf(" Module: %s", path) } } - if len(graph.Entrypoints) != 1 { - t.Errorf("Expected 1 entrypoint, got %d", len(graph.Entrypoints)) + if len(graph.Entrypoints) != len(expected.Entrypoints) { + t.Errorf("Expected %d entrypoints, got %d", len(expected.Entrypoints), len(graph.Entrypoints)) } }As per coding guidelines,
**/*_test.go: “Practice TDD and use the fixture/golden patterns for testing with fixtures intestdata/directories and goldens as expected output files”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@trace/trace_test.go` around lines 529 - 548, TestTraceHTML_CircularModules hard-codes expected module/entrypoint counts instead of using a fixture golden; update the test to load the expected shape from the fixture golden (e.g. add /test/expected.json under the trace/circular-modules fixture) and assert TraceHTML's result against that golden rather than comparing len(graph.Modules) and len(graph.Entrypoints) directly; locate the test function TestTraceHTML_CircularModules and use the same pattern as surrounding tests (using NewTracer, TraceHTML and the fixture FS) to read and unmarshal the expected JSON and compare the actual graph.Modules and graph.Entrypoints to the expected structure.inject/inject_test.go (1)
243-296: Move the batch-injection fixture files undertestdata/.The inline HTML, package.json, and JS module content should be fixture files loaded through
testutil.NewFixtureFS, matching the rest of this test file. As per coding guidelines,**/*_test.go: “Practice TDD and use the fixture/golden patterns for testing with fixtures intestdata/directories and goldens as expected output files” and “Don't inline source code in tests; use fixture files instead with input files and expected output”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inject/inject_test.go` around lines 243 - 296, TestInjectBatch_MultipleFiles currently inlines fixture files via setupInjectFixture and mapfs.New; replace those inline fixtures with files under testdata/ and load them with testutil.NewFixtureFS so tests follow the project's fixture/golden pattern. Specifically: create the fixture files (package.json, node_modules/lit/package.json, node_modules/lit/index.js, index.html, other.html) under the testdata directory used by this test, update setupInjectFixture to call testutil.NewFixtureFS(...) to return the fs, and update TestInjectBatch_MultipleFiles to use that fixture fs when calling InjectBatch (preserving the call to InjectBatch and Options{DryRun:true}) so behavior and paths remain the same.packagejson/packagejson_test.go (1)
738-757: Move this package.json sample into a fixture.This new dependency-parsing case should use
testdata/packagejson/.../package.jsonlike the surrounding packagejson tests, keeping expected package data out of the test body. As per coding guidelines,**/*_test.go: “Don't inline source code in tests; use fixture files instead with input files and expected output”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packagejson/packagejson_test.go` around lines 738 - 757, The test TestPeerAndOptionalDependencies currently inlines a package.json string; move that JSON into a fixture file under testdata/packagejson/<something>/package.json and update the test to read the fixture and pass its bytes to packagejson.Parse instead of the hardcoded input; keep the assertions on pkg.PeerDependencies["peer-b"] and pkg.OptionalDependencies["opt-c"] and ensure the test uses the same test helper pattern as neighboring tests (read file, call packagejson.Parse, check err) so no expected data is embedded in the test body.importmap/importmap.go (1)
307-316: Keep checking overlapping trailing-slash prefixes until one actually covers the entry.Line 315 breaks after the first key-prefix match, even when
baseTarget+relPath != value. With overlapping entries likepkg/andpkg/sub/, map iteration order can keep entries thatpkg/sub/would correctly simplify.♻️ Proposed fix
for tsKey := range trailingSlashKeys { if strings.HasPrefix(key, tsKey) { relPath := key[len(tsKey):] baseTarget := imports[tsKey] if baseTarget+relPath == value { covered = true + break } - break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@importmap/importmap.go` around lines 307 - 316, The loop over trailingSlashKeys currently breaks unconditionally after the first prefix match, which prevents checking other overlapping prefixes (e.g., "pkg/" vs "pkg/sub/"); in the function/section handling keys (variables: key, value, trailingSlashKeys, imports, covered) only stop iterating when you actually find a covering mapping—i.e., set covered = true and break inside the conditional that checks if baseTarget+relPath == value, otherwise continue checking other tsKey entries (remove or relocate the unconditional break so it only runs when covered is set).resolve/template.go (1)
105-130: LGTM — clean refactor withstrings.Cut.Both
SplitPackageNameand the newPackageNamecorrectly handle scoped (@scope/pkg[/sub]), unscoped (name[/sub]), and no-slash edge cases. Delegatinglocal.parsePackageNameandtrace.getPackageNametoresolve.PackageNameremoves duplication cleanly.Minor nit (optional): inside
SplitPackageNametheif scope, name, ok := strings.Cut(pkg, "/"); ok { ... }shadows the named return valuesname, scope. It's functionally fine but slightly confusing to read — consider renaming the locals (e.g.s, n) to avoid the shadow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resolve/template.go` around lines 105 - 130, SplitPackageName currently declares local variables scope and name in the strings.Cut call which shadow the function's named return values (name, scope); rename those locals (e.g., to s, n or localScope, localName) inside the if block so they don’t shadow the named returns and then return the correct values using the function-level names, keeping the same logic around strings.Cut and strings.TrimPrefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/generate/generate.go`:
- Line 65: The help text for the --optimize flag (registered via
Cmd.Flags().IntP("optimize", "O", 1, ...)) advertises a level 2 behavior that
doesn't exist: the code path that reads the optimize value always calls
Simplify() for any value >= 1 (the Simplify() branch). Either implement a
distinct level-2 branch where optimize == 2 runs the dedup logic, or (simpler)
change the help text to accurately describe the implemented behavior (e.g.,
"Optimization level: 0=none, 1=simplify (default), >=1=simplify") so the flag
description matches the Simplify()-only implementation.
In `@packagejson/packagejson.go`:
- Around line 160-166: The wildcard-to-folder collapse is losing extension
transformations: in PackageJSON.packagejson.go the loop using
pkg.WildcardExports(opts) and computing patternPrefix via
trimDotSlash(w.Pattern) then trimming "*" should only emit an ImportMapEntry
folder mapping when both the pattern and the target are pure folder
substitutions (i.e., after removing the "*" both pattern and target end with a
"/"); otherwise do not collapse to a folder mapping (skip adding to result or
leave for subpath-level resolution) so transforms like "./features/*.js" ->
"./src/features/*.mjs" are not converted to a folder mapping that loses the
".mjs" trailer. Update the logic around patternPrefix and the ImportMapEntry
append so it checks the post-trim suffixes of w.Pattern and w.Target (or use a
helper) and only append the folder mapping when both are folder-style wildcards;
otherwise exclude them from ImportMapEntries for separate handling.
- Around line 169-174: The code currently falls back to pkg.Main whenever no
entries resolved, even if pkg.Exports exists; change the condition in the block
that appends the bare ImportMapEntry (which uses entries, pkg.Main, pkg.Exports,
trimDotSlash, and result) so it only falls back to pkg.Main when pkg.Exports is
absent (e.g., pkg.Exports == nil or truly empty) — i.e., require len(entries) ==
0 && pkg.Main != "" && (pkg.Exports == nil || isEmptyExports(pkg.Exports)) so
that when exports is defined but omits "." we do not export the bare package.
Ensure the check references the actual exports representation used in this
codebase.
- Around line 216-225: In the pattern loop inside ResolveImport (and analogously
in ResolveExport) the code currently treats any error from
resolveExportValueWithOpts(value, opts) as a non-match and continues; instead
detect when resolveExportValueWithOpts returns an error representing an explicit
null target and return that error immediately (do not fall through to
less-specific patterns). Locate the loop that iterates over patterns and uses
importsMap[pattern], matchExportPattern(specifier) and
resolveExportValueWithOpts, and change the control flow so that on errors from
resolveExportValueWithOpts (the null/blocked-target case) you return the error
from ResolveImport/ResolveExport rather than continue, while still continuing
only when matchExportPattern indicates no match.
In `@trace/trace_test.go`:
- Around line 550-572: TestExtractImports_NamespaceReExport currently inlines
the JS source; move that input into a fixture file (e.g., create a new fixture
JS file with the two export lines) and add the expected imports to the
corresponding expected.json used by
TestExtractImports/TestExtractImports_ReExports; then update
TestExtractImports_NamespaceReExport to read the fixture file and expected.json
and call ExtractImports(js) the same way the other tests do, preserving the
existing assertions (verify two imports, ensure IsDynamic is false for
re-exports, and check for "namespace-module" and "star-module" in the results).
---
Outside diff comments:
In `@resolve/local/local.go`:
- Around line 702-719: The recursion is using the hoisted package name instead
of the resolved filesystem path, causing nested copies to reuse the wrong scope;
change the recursive call to processPackageDependenciesParallelWithGraph to pass
the resolved depPath (and use a visited key based on the resolved package
path/version or depPath) instead of nodeModulesPath+depName so nested packages
are processed separately. Specifically, after
findDependencyPath/parsePackageJSON/expandDepURL, call
r.processPackageDependenciesParallelWithGraph(im, mu, visited, depPath, depName,
rootDir, graph) and ensure the visited map keys are derived from the resolved
package path/version (e.g., depPath or depPkg identifier) rather than the
original depName.
---
Nitpick comments:
In `@importmap/importmap.go`:
- Around line 307-316: The loop over trailingSlashKeys currently breaks
unconditionally after the first prefix match, which prevents checking other
overlapping prefixes (e.g., "pkg/" vs "pkg/sub/"); in the function/section
handling keys (variables: key, value, trailingSlashKeys, imports, covered) only
stop iterating when you actually find a covering mapping—i.e., set covered =
true and break inside the conditional that checks if baseTarget+relPath ==
value, otherwise continue checking other tsKey entries (remove or relocate the
unconditional break so it only runs when covered is set).
In `@inject/inject_test.go`:
- Around line 243-296: TestInjectBatch_MultipleFiles currently inlines fixture
files via setupInjectFixture and mapfs.New; replace those inline fixtures with
files under testdata/ and load them with testutil.NewFixtureFS so tests follow
the project's fixture/golden pattern. Specifically: create the fixture files
(package.json, node_modules/lit/package.json, node_modules/lit/index.js,
index.html, other.html) under the testdata directory used by this test, update
setupInjectFixture to call testutil.NewFixtureFS(...) to return the fs, and
update TestInjectBatch_MultipleFiles to use that fixture fs when calling
InjectBatch (preserving the call to InjectBatch and Options{DryRun:true}) so
behavior and paths remain the same.
In `@packagejson/packagejson_test.go`:
- Around line 738-757: The test TestPeerAndOptionalDependencies currently
inlines a package.json string; move that JSON into a fixture file under
testdata/packagejson/<something>/package.json and update the test to read the
fixture and pass its bytes to packagejson.Parse instead of the hardcoded input;
keep the assertions on pkg.PeerDependencies["peer-b"] and
pkg.OptionalDependencies["opt-c"] and ensure the test uses the same test helper
pattern as neighboring tests (read file, call packagejson.Parse, check err) so
no expected data is embedded in the test body.
In `@resolve/template.go`:
- Around line 105-130: SplitPackageName currently declares local variables scope
and name in the strings.Cut call which shadow the function's named return values
(name, scope); rename those locals (e.g., to s, n or localScope, localName)
inside the if block so they don’t shadow the named returns and then return the
correct values using the function-level names, keeping the same logic around
strings.Cut and strings.TrimPrefix.
In `@trace/trace_test.go`:
- Around line 529-548: TestTraceHTML_CircularModules hard-codes expected
module/entrypoint counts instead of using a fixture golden; update the test to
load the expected shape from the fixture golden (e.g. add /test/expected.json
under the trace/circular-modules fixture) and assert TraceHTML's result against
that golden rather than comparing len(graph.Modules) and len(graph.Entrypoints)
directly; locate the test function TestTraceHTML_CircularModules and use the
same pattern as surrounding tests (using NewTracer, TraceHTML and the fixture
FS) to read and unmarshal the expected JSON and compare the actual graph.Modules
and graph.Entrypoints to the expected structure.
🪄 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: feff392c-53ae-4af7-a721-08d74c513285
⛔ Files ignored due to path filters (7)
testdata/resolve/circular-deps/node_modules/pkg-a/package.jsonis excluded by!**/node_modules/**testdata/resolve/circular-deps/node_modules/pkg-b/package.jsonis excluded by!**/node_modules/**testdata/resolve/nested-node-modules/node_modules/parent-pkg/node_modules/shared-dep/package.jsonis excluded by!**/node_modules/**testdata/resolve/nested-node-modules/node_modules/parent-pkg/package.jsonis excluded by!**/node_modules/**testdata/resolve/nested-node-modules/node_modules/shared-dep/package.jsonis excluded by!**/node_modules/**testdata/resolve/with-peer-deps/node_modules/framework/package.jsonis excluded by!**/node_modules/**testdata/resolve/with-peer-deps/node_modules/plugin-pkg/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (31)
cmd/generate/generate.goimportmap/importmap.goimportmap/importmap_test.goinject/inject_test.gopackagejson/packagejson.gopackagejson/packagejson_test.goresolve/cdn/cdn.goresolve/local/local.goresolve/local/local_test.goresolve/template.gotestdata/importmap/simplify-dedup-scopes/expected.jsontestdata/importmap/simplify-dedup-scopes/input.jsontestdata/importmap/validate-scope-entries/input.jsontestdata/inject/build-insert-new/index.htmltestdata/inject/build-no-head/index.htmltestdata/inject/build-replace-existing/index.htmltestdata/packagejson/null-exports/expected.jsontestdata/packagejson/subpath-imports/expected.jsontestdata/packagejson/subpath-imports/package.jsontestdata/resolve/circular-deps/expected.jsontestdata/resolve/circular-deps/package.jsontestdata/resolve/nested-node-modules/expected.jsontestdata/resolve/nested-node-modules/package.jsontestdata/resolve/with-peer-deps/expected.jsontestdata/resolve/with-peer-deps/package.jsontestdata/resolve/with-wildcard-scopes/expected.jsontestdata/trace/circular-modules/a.jstestdata/trace/circular-modules/b.jstestdata/trace/circular-modules/index.htmltrace/graph.gotrace/trace_test.go
✅ Files skipped from review due to trivial changes (20)
- testdata/trace/circular-modules/index.html
- testdata/inject/build-insert-new/index.html
- testdata/importmap/validate-scope-entries/input.json
- testdata/resolve/circular-deps/expected.json
- testdata/inject/build-no-head/index.html
- testdata/resolve/circular-deps/package.json
- testdata/resolve/with-peer-deps/package.json
- testdata/importmap/simplify-dedup-scopes/input.json
- testdata/trace/circular-modules/b.js
- testdata/packagejson/null-exports/expected.json
- testdata/packagejson/subpath-imports/expected.json
- testdata/trace/circular-modules/a.js
- testdata/resolve/nested-node-modules/expected.json
- testdata/importmap/simplify-dedup-scopes/expected.json
- testdata/resolve/with-wildcard-scopes/expected.json
- testdata/packagejson/subpath-imports/package.json
- testdata/resolve/nested-node-modules/package.json
- testdata/inject/build-replace-existing/index.html
- testdata/resolve/with-peer-deps/expected.json
- trace/graph.go
🚧 Files skipped from review as they are similar to previous changes (1)
- resolve/local/local_test.go
- simplifyImports: only break when a covering mapping is found, allowing overlapping trailing-slash prefixes to be checked - ImportMapEntries: skip non-pure folder wildcards (e.g., ./*.js -> ./src/*.mjs) that can't be expressed as trailing-slash keys - ImportMapEntries: only fall back to Main when Exports is nil - ResolveExport/ResolveImport: propagate null target errors from wildcard matches instead of falling through to less-specific patterns - WildcardExport: add TargetSuffix field for extension transforms - processPackageDependenciesParallelWithGraph: use resolved path as visited key so nested copies are processed independently - Validate: check scope keys as valid URL prefixes - Fix --optimize help text to match implementation - Fix SplitPackageName variable shadowing - Move inline test data to fixtures (re-exports, circular modules, peer/optional deps, inject setup) Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
importmap/importmap.go (1)
238-247: Scope dedup only catches exact-key overlaps with top-level.The dedup loop compares each simplified scope entry against
result.Imports[key]by exact key, butresult.Importshas already been simplified (its trailing-slash keys absorb concrete entries). As a result, a scope entry like"lit/foo.js" -> "/a/foo.js"is not removed even when top-level has"lit/" -> "/a/", which would resolve the same specifier to the same URL via fallback. This leaves redundant entries inScopesthat the PR's "dedup scopes matching top-level imports" intends to eliminate.Consider also checking trailing-slash coverage from
result.Importswhen deciding whether a scope entry is redundant, e.g.:♻️ Suggested extension
- if result.Imports != nil { - deduplicated := make(map[string]string) - for key, value := range simplified { - if topLevel, ok := result.Imports[key]; !ok || topLevel != value { - deduplicated[key] = value - } - } - simplified = deduplicated - } + if result.Imports != nil { + deduplicated := make(map[string]string) + for key, value := range simplified { + if topLevel, ok := result.Imports[key]; ok && topLevel == value { + continue // exact match with top-level + } + covered := false + for tlKey, tlValue := range result.Imports { + if !strings.HasSuffix(tlKey, "/") { + continue + } + if rel, ok := strings.CutPrefix(key, tlKey); ok && tlValue+rel == value { + covered = true + break + } + } + if !covered { + deduplicated[key] = value + } + } + simplified = deduplicated + }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inject/inject_test.go`:
- Around line 245-254: The test currently inlines HTML via
mfs.AddFile("/project/other.html", ...) which violates the convention of using
fixture files; move the inlined content into test fixtures under
testdata/inject/multiple-files/ (create other.html and ensure index.html exists
there) and update the test to use NewFixtureFS to load that fixture instead of
calling mfs.AddFile; adjust any setup that refers to "/project/other.html" to
use the fixture filesystem returned by NewFixtureFS and keep the same file names
(other.html, index.html) so existing test logic (e.g., file lookup) continues to
work.
🪄 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: fc77bbeb-f6a4-4047-9d25-260e7f70b10b
📒 Files selected for processing (14)
cmd/generate/generate.goimportmap/importmap.goinject/inject_test.gopackagejson/packagejson.gopackagejson/packagejson_test.goresolve/local/local.goresolve/template.gotestdata/packagejson/peer-optional-deps/expected.jsontestdata/packagejson/peer-optional-deps/package.jsontestdata/resolve/with-wildcard-scopes/expected.jsontestdata/trace/circular-modules/expected.jsontestdata/trace/extract-reexports/expected.jsontestdata/trace/extract-reexports/module.jstrace/trace_test.go
✅ Files skipped from review due to trivial changes (6)
- testdata/trace/extract-reexports/expected.json
- testdata/trace/circular-modules/expected.json
- testdata/packagejson/peer-optional-deps/package.json
- testdata/packagejson/peer-optional-deps/expected.json
- resolve/local/local.go
- testdata/trace/extract-reexports/module.js
🚧 Files skipped from review as they are similar to previous changes (4)
- testdata/resolve/with-wildcard-scopes/expected.json
- trace/trace_test.go
- packagejson/packagejson.go
- packagejson/packagejson_test.go
- Simplify scope dedup: also remove scope entries covered by top-level trailing-slash keys (e.g., scope "lit/foo.js" is redundant when top-level has "lit/" that resolves to the same URL) - Move inline other.html to testdata/inject/no-importmap/ fixture Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
inject/inject_test.go (1)
211-240:⚠️ Potential issue | 🟡 MinorAssert exactly one result for single-file error cases.
Each test passes one file to
InjectBatch, butcount == 0still allows duplicate error results to pass. Match the dry-run cardinality check and require exactly one result.Proposed test hardening
- if count == 0 { - t.Fatal("Expected at least one result from InjectBatch") + if count != 1 { + t.Fatalf("Expected 1 result, got %d", count) }- if count == 0 { - t.Fatal("Expected at least one result from InjectBatch") + if count != 1 { + t.Fatalf("Expected 1 result, got %d", count) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inject/inject_test.go` around lines 211 - 240, The tests TestInjectBatch_MissingFile (and the similar invalid-variable test) call InjectBatch with a single file but only assert count != 0; change the cardinality assertion to require exactly one result. After iterating over results (variable results) and incrementing count, replace the final check if count == 0 { t.Fatal(...) } with an assertion that count == 1 (e.g. if count != 1 { t.Fatalf("expected exactly one result, got %d", count) }) so duplicate/multiple results fail the test while preserving the existing per-result assertions on r.Error and r.File.
🧹 Nitpick comments (1)
inject/inject_test.go (1)
42-91: Compare generated HTML against golden fixtures.These tests only assert substrings, so malformed placement, duplicated tags, indentation regressions, or unexpected extra content could still pass. Since the inputs are fixture-backed, add expected-output goldens and compare
resultexactly.As per coding guidelines: “Practice TDD and use the fixture/golden patterns for testing with fixtures in
testdata/directories and goldens as expected output files”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inject/inject_test.go` around lines 42 - 91, Replace the substring assertions with exact golden file comparisons: for both test cases (the first TestBuildNewContent_* that calls buildNewContent and TestBuildNewContent_ReplaceExisting) write the expected output to golden files in testdata (e.g., testdata/build-new-importmap.golden and testdata/build-replace-existing.golden) and update the tests to read those goldens and compare the full string(bytes) of result to the golden contents (use the existing mfs/testutil.NewFixtureFS pattern), failing the test when they differ; keep using buildNewContent, loc := trace.FindImportMapTag(html), and the inserted boolean for behavior assertions but replace all substrings checks on result with an exact equality check against the corresponding golden file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@importmap/importmap.go`:
- Around line 44-63: The Generate flow never calls ImportMap.Validate(), so
validation errors are never surfaced; update the command handler in
cmd/generate/generate.go to call im := importMap.Simplify(); errs :=
im.Validate() immediately after Simplify(), log each ValidationError (use its
Key and Message) as non-fatal warnings, and add a --strict boolean flag to the
generate command that causes the handler to exit non-zero when errs is
non-empty; ensure you reference ImportMap.Validate, ValidationError, Simplify,
and the generate command's main handler when implementing these changes.
- Around line 53-61: The scope-key validation currently creates a
ValidationError with Key set to the invalid scope and an empty Scope, which
mislabels scope-prefix errors as top-level key errors; update the loop over
im.Scopes (the block that appends to errs) to populate the ValidationError.Scope
field with the failing scope (and clear or mirror Key as desired) instead of
leaving Scope empty, and also add a small branch in the ValidationError.Error()
method to format messages when Scope != "" && Key == "" (e.g., "scope %q: ...")
so the formatted message and fields consistently indicate a malformed scope
prefix.
---
Duplicate comments:
In `@inject/inject_test.go`:
- Around line 211-240: The tests TestInjectBatch_MissingFile (and the similar
invalid-variable test) call InjectBatch with a single file but only assert count
!= 0; change the cardinality assertion to require exactly one result. After
iterating over results (variable results) and incrementing count, replace the
final check if count == 0 { t.Fatal(...) } with an assertion that count == 1
(e.g. if count != 1 { t.Fatalf("expected exactly one result, got %d", count) })
so duplicate/multiple results fail the test while preserving the existing
per-result assertions on r.Error and r.File.
---
Nitpick comments:
In `@inject/inject_test.go`:
- Around line 42-91: Replace the substring assertions with exact golden file
comparisons: for both test cases (the first TestBuildNewContent_* that calls
buildNewContent and TestBuildNewContent_ReplaceExisting) write the expected
output to golden files in testdata (e.g., testdata/build-new-importmap.golden
and testdata/build-replace-existing.golden) and update the tests to read those
goldens and compare the full string(bytes) of result to the golden contents (use
the existing mfs/testutil.NewFixtureFS pattern), failing the test when they
differ; keep using buildNewContent, loc := trace.FindImportMapTag(html), and the
inserted boolean for behavior assertions but replace all substrings checks on
result with an exact equality check against the corresponding golden file.
🪄 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: bdd67e89-da50-411a-8b93-c0f9e69dfdb9
📒 Files selected for processing (3)
importmap/importmap.goinject/inject_test.gotestdata/inject/no-importmap/other.html
✅ Files skipped from review due to trivial changes (1)
- testdata/inject/no-importmap/other.html
- generate: call Validate() after Simplify(), log warnings to stderr, add --strict flag to exit non-zero on validation warnings - ValidationError: populate Scope (not Key) for scope-prefix errors, add Error() format for scope-only errors - inject tests: replace substring checks with golden file comparisons for buildNewContent, tighten count assertions to exactly 1 - inject tests: move inline other.html to fixture Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/generate/generate.go (1)
140-147: Prefercmd.ErrOrStderr()over directos.Stderrwrites.Writing directly to
os.Stderrbypasses cobra's output streams, making it harder to capture validation warnings in tests or when the command is embedded. Usingcmd.ErrOrStderr()is the idiomatic cobra pattern and keeps output configurable.♻️ Proposed refactor
- if errs := outputMap.Validate(); len(errs) > 0 { - for _, e := range errs { - fmt.Fprintf(os.Stderr, "Warning: %s\n", e) - } - if viper.GetBool("strict") { - return fmt.Errorf("import map has %d validation warning(s)", len(errs)) - } - } + if errs := outputMap.Validate(); len(errs) > 0 { + for _, e := range errs { + fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", e) + } + if viper.GetBool("strict") { + return fmt.Errorf("import map has %d validation warning(s)", len(errs)) + } + }The
osimport can then be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generate/generate.go` around lines 140 - 147, Replace direct writes to os.Stderr in the outputMap validation block with Cobra's configurable error writer: use cmd.ErrOrStderr() as the io.Writer when calling fmt.Fprintf (inside the block where outputMap.Validate() is checked), so warnings are emitted via Cobra's streams and tests can capture them; keep the existing behavior of returning an error when viper.GetBool("strict") is true. Also remove the now-unused os import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/generate/generate.go`:
- Around line 140-147: Replace direct writes to os.Stderr in the outputMap
validation block with Cobra's configurable error writer: use cmd.ErrOrStderr()
as the io.Writer when calling fmt.Fprintf (inside the block where
outputMap.Validate() is checked), so warnings are emitted via Cobra's streams
and tests can capture them; keep the existing behavior of returning an error
when viper.GetBool("strict") is true. Also remove the now-unused os import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1754c8-224e-42ea-bcd6-d58ff2bca8a7
📒 Files selected for processing (6)
cmd/generate/generate.goimportmap/importmap.goimportmap/importmap_test.goinject/inject_test.gotestdata/inject/build-insert-new/expected.htmltestdata/inject/build-replace-existing/expected.html
✅ Files skipped from review due to trivial changes (3)
- testdata/inject/build-replace-existing/expected.html
- testdata/inject/build-insert-new/expected.html
- importmap/importmap_test.go
Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- processPackageDependenciesParallelWithGraph: pass the nested dep's parent node_modules for recursive calls so transitive deps of nested copies are found at the correct location - ResolveSpecifiers: use ImportMapEntries for trailing-slash key generation, fixing non-pure wildcards being emitted as folder mappings Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#importssubpath resolution, peer/optional dependency parsingValidate()to importmap package for trailing-slash and URL format checksnode_modulesbefore falling back to hoisted--optimizeflag: configurable optimization level ongeneratecommandChanges
packagejson
resolveExportValueWithOpts: handlenil(null targets) and[]any(fallback arrays)resolveConditionsWithOpts: handle null condition values and array fallbacksResolveImport()for#importssubpath resolution (with wildcards, conditions, null)PeerDependenciesandOptionalDependenciesfieldsimportmap
Validate()method withValidationErrortypeSimplifyEntries()for scope-level useSimplify()now deduplicates scope entries matching top-level importsresolve/local
SimplifyEntrieson scope entries before mergingfindDependencyPath()for nested node_modules resolutioncmd/generate
--optimize/-Oflag (0=none, 1=simplify, default=1)Test coverage (+25 new tests, 316 total)
#importsresolution, peer/optional deps, malformed JSONTest plan
make lintpasses (0 issues)make testpasses (316 tests, race detector enabled)Summary by CodeRabbit
New Features
Tests