Skip to content

Comments

fix(vscode): resolve binary paths with node resolver#17970

Merged
graphite-app[bot] merged 1 commit intomainfrom
01-13-fix_vscode_resolve_binary_paths_with_node_resolver
Jan 18, 2026
Merged

fix(vscode): resolve binary paths with node resolver#17970
graphite-app[bot] merged 1 commit intomainfrom
01-13-fix_vscode_resolve_binary_paths_with_node_resolver

Conversation

@Sysix
Copy link
Member

@Sysix Sysix commented Jan 13, 2026

Refactored the search to use path.resolve instead of glob search with VSCode.
The VSCode snippet did some excludes in the background with search.excludes and/or other settings.
Now it should work without these restriction and hopefully

closes #17936
closes #17881
closes #17744

It will not search for globally installed oxlint/oxfmt. Created an extra issue for this: #18005

Copy link
Member Author

Sysix commented Jan 13, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Sysix Sysix changed the base branch from 01-08-chore_oxlint_napi_add_napi_function_createworkspace_and_destroyworkspace_ to graphite-base/17970 January 13, 2026 20:07
@Sysix Sysix force-pushed the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch from eaca237 to d52e318 Compare January 13, 2026 20:07
@Sysix Sysix force-pushed the graphite-base/17970 branch from 82b44d7 to 8d48bd7 Compare January 13, 2026 20:07
@Sysix Sysix changed the base branch from graphite-base/17970 to main January 13, 2026 20:08
@Sysix Sysix force-pushed the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch 3 times, most recently from 1f20f99 to 446f709 Compare January 13, 2026 21:20
@Sysix Sysix requested a review from Copilot January 13, 2026 21:42
Copy link
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 pull request refactors binary path resolution in the VS Code extension to use Node.js's require.resolve instead of filesystem globbing. The change aims to simplify and potentially improve the reliability of locating oxlint and oxfmt binaries in node_modules.

Changes:

  • Replaced workspace file globbing with require.resolve for binary discovery
  • Updated runExecutable function to accept a nodeBinName parameter for better path pattern detection
  • Simplified test helpers and removed file system operations from tests

Reviewed changes

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

Show a summary per file
File Description
editors/vscode/client/ConfigService.ts Replaced file globbing with require.resolve and removed cancellation token logic
editors/vscode/client/tools/lsp_helper.ts Added nodeBinName parameter and path pattern detection for Node.js binaries
editors/vscode/client/tools/linter.ts Updated runExecutable call to pass "oxlint" as tool name
editors/vscode/client/tools/formatter.ts Updated runExecutable call to pass "oxfmt" as tool name
editors/vscode/tests/unit/lsp_helper.spec.ts Updated all test calls to include the new tool parameter
editors/vscode/tests/unit/ConfigService.spec.ts Simplified test helpers and removed file system setup/teardown operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Sysix Sysix force-pushed the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch from 446f709 to dacfcb8 Compare January 13, 2026 22:09
@Sysix Sysix force-pushed the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch 2 times, most recently from ba57f47 to cd64640 Compare January 14, 2026 18:26
@Sysix Sysix marked this pull request as ready for review January 14, 2026 18:43
@Sysix Sysix requested a review from camc314 as a code owner January 14, 2026 18:43
@rikbrown
Copy link

Confirming as an affected user in #17936, this fixes the problem for me (mac/pnpm).

@Sysix
Copy link
Member Author

Sysix commented Jan 18, 2026

@connorshea / @Boshen: I think @camc314 is not available before the next release. Could you look into this stack :)? The most code was inspired by biome:
https://github.com/biomejs/biome-vscode/blob/ae9b6df2254d0ff8ee9d626554251600eb2ca118/src/locator.ts#L267-L339

@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Jan 18, 2026
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 18, 2026

Merge activity

Refactored the search to use `path.resolve` instead of glob search with VSCode.
The VSCode snippet did some excludes in the background with `search.excludes` and/or other settings.
Now it should work without these restriction and hopefully

closes #17936
closes #17881
closes #17744

It will not search for globally installed oxlint/oxfmt. Created an extra issue for this: #18005
@graphite-app graphite-app bot force-pushed the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch from cd64640 to a7e2eb2 Compare January 18, 2026 15:44
@graphite-app graphite-app bot merged commit a7e2eb2 into main Jan 18, 2026
20 checks passed
@graphite-app graphite-app bot deleted the 01-13-fix_vscode_resolve_binary_paths_with_node_resolver branch January 18, 2026 15:49
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 18, 2026
@connorshea
Copy link
Member

Speak of the devil and he shall appear :)

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

Labels

A-editor Area - Editor and Language Server C-bug Category - Bug

Projects

None yet

4 participants