test(editor): cover LSP server preset resolution - #1063
Conversation
serversForLanguage, serverForLanguage, serverById and allServers resolve which language server handles a file, but had no tests. Lock the built-in matches for a language, custom servers appended after presets, null/unknown language returning nothing, and the activation precedence in serverForLanguage: an enabled server wins over preset order, a dismissed one is skipped for the next fresh candidate, and the first candidate is the default.
📝 WalkthroughWalkthroughAdds Vitest coverage for LSP preset selection utilities, including language matching, enabled and dismissed server handling, ID lookup, custom-server inclusion, and aggregate server listing. ChangesLSP preset selection tests
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/lsp/lib/presets.test.ts`:
- Around line 77-79: Strengthen the allServers test for custom-server ordering
by asserting the resulting server IDs, with customPy.id as the final entry after
all preset IDs. Update the test around allServers and retain the existing length
check only if it remains useful.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a92ecb-31d4-4739-9d57-c8aa931ce469
📒 Files selected for processing (1)
src/modules/lsp/lib/presets.test.ts
| it("appends custom servers to the presets", () => { | ||
| expect(allServers([customPy])).toHaveLength(LSP_PRESETS.length + 1); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert custom-server identity and append order.
Line [78] checks only the count, so allServers could return the wrong custom server or place it before the presets while this test still passes. Assert the resulting IDs, including customPy.id as the final entry.
Proposed test assertion
- expect(allServers([customPy])).toHaveLength(LSP_PRESETS.length + 1);
+ expect(allServers([customPy]).map((p) => p.id)).toEqual([
+ ...LSP_PRESETS.map((p) => p.id),
+ customPy.id,
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("appends custom servers to the presets", () => { | |
| expect(allServers([customPy])).toHaveLength(LSP_PRESETS.length + 1); | |
| }); | |
| it("appends custom servers to the presets", () => { | |
| expect(allServers([customPy]).map((p) => p.id)).toEqual([ | |
| ...LSP_PRESETS.map((p) => p.id), | |
| customPy.id, | |
| ]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/lsp/lib/presets.test.ts` around lines 77 - 79, Strengthen the
allServers test for custom-server ordering by asserting the resulting server
IDs, with customPy.id as the final entry after all preset IDs. Update the test
around allServers and retain the existing length check only if it remains
useful.
1 participant
What
Adds unit tests for the LSP preset resolvers in
lsp/lib/presets.ts. Test-only:no production files touched.
Why
serversForLanguage,serverForLanguage,serverByIdandallServersdecidewhich language server handles a file, and the activation precedence in
serverForLanguageis non-obvious. None had tests.How
Pure functions, tested directly against the built-in
LSP_PRESETS.Locked behavior:
serversForLanguage: the built-in servers that claim a language (py -> pyright,ruff), custom servers appended after presets, and nothing for a null or unknown
language.
serverForLanguageprecedence: first candidate by default, anenabledserverwins over preset order, and a
dismissedserver is skipped for the next freshcandidate.
serverById: preset and custom lookup, null for an unknown id.allServers: presets plus custom servers.Testing
Ran the full suite plus type-check and lint locally on Windows.
pnpm lintclean (unchanged: 103 pre-existing warnings, none introduced)pnpm check-typescleanpnpm testclean (the new suite passes; see reviewer note on onepre-existing suite)
src-tauri/) cargo clippy - N/A, no Rust changessrc-tauri/) cargo nextest - N/A, no Rust changes#[tauri::command]signature) - N/Apnpm tauri dev- N/A, no UI changeScreenshots / GIFs
N/A - no UI change.
Notes for reviewer
mainon purpose (my token lacksthe
workflowscope). Only adds a new file; should merge cleanly. Happy torebase.
src/app/eager-budget.test.tsfails locally under Windows with
git core.autocrlf=true; the committed blob isLF and it passes on CI.
Summary by CodeRabbit