Skip to content

fix(pm): avoid reusing anchor-only hoists for workspaces#2819

Open
killagu wants to merge 1 commit intonextfrom
fix/workspace-hoist-reachability
Open

fix(pm): avoid reusing anchor-only hoists for workspaces#2819
killagu wants to merge 1 commit intonextfrom
fix/workspace-hoist-reachability

Conversation

@killagu
Copy link
Copy Markdown
Contributor

@killagu killagu commented Apr 21, 2026

Summary

  • resolve dependency reuse from the requester path instead of blindly starting at the graph parent
  • only reuse hoisted packages from node_modules directories that are actually reachable by Node upward resolution
  • add regression tests for nested-preferred reuse and sibling workspace hoist isolation

Validation

  • cargo +nightly-2026-02-05 fmt --check
  • cargo +nightly-2026-02-05 test -p utoo-ruborist test_find_compatible_node_ -- --nocapture

Reproduction

  • reproduced on elrrrrrrr/lobehub branch ut-ci-test after bypassing an unrelated HTTP tarball blocker
  • confirmed apps/desktop/node_modules/xlsx existed while packages/file-loaders could not resolve xlsx from its own directory

Copilot AI review requested due to automatic review settings April 21, 2026 15:07
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the dependency resolution logic in DependencyGraph to ensure that node_modules directories are only considered if they are physically reachable from the requester's path, preventing incorrect reuse of hoisted dependencies in workspace scenarios. The changes include a new reachability check and updated recursion logic in find_in_parent_chain. Feedback suggests optimizing the reachability check by passing the requester's path through the recursion to avoid redundant node lookups.

Comment thread crates/ruborist/src/model/graph.rs
Copy link
Copy Markdown
Contributor

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 adjusts dependency reuse/hoisting decisions in ruborist’s dependency graph to ensure previously-installed packages are only reused when their node_modules/ locations are actually reachable from the requesting package’s on-disk path (matching Node’s upward module resolution behavior), and adds regressions around nested-vs-hoisted preference and sibling workspace isolation.

Changes:

  • Start find_compatible_node search from the requester node (preferring nested installs before hoisted parents).
  • Gate reuse checks to only consider ancestor node_modules/ locations that are path-reachable from the requester.
  • Add regression tests for nested preference and sibling-workspace hoist isolation.

Comment thread crates/ruborist/src/model/graph.rs Outdated
Comment thread crates/ruborist/src/model/graph.rs
@killagu
Copy link
Copy Markdown
Contributor Author

killagu commented Apr 21, 2026

Addressed the inline review comments:

  • carry a normalized requester path through find_in_parent_chain instead of repeatedly looking it up by node index
  • normalize both requester/candidate paths lexically before the reachability check so dot-dot segments cannot produce false reachable results
  • switch the sibling-workspace regression test to platform-agnostic relative paths

Revalidated with:

  • cargo +nightly-2026-02-05 fmt --check
  • cargo +nightly-2026-02-05 test -p utoo-ruborist test_find_compatible_node_ -- --nocapture

@killagu killagu changed the title Fix dependency reachability for sibling workspaces fix(pm): avoid reusing anchor-only hoists for workspaces Apr 21, 2026
@killagu killagu force-pushed the fix/workspace-hoist-reachability branch from f8f5044 to 1b7a778 Compare April 21, 2026 16:28
@elrrrrrrr
Copy link
Copy Markdown
Contributor

@killagu 需要补充一下原始 issue 里的 e2e case

@killagu killagu force-pushed the fix/workspace-hoist-reachability branch from 1b7a778 to eb67777 Compare April 22, 2026 06:26
@elrrrrrrr
Copy link
Copy Markdown
Contributor

#2811

原始 issue 在这,pr 里应该保持关联

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants