feat: add Zed extension and make language server editor-agnostic#125
feat: add Zed extension and make language server editor-agnostic#125withxat wants to merge 24 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughAdds an in-repo Zed extension that launches the shared npmx language server over stdio and forwards Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (4)
packages/language-server/src/server.ts (1)
1-4: Optional: collapse the two imports fromnpmx-language-service/types.Lines 1 and 4 both import from the same module; the type and value imports can be combined into a single statement using inline
typemarkers without violating the "type imports first" ordering rule.♻️ Proposed refactor
-import type { ClientFeatures } from 'npmx-language-service/types' import { createConnection, createServer, createSimpleProject } from '@volar/language-server/node' import { createNpmxLanguageServicePlugins } from 'npmx-language-service' -import { DEFAULT_CLIENT_FEATURES } from 'npmx-language-service/types' +import { type ClientFeatures, DEFAULT_CLIENT_FEATURES } from 'npmx-language-service/types'packages/language-server/src/workspace.ts (1)
17-32: Confirm the intent ofstopDir: rootPathand the implicit'deno' → 'npm'mapping.By default
detect()crawls upward to parent directories, but pinningstopDirtocwdhere effectively restricts detection to the workspace root only. That is likely what you want for a per-folderWorkspaceContext, but it does mean that opening, e.g., a sub-folder of a monorepo will silently fall back to'npm'rather than picking up the parent's lockfile. Worth confirming this matches your intended UX.Separately,
package-manager-detectorcan also return'deno'. Thedefaultbranch maps it (and any future agent) to'npm'. If'deno'is intentionally unsupported byPackageManager, consider an explicitcase 'deno':to make the fallback intent obvious and avoid a silent regression shouldPackageManagerever gain a'deno'member.packages/language-service/src/plugins/hover.ts (1)
10-35: Clean icon registry; small consolidation opportunity.The single
ICONSregistry plusiconLabel/iconTexthelpers nicely centralise the codicon-vs-emoji branching previously scattered through the renderer. The vs regular space split betweeniconLabel(used inside[…](…)link text where Markdown otherwise collapses whitespace) andiconText(plain text) is a sensible distinction.If you'd like to trim a few lines, the two helpers differ only in the separator character and could be unified, e.g.:
♻️ Optional consolidation
-function iconLabel(iconStyle: IconStyle, name: IconName, label: string): string { - const { codicon, emoji } = ICONS[name] - return iconStyle === 'codicon' - ? `$(${codicon}) ${label}` - : `${emoji} ${label}` -} - -function iconText(iconStyle: IconStyle, name: IconName, text: string): string { - const { codicon, emoji } = ICONS[name] - return iconStyle === 'codicon' - ? `$(${codicon}) ${text}` - : `${emoji} ${text}` -} +function renderIcon(iconStyle: IconStyle, name: IconName, text: string, sep: ' ' | ' ' = ' '): string { + const { codicon, emoji } = ICONS[name] + return iconStyle === 'codicon' ? `$(${codicon})${sep}${text}` : `${emoji} ${text}` +}Happy to leave as-is for clarity.
packages/language-service/src/config.ts (1)
90-115: Up to three sequentialworkspace/configurationround-trips pergetConfigcall.
getConfigis invoked on every hover/completion/diagnostic; in the worst case it now performs three sequentialgetConfiguration(...)LSP requests (section,spec.scopedKey,scopedConfigs.scope). VS Code clients typically resolve configuration synchronously from a local cache, but for Zed (and any client without aggressive caching) every miss is a real round-trip on the request thread, multiplied across feature calls.Two low-effort mitigations worth considering:
- Short-circuit when a known shape is expected (e.g., cache the detected "shape" per workspace after the first successful resolution and only query that source thereafter).
- Fetch the root
npmxobject once per request and serve all keys from it, falling back to per-key calls only if the root isundefined.Not a blocker — flagging because it's invisible from the diff but compounds quickly under load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d86122a-beae-4cda-b56c-61f68b3e21b1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
README.mdextensions/vscode/src/client.tsextensions/vscode/src/request.tsextensions/zed/.gitignoreextensions/zed/Cargo.tomlextensions/zed/README.mdextensions/zed/extension.tomlextensions/zed/src/lib.rspackages/language-server/package.jsonpackages/language-server/src/server.tspackages/language-server/src/workspace.test.tspackages/language-server/src/workspace.tspackages/language-server/tsdown.config.tspackages/language-service/src/config.tspackages/language-service/src/plugins/catalog.tspackages/language-service/src/plugins/hover.test.tspackages/language-service/src/plugins/hover.tspackages/language-service/src/types.tspackages/shared/src/protocol.tspnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- extensions/vscode/src/request.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/publish-language-server.yml (1)
26-26: ⚡ Quick winUse JSON parsing instead of
sedfor version extraction.Line 26 depends on exact whitespace/comma formatting in
package.json, making the release check fragile. Prefernode -p "require('./packages/language-server/package.json').version"(orjq) for a stable check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05fa6e58-217f-470e-aeb9-9612c7aee164
📒 Files selected for processing (8)
.github/workflows/publish-language-server.yml.github/workflows/publish-vscode.yml.github/workflows/publish-zed.ymlextensions/zed/Cargo.tomlextensions/zed/LICENSE.mdextensions/zed/extension.tomlpackage.jsonscripts/sync-zed-version.mjs
✅ Files skipped from review due to trivial changes (3)
- extensions/zed/Cargo.toml
- scripts/sync-zed-version.mjs
- extensions/zed/extension.toml
| # TODO: Restore this trigger after these Zed release changes are merged to the main branch. | ||
| # push: | ||
| # tags: | ||
| # - v* | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Do not merge with tag publishing disabled unless this is intentionally permanent.
Line 4–Line 8 remove automatic tag-triggered publishing, which can silently halt VS Code release automation on v* tags. If this is temporary, gate it behind a short-lived branch-only change or restore the trigger before merge.
| fi | ||
|
|
||
| - name: Update Zed extensions registry | ||
| uses: huacnlee/zed-extension-action@v2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f .github/workflows/publish-zed.yml ]; then
echo "=== File exists, reading lines 20-40 for context ==="
sed -n '20,40p' .github/workflows/publish-zed.yml | cat -n
echo ""
echo "=== Full workflow file ==="
cat -n .github/workflows/publish-zed.yml
else
echo "File not found"
fiRepository: npmx-dev/vscode-npmx
Length of output: 2566
🏁 Script executed:
# Check other workflow files to understand pinning patterns in the repo
echo "=== Searching for action usage patterns across all workflows ==="
find .github/workflows -name "*.yml" -o -name "*.yaml" | while read f; do
echo "--- File: $f ---"
rg "uses:" "$f" | head -20
doneRepository: npmx-dev/vscode-npmx
Length of output: 1127
Pin the Zed publish action to a commit SHA.
Line 30 uses a mutable tag (@v2) whilst all other actions in the repository (across six workflows) are pinned to commit SHAs. Update to a specific commit digest to maintain consistency with repository security practices and reduce supply-chain risk in release workflows.
| # TODO: Update this to the correct fork before enabling automated publishing. | ||
| push-to: withxat/issue-zed-extensions | ||
| env: | ||
| # TODO: Add the COMMITTER_TOKEN secret before enabling automated publishing. | ||
| COMMITTER_TOKEN: ${{ secrets.COMMITTER_TOKEN }} |
There was a problem hiding this comment.
Do not keep placeholder publishing config in an active tag-triggered workflow.
Line 33–Line 37 still point to a temporary fork and TODO token setup while Line 4–Line 6 already publish on v* tags. This can publish to the wrong destination or fail releases; disable the trigger until final push-to and secrets are ready.
|
@9romise Quick update + a question about next steps: The implementation is mostly done, and I’ve been testing it locally for a while — it seems to work pretty well. What remains is mainly packaging / publishing. Since this is Rust-based, it doesn’t fit perfectly into the existing VS Code extension workflow. One blocker is the LSP binary: it doesn’t seem to be published anywhere right now, while Zed expects a downloadable artifact. My current workaround is to fetch it from GitHub Release assets, though publishing the LSP officially might be a cleaner option. Would you prefer this to live in this repo and go through the normal release flow, or should it be maintained as a separate repo/package? Happy to help with either direction. |
Sorry for the late reply! |
|
@9romise Sorry I’ve been busy with my own stuff lately. This PR is pretty much complete now, so could you please review whether the changes to the LSP and VS Code extension look appropriate? The main change is adding a parameter to determine whether to use VSCodium-specific icons or plain emoji, with the goal of making the LSP more editor-agnostic. I also added and updated the workflows, and included the unified version bump changes as well. As for the Zed extension, like I mentioned earlier, the biggest blocker is that the LSP needs to be published separately—either as a package on npmjs, or via the approach I’ve implemented here: distributing it through GitHub Releases. PTAL :) |
2 participants
Summary
npmx-language-serverover stdio, giving Zed users hover, version completion, diagnostics, document links, and catalog resolution.package-manager-detectorreplaces thenpmx/getPackageManagerrequest, and a newClientFeatureschannel lets each editor opt into codicons, catalog inlay hints, etc.lsp.npmx.settings.*shape works alongside VS Code's namespaced keys.What changed
New: Zed extension (
extensions/zed/)wasm32-wasip2, built againstzed_extension_api0.7.node packages/language-server/dist/index.cjs --stdioby default; respectslsp.npmx.binaryfor full overrides, mergingworktree.shell_env()with user-supplied env.lsp.npmx.settingsas workspace configuration under thenpmxnamespace.Language server: editor-agnostic
npmx/getPackageManagerrequest that depended on VS Code'snpm.packageManagercommand. Now usespackage-manager-detector, bundled into the dist.ClientFeatures(negotiated viainitializationOptions):iconStyle: 'codicon' | 'emoji'— VS Code uses codicons, others get emoji.catalogInlayHints: boolean— VS Code keeps its custom decorations; non-VS Code clients get LSP-native inlay hints.DEFAULT_CLIENT_FEATURESco-located with theClientFeaturesinterface inlanguage-service/types.ts.Language service
getConfigresolves keys in order: exact (npmx.foo.bar) → scoped (foo.bar) → nested undernpmx. Lets Zed users write flat config without the prefix.(useCodicons, codicon, emoji)triples at every call site.inlayHintProvideron the catalog plugin shows the resolved spec next tocatalog:fooreferences, gated onClientFeatures.catalogInlayHints.VS Code
iconStyle: 'codicon'andcatalogInlayHints: falseininitializationOptionsto preserve existing UX.extensions/vscode/src/request.ts.Tests
workspace.test.tscoversdetectPackageManagerFromProject(supported PMs, unsupported, no detection).hover.test.tssnapshotsrenderHoverMarkdownfor both icon styles plus the JSR fallback path.I'm not sure if it's too early to consider supporting editors beyond VS Code, but I've been using Zed and really enjoying it. I also love using npmx, so I thought this might be helpful.
I haven’t published the extension for Zed myself because I felt that might be stepping beyond my role and potentially disrespectful to the original maintainers, especially since expanding support to another editor is ultimately a project decision. I just wanted to share this in case it could be useful and help move things forward.
There are also quite a few choices in the code that reflect my personal preferences—such as using emoji or inlay hints—so please feel free to adjust or change anything as needed.