fix(resolve): treat .mjs/.cjs/.mts/.cts as source files for symbol tracing - #85
Conversation
…acing Changed .mts/.cts/.mjs/.cjs files were classified as "assets" instead of source files, so their own project was marked affected but their exports were never traced through the semantic import index — silently dropping downstream consumer projects (false negatives) in strict-ESM monorepos and dual-package (ESM+CJS) libraries. - utils.rs: add the four extensions to SOURCE_EXTENSIONS/is_source_file - semantic/analyzer.rs: collect_file_paths now delegates to is_source_file instead of using its own separate, now-stale hardcoded extension filter - semantic/resolve_options.rs: add the extensions to the oxc_resolver extensions list (TS variants before their JS counterparts, plus .d.mts/.d.cts), and add extension_alias entries so a .mjs/.cjs specifier resolves to its .mts/.cts source (TypeScript's "import with output extension" convention), kept separate from .js/.jsx so ESM- and CJS-explicit specifiers can't cross-resolve into each other - semantic/mod.rs: mirror the same extensions/aliases in simple_resolve_relative, the fallback resolver used when oxc_resolver fails Adds integration tests tracing symbol changes across projects through .mts and .mjs source files (using extension-less imports so the naive asset filename-text-match fallback can't mask a still-broken fix), plus a dedicated test for the .mjs-specifier-resolves-to-.mts-source alias, and unit test coverage for is_source_file with the new extensions.
Closes code-review blocking items on the .mjs/.cjs/.mts/.cts extension support branch: - Add unit tests for the simple_resolve_relative fallback (used only when oxc_resolver fails), mirroring the existing js->ts remapping test: `./utils.mjs` -> on-disk `utils.mts` and `./helper.cjs` -> on-disk `helper.cts`. Verified these fail against main's src/semantic/mod.rs (fallback logic unmodified) and pass against the branch's version. - Add an integration test exercising the real oxc_resolver extension_alias for .cjs -> .cts, mirroring the existing .mjs -> .mts test, including an unrelated third project asserted as NOT affected. - Add the same unrelated-third-project assertion to the existing .mjs -> .mts extension_alias integration test. - Document, on the two extension-less source-tracing tests, that they rely on domino's deliberately-permissive extension-less resolution (real TS under node16/nodenext would require the explicit extension).
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModule discovery and resolution now recognize ChangesStrict extension resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant find_affected
participant WorkspaceAnalyzer
participant Resolver
participant ProjectGraph
find_affected->>WorkspaceAnalyzer: collect source files
WorkspaceAnalyzer-->>find_affected: include MTS, CTS, MJS, and CJS files
find_affected->>Resolver: resolve extension-based imports
Resolver-->>find_affected: map MJS/CJS to MTS/CTS sources
find_affected->>ProjectGraph: mark dependent projects affected
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
📦 Preview Release AvailableA preview release has been published for commit f5bb3f0. Installationnpm install https://github.com/frontops-dev/domino/releases/download/pr-85-f5bb3f0/front-ops-domino-2.0.0.tgzRunning the previewnpx https://github.com/frontops-dev/domino/releases/download/pr-85-f5bb3f0/front-ops-domino-2.0.0.tgz affectedDetails |
…gin/main The ort merge strategy interleaved test_mts_source_file_traced_across_projects (this branch) with test_batch_asset_scan_attributes_references_per_project (origin/main, from #84) into a single malformed function without leaving conflict markers. Restored both functions to their original, independent bodies and ran cargo fmt.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration_test.rs (1)
2441-2577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the common repo-scaffolding between the new extension-resolution tests.
The new
.mts/.mjs/.ctstests all replicate the same TempDir + git flow +Projectconstruction. Reuse an existing per-test helper where it matches, or add a small shared helper parameterized by the project layout and extension pair, so these variants don’t drift apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_test.rs` around lines 2441 - 2577, Share the repeated TempDir setup, git initialization/commit flow, project configuration, and affected-project assertions across the extension-resolution tests at tests/integration_test.rs:2441-2577, 2722-2852, 2853-2977, and 2978-3102. Add or reuse a small helper parameterized for each project layout and extension pair, while preserving each test’s distinct source contents and expected affected projects.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration_test.rs`:
- Around line 2441-2577: Share the repeated TempDir setup, git
initialization/commit flow, project configuration, and affected-project
assertions across the extension-resolution tests at
tests/integration_test.rs:2441-2577, 2722-2852, 2853-2977, and 2978-3102. Add or
reuse a small helper parameterized for each project layout and extension pair,
while preserving each test’s distinct source contents and expected affected
projects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f944cfb-1465-480f-8c9b-e7445a148482
📒 Files selected for processing (7)
CLAUDE.mdsrc/semantic/analyzer.rssrc/semantic/mod.rssrc/semantic/reference_finder.rssrc/semantic/resolve_options.rssrc/utils.rstests/integration_test.rs
2 participants
Problem
Only
ts/tsx/js/jsxwere treated as source files. A changed.mts/.cts/.mjs/.cjsmodule (strict-ESM monorepos, dual-package libs,vite.config.mjs…) was classified as an asset: its own project got marked affected, but its exports were never traced — downstream consumer projects were silently missed. Found in the full repo review (finding #5).Solution
Add the four extensions as first-class source files everywhere. The implementation found four divergent hardcoded extension lists (the review had only found two):
src/utils.rsSOURCE_EXTENSIONS(gates source-vs-asset classification)src/semantic/analyzer.rscollect_file_paths(had its own inlinematches!filter — without this,.mtsfiles were never parsed at all); now delegates to the sharedis_source_filesrc/semantic/resolve_options.rs: resolverextensions(TS variants before their JS counterparts, mirroring the existing.ts-before-.jsprecedent, plus.d.mts/.d.cts) andextension_aliasentries.mjs→[.mts,.mjs],.cjs→[.cts,.cjs] for TypeScript's import-with-output-extension convention — deliberately NOT letting ESM- and CJS-explicit specifiers cross-resolvesrc/semantic/mod.rssimple_resolve_relative(the fallback resolver — a previously-unknown third divergent list) mirrored to stay in syncTesting
TDD: integration tests written first and shown failing (
proj-b should be affected... Got: ["proj-a"]):.mts/.mjstraced across projects,./util.mjs→util.mtsand./helper.cjs→helper.ctsthrough extension_alias, plus unit tests proving the simple-resolve fallback additions are load-bearing (shown red against main's version of that file). All tests assert absence of an unrelated project as well as presence.Second-model review did differential testing against
main: confirmed non-vacuous tests, load-bearing extension_alias, behavior-identicalis_source_fileswap, and disproved a hypothesizedrequire()regression by building the scenario on both revisions.Tallies (
--no-default-features, fixture populated): lib 215; integration 66/67 (failure =test_three_dot_diff_behavior, broken on unmodified main, addressed by #82); cli 15; clippy/fmt clean.Notes
./utilscan resolve toutils.mts(real TS under node16/nodenext wouldn't) — safe direction for affected detection, documented on the tests.d.mts/.d.ctssit last so they only cost on failure pathsPart of the repo-review fix wave (#83, #84).
Summary by CodeRabbit
New Features
.mts,.cts,.mjs, and.cjsfiles.Bug Fixes
.mjs/.cjsimports targeting TypeScript modules.Documentation