Skip to content

fix(repair): discover newest NVM Node release first - #2645

Open
rudycelekli wants to merge 1 commit into
debpalash:mainfrom
rudycelekli:fix/electron-nvm-version-order-20261006
Open

rudycelekli wants to merge 1 commit into
debpalash:mainfrom
rudycelekli:fix/electron-nvm-version-order-20261006

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

With installed NVM releases v9, v22 and v24 and no usable inherited Node path, desktop discovery sorts version folder names lexicographically. This puts v9 ahead of v24. Similarly v22.9 precedes v22.10. Discovery should prefer the newest numeric release while retaining inherited PATH precedence.

Closes #2644.

Changes

  • Sort recognized NVM release folders by numeric version after inherited PATH candidates.
  • Synchronize relevant documentation and add a quiet credited Unreleased changelog entry.
  • No new UI strings or locale keys; maintained version files are unchanged.

Type

  • 🐛 Bug fix
  • ✨ New feature
  • ♻️ Refactor
  • 📝 Documentation
  • 🧪 Tests
  • 🔧 CI / Build
  • 🚀 Release prep

Testing

  • Hosted CI audit at 2026-10-06T12:35:20.584679+00:00: no failing latest checks; required CLA passes. All latest reported checks completed successfully. No upstream merge performed.

  • Real temporary NVM directory regression: 1 failed and 3 passed before; all 4 pass after. macOS filesystem/subprocess coverage only.

  • git diff --check and the repository changelog style gate pass.

  • Focused JavaScript tests used Node 24.19.0 and a reused Vitest 5.0.1 runtime; this repository pins Vitest 4.1.11. No full build qualification is claimed.

  • Full backend suites and bun run check:electron were not run locally. The human CLA signature is registered and the required CLA status passes; hosted results are reported separately below.

Checklist

  • I've tested this locally
  • Every commit author has signed the CLA (the CLA check tells you how)
  • I've updated relevant documentation (if applicable)
  • No local machine paths, logs, or personal env details in this PR
  • Maintained version files are in sync (if an owner-requested bump): root package.json, pyproject.toml, backend/core/version.py, and lockfiles
  • If this PR changes runtime behavior, the regression fixture at tests/fixtures/omnivoice_data/ still loads green on the smoke-matrix CI job (macOS + Windows + Linux)

Release cadence

VoiceStudio ships continuous-to-main — no release candidates, no soak windows.
Every merged PR is immediately part of rolling source (main) and Docker
:latest. Electron artifact rehearsals validate desktop packages without publishing.
Version bumps require owner approval; validated releases are tagged from main
and published explicitly under the release checklist.
Users who want stability install an Electron release or pin Docker :stable.

NVM release directories are now searched in descending numeric version order, while inherited PATH candidates retain precedence. This prevents older releases such as v9 or v22.9 from outranking v24 or v22.10. Full backend checks and cross-platform smoke tests remain pending, so cross-platform behavior needs verification.

Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Changes how the desktop app discovers Node versions.

The PR appears safe to merge based on the reviewed changes.

Summary

The PR changes Electron’s NVM directory search to order recognized Node releases numerically, while preserving inherited PATH precedence.

  • Adds a directory-ordering regression test and updates the repair documentation and changelog.

Reviews (1) · Last reviewed commit: "fix(repair): discover newest NVM Node re..."

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: debpalash/VoiceStudio/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 04e8e75f-ad50-4e34-81ab-e45f07def398
📥 Commits

Reviewing files that changed from the base of the PR and between 286f46b and a41d5b3.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • docs/electron-repair.md
  • electron/src/main/tool-path.test.ts
  • electron/src/main/tool-path.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

NVM release directories are now sorted by descending numeric Node version instead of lexicographic order. A regression test checks that ordering and confirms inherited PATH entries remain first. The documentation and changelog describe the discovery behavior.

Changes

NVM Node Discovery

Layer / File(s) Summary
Numeric release ordering and verification
electron/src/main/tool-path.ts, electron/src/main/tool-path.test.ts, docs/electron-repair.md, CHANGELOG.md
NVM release directories use descending numeric-aware version ordering. The test checks versions with different minor-component widths and preserves inherited PATH precedence. The documentation and changelog describe the discovery behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: debpalash

Merge Risk: ⚪ Minimal · up to a41d5

Desktop discovery should now prefer the numerically newest installed NVM Node when falling back from inherited PATH. No concrete merge-blocking risk is established by the supplied review; full platform qualification remains outstanding.

🚥 Pre-merge checks | ✅ 7 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Cross-Platform Default Parity ⚠️ Warning The default discovery behavior now prefers the newest NVM release on macOS and Linux, but Windows never searches NVM release directories. toolSearchDirs() applies the changed numeric sort in the non… Add equivalent Windows NVM release discovery with numeric ordering and verify the default behavior on macOS, Windows, and Linux; or move NVM release discovery behind an explicit opt-in such as a Settings toggle, environment variable, or CLI…
✅ Passed checks (7 passed)
Check name Status Explanation
Linked Issues check ✅ Passed [#2644] nvmBinDirs filters version-form release folders and sorts them in descending numeric order. The regression test checks v24 before v22.10, v22.10 before v22.9, and v22.9 before v9, while keep…
Out of Scope Changes check ✅ Passed The documentation, regression test, and changelog entry all support the NVM discovery fix in [#2644]. The diff contains no unrelated changes.
I18n Completeness (21 Locales) ✅ Passed The check is not triggered by this PR. The Electron changes are limited to NVM path sorting in electron/src/main/tool-path.ts and a regression test; they add or change no UI translation keys or user…
Local-First Guarantee ✅ Passed The PR changes only NVM directory filtering and numeric sort order, local-path discovery tests, and documentation. toolSearchDirs reads local directories and returns search paths; it adds no network…
Backward Compatibility ✅ Passed The change only reorders NVM directories in desktop CLI discovery (electron/src/main/tool-path.ts); its consumer searches for uv in those directories. The PR changes no voice, project, settings, e…
Title check ✅ Passed The title uses the required conventional-commit format with scope, and the description references issue #2644.
Description check ✅ Passed The description includes the required sections and explains the change, tests, checklist status, and release cadence. It also identifies pending platform checks.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 unsupported.)

Full details: Cross-Platform Default Parity

Explanation

The default discovery behavior now prefers the newest NVM release on macOS and Linux, but Windows never searches NVM release directories. toolSearchDirs() applies the changed numeric sort in the non-Windows branch (electron/src/main/tool-path.ts:15–16, 43–53), and production callers use it without opt-in (electron/src/main/backend.ts:258–268). The added regression test covers only macOS (electron/src/main/tool-path.test.ts:47–60), so the default behavior diverges on Windows.

Resolution

Add equivalent Windows NVM release discovery with numeric ordering and verify the default behavior on macOS, Windows, and Linux; or move NVM release discovery behind an explicit opt-in such as a Settings toggle, environment variable, or CLI flag.

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

All contributors on this pull request have signed the VoiceStudio CLA. Thank you!

@rudycelekli

Copy link
Copy Markdown
Contributor Author

recheck

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.

Desktop Node discovery ranks older NVM releases ahead of newer ones

1 participant