fix(trace): output imports and scopes in single and batch modes

[bennypowers]

Fixes #20
Fixes #15

Summary by CodeRabbit

• Bug Fixes

  • Fixed resolution of deep package imports not listed in package exports so traced imports now include nested paths reliably in single and batch tracing.

• Refactor

  • Tracing now uses targeted per-specifier resolution instead of post-resolution filtering, improving accuracy and consistency across modes.

• Tests

  • Added tests covering deep import resolution for single-file and batch tracing scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds Resolver.ResolveSpecifiers and changes tracing to call it directly for traced bare specifiers. Single-file and batch trace paths now use a shared baseResolver to produce per-file imports via ResolveSpecifiers while still deriving scopes from a full resolve.

Changes

Cohort / File(s)	Summary
CLI / Tracer main.go	Replaces per-file post-resolution filtering with a shared baseResolver and direct calls to baseResolver.ResolveSpecifiers(workspaceRoot, bareSpecs) in single-file and batch trace flows; updates traceFile signature to accept baseResolver.
Resolver API resolve/local/local.go	Adds func (r *Resolver) ResolveSpecifiers(rootDir string, specifiers []string) map[string]string which resolves each bare specifier to a URL using exports if available, otherwise falls back to subpath/main/index mapping and applies the configured template.
Tests main_test.go	Adds TestTraceDeepImportNotInExports and TestTraceBatchDeepImportNotInExports to assert deep imports not listed in package exports are present in per-file imports (JSON/NDJSON outputs).
Test Fixtures testdata/trace/unresolvable/* testdata/trace/unresolvable/package.json, .../page.html, .../page2.html	New test package.json and two HTML pages importing a deep path @example/core/button/button.js to exercise unexposed deep-import resolution in single and batch runs.

Sequence Diagram(s)

sequenceDiagram
    participant CLI as mappa trace (main)
    participant Worker as traceFile / worker
    participant Parser as HTML parser (bareSpec discovery)
    participant BaseR as baseResolver (Resolver)
    participant FullR as full resolve (for scopes)
    CLI->>Worker: start trace (single or batch)
    Worker->>Parser: parse file -> bareSpecs
    Worker->>BaseR: ResolveSpecifiers(workspaceRoot, bareSpecs)
    BaseR-->>Worker: map[baredSpecifier]=resolvedURL (imports)
    Worker->>FullR: perform full resolve (to collect scopes)
    FullR-->>Worker: scopes map
    Worker->>CLI: emit result {file, imports, scopes}

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(trace): output import maps #18: Modifies trace import-map generation and uses post-resolution filtering; contrasts with this PR's addition of ResolveSpecifiers to avoid post-filtering.
• feat(trace): batch mode #19: Adds batch tracing paths and per-file tracing helpers that this PR adapts to accept a shared baseResolver and call ResolveSpecifiers.
• feat: incremental updates #14: Also extends the resolver surface; related at the resolver API and resolution-behavior level.

Poem

🐰
I hop through specs both near and far,
Directly fetching each little star,
No extra filters in my way,
Deep imports found — hooray hooray! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: enabling output of imports and scopes in both single-file and batch trace modes, which directly addresses the linked issues.
Linked Issues check	✅ Passed	The PR implements core requirements from both issues: ResolveSpecifiers method resolves bare specs to URLs using templates [#15], and batch glob mode now outputs imports/scopes correctly [#20].
Out of Scope Changes check	✅ Passed	All changes directly support resolving bare specifiers to import maps: the new ResolveSpecifiers method, refactored tracing logic, and comprehensive test coverage for deep imports.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52e3726 and 6ebc587.

📒 Files selected for processing (2)

• main.go
• resolve/local/local.go

🚧 Files skipped from review as they are similar to previous changes (2)

• resolve/local/local.go
• main.go

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

main.go (1)

522-531: Consider reusing resolver across batch workers for efficiency.

Each worker creates a new resolver per file with the same configuration (templateArg, conditions). While this is thread-safe, creating the resolver once and sharing it across workers would be slightly more efficient:

♻️ Optional: Share resolver across workers

In runTraceBatch, create the resolver once before spawning workers:

// In runTraceBatch, before worker creation:
resolver := local.New(osfs, nil)
resolver, err = resolver.WithTemplate(templateArg)
if err != nil {
    return fmt.Errorf("invalid template: %w", err)
}
if len(conditions) > 0 {
    resolver = resolver.WithConditions(conditions)
}

// Pass resolver to traceFile instead of templateArg and conditions

Then update traceFile signature to accept the resolver directly. The Resolver type and its methods appear to be safe for concurrent use since ResolveSpecifiers only reads configuration.

main_test.go (1)

410-438: LGTM – valid regression test for issue #15.

The test correctly verifies that deep imports not listed in package exports are resolved in single-file trace mode. The assertion logic is sound.

Optional: Consider adding an expected.json golden file in testdata/trace/unresolvable/ and using compareOrUpdateGolden for more comprehensive output validation, consistent with TestTrace and TestTraceWithTemplate. This would catch unexpected regressions in other output fields.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ab60721 and 52e3726.

⛔ Files ignored due to path filters (3)

• testdata/trace/unresolvable/node_modules/@example/core/button/button.js is excluded by !**/node_modules/**
• testdata/trace/unresolvable/node_modules/@example/core/index.js is excluded by !**/node_modules/**
• testdata/trace/unresolvable/node_modules/@example/core/package.json is excluded by !**/node_modules/**

📒 Files selected for processing (6)

• main.go
• main_test.go
• resolve/local/local.go
• testdata/trace/unresolvable/package.json
• testdata/trace/unresolvable/page.html
• testdata/trace/unresolvable/page2.html

🧰 Additional context used

📓 Path-based instructions (3)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Getter methods should be named Foo(), not GetFoo()
Run go vet to surface gopls suggestions, including: replace interface{} with any, replace if/else with min, replace m[k]=v loop with maps.Copy, and simplify loops using slices.Contains
Always use the pluggable FileSystem interface: never use os.ReadFile, os.Stat, os.ReadDir, etc. directly; all functions that read from disk must accept a FileSystem parameter
Use NewOSFileSystem() only at the top level (CLI entry point), not throughout the codebase

Files:

• resolve/local/local.go
• main_test.go
• main.go

**/testdata/**

📄 CodeRabbit inference engine (CLAUDE.md)

Each test fixture scenario should be a subdirectory containing required input files (input.json, index.html, module.js, or package.json) and expected.json for assertions

Files:

• testdata/trace/unresolvable/package.json
• testdata/trace/unresolvable/page.html
• testdata/trace/unresolvable/page2.html

**/*_test.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*_test.go: Practice TDD and use fixture/golden patterns for tests: place input test data in testdata/ directories and expected output files (e.g., expected.json) for comparison; tests should support --update flag to regenerate golden files
Always use testutil.NewFixtureFS for tests to load fixtures into MapFileSystem, never use NewOSFileSystem() in tests
For unit tests that don't need fixtures, create an empty MapFileSystem using importmaps.NewMapFileSystem() instead of using the OS filesystem
Don't inline source code in tests; use fixture files instead with input files and expected output in expected.json
Use NewMapFileSystem() and NewFixtureFS() in tests for testability with mock filesystems and integration with cem

Files:

• main_test.go

🧠 Learnings (4)

📚 Learning: 2026-01-14T10:24:30.377Z

Learnt from: CR
Repo: bennypowers/mappa PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T10:24:30.377Z
Learning: Applies to **/*_test.go : Don't inline source code in tests; use fixture files instead with input files and expected output in `expected.json`

Applied to files:

• main_test.go

📚 Learning: 2026-01-14T10:24:30.377Z

Learnt from: CR
Repo: bennypowers/mappa PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T10:24:30.377Z
Learning: Applies to **/*_test.go : Practice TDD and use fixture/golden patterns for tests: place input test data in `testdata/` directories and expected output files (e.g., `expected.json`) for comparison; tests should support `--update` flag to regenerate golden files

Applied to files:

• main_test.go

📚 Learning: 2026-01-14T10:24:30.377Z

Learnt from: CR
Repo: bennypowers/mappa PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T10:24:30.377Z
Learning: Applies to **/*_test.go : Use `NewMapFileSystem()` and `NewFixtureFS()` in tests for testability with mock filesystems and integration with cem

Applied to files:

• main_test.go

📚 Learning: 2026-01-14T10:24:30.377Z

Learnt from: CR
Repo: bennypowers/mappa PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-14T10:24:30.377Z
Learning: Applies to **/*_test.go : For unit tests that don't need fixtures, create an empty MapFileSystem using `importmaps.NewMapFileSystem()` instead of using the OS filesystem

Applied to files:

• main_test.go

🧬 Code graph analysis (2)

resolve/local/local.go (1)

resolve/resolve.go (2)

• Resolver (30-33)
• FindWorkspaceRoot (49-77)

main.go (1)

importmap/importmap.go (1)

• ImportMap (27-37)

🔇 Additional comments (6)

testdata/trace/unresolvable/package.json (1)

1-6: LGTM!

The package manifest correctly declares the @example/core dependency needed for the unresolvable import test scenario.

testdata/trace/unresolvable/page.html (1)

1-10: LGTM!

The test file clearly documents its purpose with an inline comment explaining that this deep import exists as a file but is not in the package's exports. This is a good testing practice that makes the test scenario self-documenting.

main.go (2)

340-366: LGTM - Single trace mode correctly combines Resolve and ResolveSpecifiers.

The approach of using Resolve for scopes (transitive dependencies) while using ResolveSpecifiers for direct imports handles the deep import scenario correctly. This ensures that:

1. Deep imports not in package exports are resolved via ResolveSpecifiers
2. Scopes for transitive dependencies are still computed via full Resolve

---

533-535: This is not an inconsistency—single mode and batch mode use entirely different output types by design.

Single mode (runTraceSingle) outputs a complete importmap.ImportMap (with both Imports and Scopes) for a single file. Batch mode (runTraceBatch → traceFile) outputs traceResult records (one per file) with only File, Imports, Error, and Warnings fields. The traceResult struct was never designed to include scopes—it's intentionally minimal for per-file NDJSON output.

Likely an incorrect or invalid review comment.

testdata/trace/unresolvable/page2.html (1)

1-10: Test fixture correctly structured for programmatic test validation.

The HTML file properly sets up a test case for deep module imports (@example/core/button/button.js). The testdata/trace/unresolvable directory appropriately provides the necessary input files: page.html, page2.html, package.json, and mock node_modules. The test validates output through programmatic assertions on CLI stdout (JSON/NDJSON parsing), not through expected.json comparison, which is the established pattern in this test suite.

main_test.go (1)

440-476: LGTM – valid regression test for issue #20.

The test correctly verifies that batch mode resolves deep imports across multiple HTML files. Fixtures are properly structured with both page.html and page2.html importing @example/core/button/button.js from the mock @example/core package. The NDJSON parsing and validation pattern aligns with existing batch tests.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}