fix: LSP workspace root detection and absolute URI generation#8938
fix: LSP workspace root detection and absolute URI generation#8938daizutabi wants to merge 3 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes LSP initialization for basedpyright by ensuring the client provides absolute file: URIs and a meaningful workspace root so the server can discover project configuration (e.g., pyproject.toml) as intended.
Changes:
- Added workspace-root derivation in LSP utilities (using
cwd+ workspace-relative filename). - Updated LSP document/root URI generation to prefer absolute paths when possible.
- Configured basedpyright initialization to set
rootUriandworkspaceFoldersexplicitly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frontend/src/core/codemirror/lsp/utils.ts | Adds workspace root detection and updates document/root URI generation to prefer absolute paths. |
| frontend/src/core/codemirror/lsp/tests/utils.test.ts | Adds unit tests covering URI/root behavior across Unix/Windows and fallback cases. |
| frontend/src/core/codemirror/language/languages/python.ts | Updates basedpyright client initialization to pass rootUri and workspaceFolders. |
| export function getLSPDocument() { | ||
| return `file://${getFilenameFromDOM() ?? "/__marimo_notebook__.py"}`; | ||
| const root = getWorkspaceRoot(); | ||
| const filename = getFilenameFromDOM(); | ||
|
|
||
| if (root && filename) { | ||
| return `file://${PathBuilder.guessDeliminator(root).join(root, filename)}`; | ||
| } |
There was a problem hiding this comment.
getLSPDocument()/getLSPDocumentRootUri() are building file URIs by simple string concatenation with OS paths. This produces invalid file: URIs on Windows (backslashes + missing file:///C:/... form) and also fails to percent-encode characters like spaces or #, which can break URI parsing in LSP servers (e.g., pyright/basedpyright). Consider introducing a small toFileUri(absolutePath: string) helper that normalizes separators to /, handles Windows drive letters, and URL-encodes path segments, and use it for both the document URI and root URI.
| function getWorkspaceRoot() { | ||
| const cwd = store.get(cwdAtom); | ||
| const filename = getFilenameFromDOM(); | ||
|
|
||
| if (!cwd || !filename) { | ||
| return null; | ||
| } | ||
|
|
||
| // Split the relative path from the workspace root | ||
| const numDirs = filename.split(/[/\\]/).filter(Boolean).length - 1; | ||
|
|
||
| let root = cwd; | ||
| for (let i = 0; i < numDirs; i++) { | ||
| root = Paths.dirname(root); | ||
| } | ||
| return root; |
There was a problem hiding this comment.
getWorkspaceRoot() assumes filename is a workspace-relative path. When filename is absolute (non-directory mode), numDirs becomes very large and the loop walks cwd up to ""; the function then silently falls back to other branches. It would be safer (and cheaper) to bail out early when Paths.isAbsolute(filename) is true, and only attempt this calculation for workspace-relative display paths.
| const rootUri = getLSPDocumentRootUri(); | ||
| const lspClientOpts = { | ||
| transport, | ||
| rootUri: getLSPDocumentRootUri(), | ||
| workspaceFolders: [], | ||
| rootUri, | ||
| workspaceFolders: [{ uri: rootUri, name: "marimo" }], | ||
| }; |
There was a problem hiding this comment.
workspaceFolders is now always set for basedpyright using rootUri. In fallback cases getLSPDocumentRootUri() can return values like file:/// (no filename) or file://. (no cwd), which can cause basedpyright to treat the filesystem root/current directory as the workspace and potentially trigger very expensive scans or even fail URI parsing. Recommend only setting workspaceFolders when you have a real absolute project root (e.g., derived from directory-mode workspace root), otherwise keep it []/null and rely on the per-document URI.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 Summary
This PR addresses Issue #8662, where basedpyright failed to apply project-level configurations (e.g.,
pyproject.toml).The root cause was two-fold:
file://notebook.py).rootUriorworkspaceFolders.Without an absolute workspace root, LSP servers cannot determine the project's boundaries. Consequently, they fail to discover and load configuration files like
pyproject.tomlorpyrightconfig.jsonthat reside in the root directory, falling back to default global settings instead.Closes #8662
Changes
getWorkspaceRoot()utility: Introduced a new function infrontend/src/core/codemirror/lsp/utils.tsthat calculates the absolute workspace root.getLSPDocument()andgetLSPDocumentRootUri()to return proper absolutefile:///URIs.workspaceFolders, ensuring the server correctly identifies the project root and applies local configurations.Verification (Screenshots)
Verified using a root
pyproject.tomlwith:Before
The project configuration is ignored because the workspace root was not specified. basedpyright incorrectly reports a warning on line 4 (
func()) despite it being disabled inpyproject.toml.After
The configuration is now correctly applied. The warning on line 4 is suppressed, while a legitimate type error (line 2) is still reported.
📋 Pre-Review Checklist
✅ Merge Checklist