fix: bundle ripgrep and fall back to it when VS Code's copy is not found#243
fix: bundle ripgrep and fall back to it when VS Code's copy is not found#2430xMink wants to merge 7 commits into
Conversation
0xMink
requested review from
JamesRobert20,
edelauna,
hannesrudolph and
navedmerchant
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBundles a ripgrep binary into the extension build, exposes a platform-arch scoped ChangesRipgrep bundling and fallback
🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
src/services/ripgrep/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses failures in search_files / list_files caused by VS Code Insiders’ staged-install layout by bundling a ripgrep binary into the extension and falling back to it when VS Code’s built-in copy cannot be found.
Changes:
- Add
@vscode/ripgrepas a build-time dependency and copy itsrgbinary intodist/bin/during the esbuild step. - Update
getBinPathto prefer VS Code’srgbut fall back to the bundled binary when necessary. - Add unit tests for the new fallback behavior and add CI validation that the VSIX contains the bundled binary.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/ripgrep/index.ts |
Adds a bundled rg path and extends getBinPath with a bundled-binary fallback. |
src/services/ripgrep/__tests__/index.spec.ts |
Adds tests to validate fallback selection and preference order. |
src/package.json |
Adds @vscode/ripgrep to enable bundling the binary at build time. |
src/esbuild.mjs |
Introduces copyRipgrep plugin to copy rg into the dist output. |
pnpm-lock.yaml |
Locks the new @vscode/ripgrep dependency. |
.github/workflows/marketplace-publish.yml |
Adds a packaged-VSIX assertion that extension/dist/bin/rg is present. |
.changeset/bundle-ripgrep-fallback.md |
Documents the patch-level change for release notes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (await checkPath("node_modules/@vscode/ripgrep/bin/")) || | ||
| (await checkPath("node_modules/vscode-ripgrep/bin")) || | ||
| (await checkPath("node_modules.asar.unpacked/vscode-ripgrep/bin/")) || | ||
| (await checkPath("node_modules.asar.unpacked/@vscode/ripgrep/bin/")) | ||
| (await checkPath("node_modules.asar.unpacked/@vscode/ripgrep/bin/")) || | ||
| ((await fileExistsAtPath(bundledRgPath)) ? bundledRgPath : undefined) |
There was a problem hiding this comment.
Addressed in 838fb91.
The bundled binary is now copied to and resolved from dist/bin/<platform>-<arch>/ (e.g. dist/bin/linux-x64/rg). bundledRgPath includes the ${process.platform}-${process.arch} segment, so on macOS it resolves to dist/bin/darwin-arm64/rg — which a Linux-built universal VSIX does not contain — and the fallback correctly yields nothing instead of an incompatible binary. A test was added asserting the path is platform/arch-scoped.
| const { rgPath } = await import("@vscode/ripgrep") | ||
| if (!rgPath) { | ||
| throw new Error("[copyRipgrep] @vscode/ripgrep did not provide rgPath") | ||
| } | ||
| const rgDestDir = path.join(distDir, "bin") | ||
| fs.mkdirSync(rgDestDir, { recursive: true }) | ||
| const rgDest = path.join(rgDestDir, path.basename(rgPath)) | ||
| fs.copyFileSync(rgPath, rgDest) | ||
| fs.chmodSync(rgDest, 0o755) | ||
| console.log(`[copyRipgrep] Copied ${rgPath} to ${rgDest}`) |
There was a problem hiding this comment.
Addressed in 838fb91.
copyRipgrep now writes the binary under dist/bin/${process.platform}-${process.arch}/, and getBinPath resolves the matching <platform>-<arch> segment at runtime. A binary built for one OS can no longer be picked up by the fallback on another — even though the VSIX is universal and rg shares a filename across Linux and macOS.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The published VSIX is universal but copyRipgrep bundled only the build host's rg. Because rg (no extension) is the binary name on both Linux and macOS, a macOS user could resolve a Linux-built dist/bin/rg and execute an incompatible binary. Bundle and resolve the binary under dist/bin/<platform>-<arch>/ so the fallback is used only on a host matching the bundled binary.
|
Nice fix — preferring VS Code's own |
proyectoauraorg
left a comment
proyectoauraorg
left a comment
There was a problem hiding this comment.
Code Review: Bundle ripgrep fallback
Overall Assessment: Well-designed solution with thorough testing. Approve with minor suggestions.
Strengths
-
Clean fallback chain: The
getBinPath()now prefers VS Code's own ripgrep and falls back to the bundled copy — correct priority order. -
Platform-scoped binary path: The
<platform>-<arch>segment indist/bin/prevents the universal VSIX from handing a Linux binary to macOS users. Smart design. -
Test coverage is excellent: The 4 test cases cover:
- Fallback when VS Code copy is absent (Insiders staged-install)
- Preference for VS Code copy over bundled
undefinedwhen nothing exists- Platform/arch path validation
-
CI validation: The
marketplace-publish.ymlchange ensures the binary is actually present in the VSIX before publishing.
Minor Suggestions
-
VSIX size impact: The bundled ripgrep binary adds ~20MB to the VSIX. Consider noting this in the changeset or adding a comment in
esbuild.mjsabout the size tradeoff. Users on slow connections may notice. -
Cross-compilation edge case: If someone builds the VSIX on a CI runner with a different arch than the target (e.g., building on ARM64 for x86 users), the bundled binary would be wrong. This is fine for the current CI setup but worth a comment.
-
Error message: When
@vscode/ripgrepdoesn't providergPath, the esbuild plugin throws. Consider adding a more descriptive error:"@vscode/ripgrep did not provide rgPath — check that the package is installed correctly".
Nit
The fileExistsAtPath import in the test is clean and the mocking approach is idiomatic Vitest. No issues there.
LGTM 🚀
Summary
search_filesandlist_filesfail withCould not find ripgrep binaryon VS Code Insiders' newer staged-install layout, where program files moved into a per-build subfolder (microsoft/vscode#252063).getBinPathonly checked four hardcoded paths undervscode.env.appRoot, none of which resolve on that layout.This bundles a ripgrep binary inside the extension and uses it as a fallback:
@vscode/ripgrepas a build devDependency.copyRipgrepesbuild plugin copies the platformrgbinary intodist/bin/so it ships inside the VSIX.getBinPathgains a final fallback to the bundled binary; VS Code's own copy is still preferred when present.All three
getBinPathcallers (search_files,list_files,file-search) inherit the fix with no changes.Scope — PR 1 of 2
The extension ships a single universal VSIX, and
@vscode/ripgrepinstalls only the build host's platform binary, so the released VSIX carriesrgfor the CI build platform only. That fully fixes that platform and is a non-regression elsewhere — on a platform whose binary is not bundled, the fallback simply finds nothing, exactly as before this change. A follow-up PR will move to platform-specific VSIXs so every platform ships a usable binary.Test plan
vitest— newgetBinPathtests: fallback used when nothing exists under the app root; VS Code's own copy preferred over the bundled one;undefinedwhen ripgrep exists nowhere.pnpm vsix— the packaged VSIX containsextension/dist/bin/rg.search_files/list_fileswork.Summary by CodeRabbit