Skip to content

fix: take context.cwd into account for cache key #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 19, 2025
Merged

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Jun 18, 2025

close #370

related microsoft/vscode-eslint#1994

process.cwd()/context.cwd can be changed with process.cwd('new path')


Important

Fix cache key generation by including context.cwd to improve cache accuracy and reduce collisions.

  • Bug Fix:
    • Include context.cwd in cache key generation in export-map.ts and module-cache.ts to improve cache accuracy and reduce collisions.
  • Refactor:
    • Replace module.isBuiltin with isBuiltin in node-resolver.ts.
    • Update CacheKey type to string in module-cache.ts.
    • Add NormalizedCacheSettings interface in types.ts for cache settings.
  • Tests:
    • Update tests in resolve.spec.ts to validate cache key changes and ensure correct module resolution behavior.

This description was created by Ellipsis for 8311c98. You can customize this summary. It will automatically update as commits are pushed.


Summary by CodeRabbit

  • Bug Fixes

    • Improved cache accuracy by including the current working directory in cache key generation, reducing potential cache collisions.
  • Refactor

    • Updated and clarified cache-related settings and types for improved consistency and reliability.
    • Enhanced internal logic for module resolution and cache management to streamline performance and maintainability.
  • Chores

    • Added documentation for a patch update to the package.

@JounQin JounQin requested a review from Copilot June 18, 2025 16:58
@JounQin JounQin self-assigned this Jun 18, 2025
@JounQin JounQin added the bug Something isn't working label Jun 18, 2025
Copy link

changeset-bot bot commented Jun 18, 2025

🦋 Changeset detected

Latest commit: 8311c98

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-import-x Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codacy-production bot commented Jun 18, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.05% (target: -1.00%) 100.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (3ea730f) 3720 3557 95.62%
Head commit (8311c98) 3717 (-3) 3556 (-1) 95.67% (+0.05%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#388) 12 12 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Copy link

coderabbitai bot commented Jun 18, 2025

Walkthrough

This change refactors the caching mechanism in the eslint-plugin-import-x package to ensure the current working directory is included in cache key generation. It introduces a new NormalizedCacheSettings interface, updates cache handling logic, and modifies function signatures and internal logic for improved cache accuracy, especially in monorepo environments.

Changes

Files/Group Change Summary
.changeset/major-apples-invent.md Added a changeset documenting the patch update and cache key fix.
src/node-resolver.ts Refactored built-in module check, streamlined error handling, and updated resolver usage.
src/types.ts Introduced NormalizedCacheSettings interface and updated imports for type usage.
src/utils/export-map.ts Modified childContext and makeContextCacheKey to include cwd in cache key with null character separators.
src/utils/module-cache.ts Refactored ModuleCache to use string keys, updated methods to use NormalizedCacheSettings, improved logging.
src/utils/resolve.ts Updated function signatures to require NormalizedCacheSettings for cache settings.
test/utils/resolve.spec.ts Added type assertion for cacheSettings to use NormalizedCacheSettings in tests.

Sequence Diagram(s)

sequenceDiagram
    participant ESLint
    participant ImportXPlugin
    participant ModuleCache
    participant FileSystem

    ESLint->>ImportXPlugin: Lint file (with context)
    ImportXPlugin->>ModuleCache: Generate cache key (includes cwd)
    ModuleCache->>ModuleCache: Check cache for key
    alt Cache hit and fresh
        ModuleCache-->>ImportXPlugin: Return cached result
    else Cache miss or stale
        ImportXPlugin->>FileSystem: Resolve module/import
        FileSystem-->>ImportXPlugin: Resolution result
        ImportXPlugin->>ModuleCache: Set cache entry
    end
    ImportXPlugin-->>ESLint: Linting result
Loading

Assessment against linked issues

Objective Addressed Explanation
Fix false positives for import-x/no-unresolved in monorepo setups with multiple packages open (#370)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes found.

Possibly related PRs

Suggested labels

refactor

Poem

In the warren of code, a cache key anew,
Now the cwd joins the clever crew!
No more monorepo woes or import-y despair,
With nulls as separators, collisions are rare.
The bunny hops on, with cache fresh and bright—
Linting your imports, from morning to night!
🐇✨

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/node-resolver.ts

Oops! Something went wrong! :(

ESLint: 9.29.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
at finalizeResolution (node:internal/modules/esm/resolve:274:11)
at moduleResolve (node:internal/modules/esm/resolve:859:10)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:801:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:725:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:708:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:309:38)
at #link (node:internal/modules/esm/module_job:201:49)

src/utils/module-cache.ts

Oops! Something went wrong! :(

ESLint: 9.29.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
at finalizeResolution (node:internal/modules/esm/resolve:274:11)
at moduleResolve (node:internal/modules/esm/resolve:859:10)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:801:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:725:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:708:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:309:38)
at #link (node:internal/modules/esm/module_job:201:49)

test/utils/resolve.spec.ts

Oops! Something went wrong! :(

ESLint: 9.29.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
at finalizeResolution (node:internal/modules/esm/resolve:274:11)
at moduleResolve (node:internal/modules/esm/resolve:859:10)
at defaultResolve (node:internal/modules/esm/resolve:983:11)
at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:801:12)
at #cachedDefaultResolve (node:internal/modules/esm/loader:725:25)
at ModuleLoader.resolve (node:internal/modules/esm/loader:708:38)
at ModuleLoader.getModuleJobForImport (node:internal/modules/esm/loader:309:38)
at #link (node:internal/modules/esm/module_job:201:49)


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f88d289 and 8311c98.

📒 Files selected for processing (4)
  • .changeset/major-apples-invent.md (1 hunks)
  • src/node-resolver.ts (3 hunks)
  • src/utils/module-cache.ts (3 hunks)
  • test/utils/resolve.spec.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • .changeset/major-apples-invent.md
  • test/utils/resolve.spec.ts
  • src/node-resolver.ts
  • src/utils/module-cache.ts
⏰ Context from checks skipped due to timeout of 90000ms (21)
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
  • GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
  • GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on macos-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
  • GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
  • GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
  • GitHub Check: Codacy Static Code Analysis
✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codesandbox-ci bot commented Jun 18, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes cache key generation by incorporating the current working directory (process.cwd()) and improves the handling of cache settings and resolver instantiation. Key changes include:

  • Updating type definitions to use NormalizedCacheSettings instead of the previous ImportSettings['cache'].
  • Changing the cache key from an unknown type to a string and ensuring stale cache handling.
  • Adjusting cache key generation in export-map.ts to include process.cwd(), and refactoring the node resolver to use a singleton instance.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/utils/resolve.ts Updated cacheSettings parameter type to NormalizedCacheSettings.
src/utils/module-cache.ts Changed cache key type to string, and added cache clear on miss.
src/utils/export-map.ts Prepending process.cwd() to the context cache key for accuracy.
src/types.ts Added NormalizedCacheSettings interface to support new cache keys.
src/node-resolver.ts Refactored resolver instance handling; improved builtin check.
.changeset/*.md Updated changeset notes to reflect the fix and performance update.
Comments suppressed due to low confidence (1)

src/utils/export-map.ts:1135

  • [nitpick] Adding a comment to explain the inclusion of process.cwd() in the cache key could help future maintainers understand its impact on cache behavior.
    process.cwd() +

Copy link

codecov bot commented Jun 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.09%. Comparing base (3ea730f) to head (8311c98).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   96.05%   96.09%   +0.03%     
==========================================
  Files          96       96              
  Lines        4947     4944       -3     
  Branches     1855     1831      -24     
==========================================
- Hits         4752     4751       -1     
+ Misses        194      192       -2     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 73d1422 in 1 minute and 14 seconds. Click for details.
  • Reviewed 273 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/huge-streets-fold.md:1
  • Draft comment:
    Changeset message is clear; no issues here.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. .changeset/major-apples-invent.md:1
  • Draft comment:
    Changeset message clearly states the fix for including process.cwd() in the cache key.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. src/node-resolver.ts:39
  • Draft comment:
    Using 'isBuiltin' from 'node:module' increases clarity over the previous 'module.isBuiltin' check.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. src/types.ts:159
  • Draft comment:
    The new NormalizedCacheSettings interface properly extends cache settings with a 'lifetime' field.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. src/utils/export-map.ts:1118
  • Draft comment:
    Child context now explicitly extracts 'cwd', 'filename', and 'physicalFilename' for consistency.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
6. src/utils/export-map.ts:1135
  • Draft comment:
    Including process.cwd() at the beginning of the cache key addresses #370. Consider if frequent cwd changes could lead to cache thrashing.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
7. src/utils/module-cache.ts:29
  • Draft comment:
    Consider clearing expired cache entries from the map to prevent potential memory buildup in long-running processes.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
8. src/utils/resolve.ts:246
  • Draft comment:
    The cache key in fullResolve now combines the source directory, child context hash (which includes process.cwd()), and settings hash. Adding inline comments here could aid future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_QB9vzbT7oBTobVDi

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

pkg-pr-new bot commented Jun 18, 2025

Open in StackBlitz

npm i https://pkg.pr.new/eslint-plugin-import-x@388

commit: 8311c98

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed ac417f7 in 2 minutes and 15 seconds. Click for details.
  • Reviewed 269 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/node-resolver.ts:48
  • Draft comment:
    Consider logging or otherwise handling errors in the catch block rather than silently swallowing them; this will aid in debugging resolver failures.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a module resolver used by eslint-plugin-import. Failed resolutions are expected and common - they happen whenever a module isn't found. Logging errors would create noise in the logs. The function correctly handles the failure case by returning { found: false }. The empty catch block is actually the right approach here since this is part of the intended control flow. Maybe error logging could be useful for debugging genuine resolver issues vs expected "module not found" cases? The comment suggests it would aid debugging. In a resolver, failed resolution is a normal and expected outcome, not an error condition. Adding logging would create unnecessary noise that would make actual issues harder to debug. The comment should be deleted. The empty catch block is the correct implementation for this resolver pattern where failed resolution is an expected outcome.
2. src/types.ts:167
  • Draft comment:
    Add documentation (e.g. clear JsDoc comments) for the NormalizedCacheSettings interface and its 'lifetime' property to improve maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. src/utils/export-map.ts:1135
  • Draft comment:
    Including process.cwd() in the cache key is effective for isolating caches across working directories; however, ensure that dynamic changes to process.cwd() don’t lead to unintended cache invalidation.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is informative and suggests ensuring that dynamic changes to process.cwd() don't lead to unintended cache invalidation. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't confirm if the behavior is intentional or not. Therefore, it violates the rules.
4. src/utils/module-cache.ts:36
  • Draft comment:
    Calling clearNodeResolverCache() within the get() method introduces a side-effect that clears the resolver cache for all modules; ensure that this behavior is intentional and document it accordingly.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is asking for documentation of behavior, but not pointing out an actual code issue. The side effect seems intentional as it's being added in this PR. The comment is essentially asking "are you sure?" which violates our rules. Documentation requests should be handled through other channels. The side effect could be important enough to warrant documentation. Clearing a global cache is a significant operation that affects other parts of the system. While the side effect is significant, the comment is still essentially asking for confirmation and documentation rather than pointing out a concrete issue that needs fixing. Per our rules, we should not ask authors to confirm their intentions. Delete the comment as it violates our rule about not asking authors to confirm their intentions or add documentation. The comment is not pointing out a concrete code issue that needs fixing.
5. src/utils/resolve.ts:247
  • Draft comment:
    The composed cache key now includes the working directory (via childContextHashKey) and a settings hash. Verify that changes to process.cwd() during runtime do not inadvertently invalidate valid cache entries.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/utils/resolve.ts:62
  • Draft comment:
    Using toLowerCase() to compare filepath and process.cwd() might not handle locale-specific or normalization issues; consider using a robust path normalization method for accuracy on case-insensitive file systems.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_4TiNIW42UfCnowQr

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed a4d7312 in 1 minute and 37 seconds. Click for details.
  • Reviewed 273 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/node-resolver.ts:29
  • Draft comment:
    Using a global singleton with cloneWithOptions is a neat performance optimization. Ensure that this state is acceptable and doesn’t lead to unintended side‐effects if options vary between calls.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the PR author to ensure that the state doesn't lead to unintended side-effects, which violates the rule against asking the author to ensure behavior is intended. However, it does provide a specific observation about the use of a global singleton with cloneWithOptions, which could be useful. The comment could be rephrased to focus on the potential issue without asking the author to ensure anything.
2. src/node-resolver.ts:44
  • Draft comment:
    The non-null assertion (resolver!) is used assuming it’s always initialized. Consider a safeguard or comment explaining why this is safe.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The non-null assertion is actually safe here because resolver is always initialized in createNodeResolver before the returned object's resolve method can be called. The code structure makes this fairly clear. The suggestion to add a comment doesn't really improve code quality since the initialization is obvious from reading the code. The comment does point out a potential code safety issue that future maintainers might question. TypeScript's type system can't prove the initialization is safe. While true, the initialization is clear from the code structure and adding a comment would be redundant given how close the initialization is to the usage. Delete the comment as it suggests adding unnecessary documentation for something that is clear from the code structure.
3. src/utils/export-map.ts:1134
  • Draft comment:
    In makeContextCacheKey, process.cwd() is prepended to the cache key. If the context provides its own cwd, consider using it instead for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Looking at the code: 1. The function takes a context parameter that includes cwd 2. Earlier in the file, cwd is destructured from context and used 3. Using context.cwd would be more consistent with how other context properties are used 4. However, this is a cache key - using process.cwd() may be intentional to ensure consistent caching across different context objects 5. Without understanding the caching requirements fully, changing this could introduce subtle bugs The comment makes a reasonable suggestion for consistency, but changing cache key generation could have unintended consequences. We don't have enough context about the caching requirements. While consistency is good, cache key generation needs to be stable and predictable. Without strong evidence that using context.cwd is safe for caching, we should be conservative. The comment should be removed. While it makes a reasonable suggestion, changing cache key generation is risky without fully understanding the caching requirements.
4. src/utils/module-cache.ts:40
  • Draft comment:
    Clearing the node resolver cache on every cache miss may be too aggressive. Please verify that this behavior is intended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify their intention, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
5. src/utils/resolve.ts:246
  • Draft comment:
    The cache key now incorporates process.cwd() and settings hash. Confirm that this change properly invalidates the cache when the working directory changes.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_fxDJlrrPJ5IEb1Kf

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 102c661 in 51 seconds. Click for details.
  • Reviewed 269 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/huge-streets-fold.md:1
  • Draft comment:
    The performance changeset is well documented and clearly states the use of a singleton resolver instance.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. .changeset/major-apples-invent.md:1
  • Draft comment:
    The changelog clearly documents the fix to include process.cwd() for the cache key, addressing issue #370.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. src/node-resolver.ts:12
  • Draft comment:
    The clearNodeResolverCache function using optional chaining is well implemented for resetting the resolver cache.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. src/node-resolver.ts:29
  • Draft comment:
    The singleton pattern for the resolver is applied nicely by cloning with new options or creating a new instance; the non-null assertion (resolver!) at line 44 appears safe given the flow.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. src/types.ts:167
  • Draft comment:
    Adding the NormalizedCacheSettings interface improves type safety by explicitly requiring a 'lifetime' property in the cache settings.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
6. src/utils/export-map.ts:1135
  • Draft comment:
    Including process.cwd() in the cache key generation is a key improvement that should prevent false cache hits in a monorepo environment where the working directory may change.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining why a change is beneficial. It does not suggest any code changes or ask for confirmation on specific intentions. According to the rules, purely informative comments should be removed.
7. src/utils/module-cache.ts:36
  • Draft comment:
    Calling clearNodeResolverCache on a cache miss ensures stale resolver entries are cleared. Confirm that this side effect on every miss is the intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
8. src/utils/resolve.ts:237
  • Draft comment:
    The fullResolve function now builds a cache key incorporating the child context hash and a memoized hash of settings (which includes process.cwd() via makeContextCacheKey), thus addressing cache collisions in multi-package monorepos.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_wBtvySSPzuWeXimP

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed f947d64 in 49 seconds. Click for details.
  • Reviewed 17 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/utils/export-map.ts:1132
  • Draft comment:
    Consider providing a fallback for 'cwd' (e.g. using cwd || process.cwd()) to ensure the cache key is valid even if cwd isn’t set in the context.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_fFguvKKlBOuvcy7J

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@JounQin JounQin changed the title fix: take process.cwd() into account for cache key fix: take context.cwd into account for cache key Jun 19, 2025
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 779aa36 in 56 seconds. Click for details.
  • Reviewed 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/utils/export-map.ts:1119
  • Draft comment:
    Using '\0' as an explicit separator in the cache key concatenation avoids accidental collisions between context and path parts. Ensure that none of the components ever contain the null character.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 20% <= threshold 50% The comment is informative and suggests ensuring that none of the components contain the null character. It doesn't ask for a specific code change or test, nor does it suggest an alternative approach. It seems to be more of a cautionary note rather than a direct actionable comment.
2. src/utils/export-map.ts:1131
  • Draft comment:
    Replacing process.cwd() with the passed-in 'cwd' from context ensures the cache key reflects the actual working directory intended by the caller. Verify that all contexts provide a valid 'cwd' value.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to verify that all contexts provide a valid 'cwd' value, which is against the rules as it asks for verification. It does not provide a specific code suggestion or point out a specific issue with the code.

Workflow ID: wflow_JTHrAwfAe1a0smIn

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed cd30d0b in 1 minute and 17 seconds. Click for details.
  • Reviewed 64 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/huge-streets-fold.md:1
  • Draft comment:
    Removed changeset content indicating a singleton resolver. Ensure that the release notes and commit message accurately reflect that a fresh resolver instance is now created (to handle cwd changes) instead of using a global singleton.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. src/node-resolver.ts:33
  • Draft comment:
    Since 'resolver' is locally defined and always set, the non-null assertion (!) in 'resolver!.sync(...)' is unnecessary.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. src/utils/module-cache.ts:33
  • Draft comment:
    Removing the call to clearNodeResolverCache() on cache miss is appropriate now that resolver creation respects the current cwd, but verify that cache invalidation in cases of cwd change is properly handled elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_62w4eYOpm45ZEpSp

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed f88d289 in 1 minute and 36 seconds. Click for details.
  • Reviewed 101 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/node-resolver.ts:15
  • Draft comment:
    Refactored to use a local ResolverFactory instance. Ensure that any context.cwd passed in restOptions is used to compute unique cache keys.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. src/utils/module-cache.ts:33
  • Draft comment:
    Removed clearNodeResolverCache call on cache miss. Verify that cache invalidation is handled elsewhere without relying on global state.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to verify that cache invalidation is handled elsewhere, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
3. test/utils/resolve.spec.ts:454
  • Draft comment:
    Explicitly casting 'cacheSettings' as NormalizedCacheSettings improves type safety.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_deYtcWwkiEJqIQ4L

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 8311c98 in 1 minute and 30 seconds. Click for details.
  • Reviewed 111 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/major-apples-invent.md:5
  • Draft comment:
    Updated changeset message now reflects use of context.cwd. Ensure documentation and tests align with this change.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that documentation and tests align with the change, which violates the rule against asking the author to ensure things are tested or documented. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. src/node-resolver.ts:15
  • Draft comment:
    Refactored createNodeResolver to use a local resolver instance instead of a global mutable one. Confirm that this does not unintentionally bypass shared caching logic.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking for verification about caching behavior, which violates our rules about not asking authors to verify/confirm things. Additionally, the removal of global state in favor of local instances is generally a good practice. The old code's caching was likely an implementation detail rather than a required feature, since it was marked @internal. Maybe the caching was actually an important performance optimization that shouldn't be removed without careful consideration? The @internal annotation and the fact that the cache clearing function was removed suggests this was not a critical feature. If performance becomes an issue, it can be addressed later with metrics. Delete this comment as it asks for verification and makes speculative concerns about caching without clear evidence of an actual problem.
3. src/utils/module-cache.ts:30
  • Draft comment:
    Removed call to clearNodeResolverCache on cache miss. Verify that cache invalidation now properly factors in context.cwd so that stale resolver caches aren’t retained.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. test/utils/resolve.spec.ts:457
  • Draft comment:
    Added explicit type assertion for cache settings improves type safety. This helps ensure that cache keys include the appropriate context.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining the benefits of a change without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments in a pull request review.

Workflow ID: wflow_xoTwzUsPNhaPy1ec

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@JounQin JounQin merged commit 1089d9f into master Jun 19, 2025
92 checks passed
@JounQin JounQin deleted the fix/cache_key branch June 19, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(VSCode) False positive of rule import-x/no-unresolved in monorepo with multiple packages open
3 participants