Add support for fish-lsp#19
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR adds the Changesfish-lsp Zed Extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
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/lib.rs`:
- Around line 67-85: The current code uses the `?` operator on the version
checking calls (npm_package_installed_version and npm_package_latest_version)
and the install method call (Fish::install_npm_language_server), which
propagates errors and blocks LSP startup. Instead, wrap the version checking and
update operations in error handling logic that gracefully handles failures. When
npm_package_installed_version, npm_package_latest_version, or the
Fish::install_npm_language_server call fails, check if a local binary already
exists at the expected path. If a local binary exists, allow the LSP to proceed
despite the version check or update failure. Only propagate an error if no local
binary is available. This way, transient network or registry issues do not block
LSP startup when a usable binary is already present.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef4e7f93-a2e9-4808-b924-b283061f0da9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoreCargo.tomlextension.tomlrust-toolchain.tomlsrc/lib.rs
|
@cswimr thank you for the PR. I will review it by the end of the week. |
|
Addressed review feedback. I also made a couple other changes, primarily stylistic ones (like using |
This PR adds support for fish-lsp.
Binary resolution works how you would expect; we first check
lsp.fish-lsp.binary.pathfor a set binary path, then we check$PATHfor afish-lspbinary, and finally we installfish-lspfrom NPM if another binary can't be found. Note that this has a runtime requirement on thefishshell being present in$PATH, or the LSP setup will fail. I decided to make this error in the extension instead of letting the LSP error, so that users get a clearer error message.I didn't include any LSP settings, nor did I test any initialization options. Initialization options should work, I think? Didn't feel like adding support for LSP settings, it could be done in the future through environment variables though if I understand
fish-lsp's help output correctly.This is my first time really using Rust. I did test this, and everything works fine as far as I can tell, but I'm not sure on general code quality / best practices and such.
LLM Usage: LLMs were used to assist with figuring out some Rust related stuff, but did not write any code. This is all human-written slopcode, not LLM-generated slopcode.
Edit: Technically used some coderabbit-suggested code, after review. Even this was tweaked a little though.
Closes #8.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
0.2.0and registered the “Fish LSP” language server for Fish./targetdirectory.