feat: language server intelligence in the playground editor#170
feat: language server intelligence in the playground editor#170DylanPiercey wants to merge 1 commit into
Conversation
PR Preview DeployedYour changes are live at markojs.com/previews/pr-170. commit ed9b140 |
76caea8 to
4f99700
Compare
WalkthroughThis PR adds an in-browser Marko language server running in a Web Worker, backed by a virtual filesystem and browser 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 5
🧹 Nitpick comments (6)
src/util/lsp/vfs.ts (2)
36-61: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDirectory listings ignore the
directoriesset.
readDirectory/getDirectoriesonly derive entries by scanningfileskeys, so directories that exist perdirectoryExists(viadirectoriesset) but currently hold no files won't show up as children — an inconsistency between the two views of the filesystem.🤖 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/util/lsp/vfs.ts` around lines 36 - 61, The directory listing helpers in vfs.ts are only using the files map, so empty directories tracked by the directories set are omitted from readDirectory and getDirectories. Update these functions to merge entries from both files and directories when computing immediate children, using normalize and ensureTrailingSlash consistently, and keep the existing de-duplication behavior via the seen set so directoryExists and the child listing APIs stay in sync.
20-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
deleteFiledoesn't prunedirectories.
writeFileregisters ancestor dirs viaregisterDirs, butdeleteFilenever removes them, sodirectoryExists/getDirectoriescan keep reporting stale, now-empty directories after all their files are removed.Also applies to: 67-73
🤖 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/util/lsp/vfs.ts` around lines 20 - 22, `deleteFile` currently removes the file from `files` but leaves ancestor entries behind in `directories`, so stale empty directories can still be reported by `directoryExists` and `getDirectories`. Update `deleteFile` in `vfs.ts` to prune any now-empty ancestor directories after deleting the normalized path, and make sure the cleanup stays consistent with how `writeFile`/`registerDirs` adds directories and how `getDirectories`/`directoryExists` read them.src/util/lsp/assets-plugin.ts (2)
66-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
collectAssets()result.
load()re-runscollectAssets()(reads every TS lib file + Marko type defs from disk) on every resolution of the virtual module. Since both the worker and main-array plugin instances load this module, and dev-server invalidation can re-triggerload, memoize the result once per plugin instance.♻️ Proposed fix
export function markoLspAssets(): Plugin { + let cached: Record<string, string> | undefined; return { name: "marko-lsp-assets", resolveId(id) { if (id === VIRTUAL_ID) return "\0" + VIRTUAL_ID; }, load(id) { if (id !== "\0" + VIRTUAL_ID) return; - return `export default ${JSON.stringify(collectAssets())};`; + cached ??= collectAssets(); + return `export default ${JSON.stringify(cached)};`; }, }; }🤖 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/util/lsp/assets-plugin.ts` around lines 66 - 77, `markoLspAssets()` is calling `collectAssets()` inside `load()` every time the virtual module is loaded, which repeatedly reads the TS lib and Marko type files from disk. Memoize the collected assets once per plugin instance by caching the `collectAssets()` result within `markoLspAssets()` and have `load()` reuse that cached value when returning the virtual module export.
84-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSeeding all
lib.*.d.tsvariants despite a fixed, narrowerlibsetting.The generated
/tsconfig.jsononly requests["DOM", "DOM.Iterable", "ESNext"], butcollectAssets()embeds everylib.*.d.tsTypeScript ships (dozens of ES5/ES2015.../decorators variants), inflating the worker/client bundle with unused typings. Since the project always seeds the same fixed tsconfig, consider narrowing to just the required libs and their transitive dependencies (or precomputing the set once).Also applies to: 139-151
🤖 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/util/lsp/assets-plugin.ts` around lines 84 - 89, The asset seeding in collectAssets() is pulling in every lib.*.d.ts from the TypeScript lib directory, which bloats the bundle beyond the fixed tsconfig needs. Update collectAssets() (and the related asset-loading logic around the referenced shared code) to include only the libs actually required by the generated /tsconfig.json, namely the configured DOM/DOM.Iterable/ESNext set plus any transitive dependencies, or compute and cache that minimal set once instead of scanning all variants.src/util/lsp/server.ts (1)
89-101: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRequestMethod duplicated (and diverging) between client.ts and server.ts.
client.tsindependently redefinesRequestMethod/ServerMessage(per the cross-file snippet) as a narrower subset (doValidate | doHover | doComplete | doCompletionResolve | doCodeActions | doCodeActionResolve | findDefinition | findDocumentHighlights) while this file'sRequestMethodaddsfindReferences,findDocumentSymbols,findDocumentColors,getColorPresentations. Since these two definitions aren't shared, they can silently drift — e.g. if the client later needs one of the extra methods, there's no compiler signal that the sets should stay aligned, and a typo in either file's literal union won't be caught against the other side of the RPC boundary.Consider having
client.tsdo a type-only import of these types from this file (import type { ClientMessage, ServerMessage, RequestMethod } from "./server") so the two ends of the protocol share one source of truth. Type-only imports are erased at build time, so this won't pull the worker's runtime side effects (./globals,./project-defaults,setSystem(...), etc.) into the main-thread client bundle.♻️ Suggested direction
- type ServerMessage = - | { type: "ready"; id: number } - | { type: "response"; id: number; result?: unknown; error?: string }; - - type RequestMethod = - | "doValidate" - | "doHover" - | "doComplete" - | "doCompletionResolve" - | "doCodeActions" - | "doCodeActionResolve" - | "findDefinition" - | "findDocumentHighlights"; + import type { ServerMessage, RequestMethod } from "./server";🤖 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/util/lsp/server.ts` around lines 89 - 101, The RPC message types are duplicated between client.ts and server.ts and can drift apart. Move the source of truth for RequestMethod, ServerMessage, and ClientMessage to server.ts and have client.ts import them with a type-only import. Update any client-side local unions to use the shared types so both sides stay compiler-checked against the same protocol definitions, including the extra methods like findReferences and getColorPresentations.src/util/lsp/node-shims/fs-promises.ts (1)
12-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
Stats/fileNotFoundimplementations acrossfs.tsandfs-promises.ts.Both files define byte-identical (aside from the missing method above)
Statsclasses andfileNotFoundhelpers. Consider extracting a shared module (e.g.node-shims/shared.ts) that bothfs.tsandfs-promises.tsimport from, to avoid the two copies drifting.Also applies to: 59-64
🤖 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/util/lsp/node-shims/fs-promises.ts` around lines 12 - 23, The `Stats` and `fileNotFound` logic is duplicated between `fs.ts` and `fs-promises.ts`, so extract the shared `Stats` class and `fileNotFound` helper into a common module (for example, a `shared` shim) and have both `fs.ts` and `fs-promises.ts` import from it. Keep the existing `Stats` behavior, including `isFile` and `isDirectory`, but remove the duplicate local definitions so both shims stay consistent.
🤖 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 `@package.json`:
- Around line 40-41: The dependency entries for `@marko/language-server` and
`@marko/language-tools` point to versions that are not published on npm, so
installation will fail. Update the version specifiers in package.json to the
actual published releases for these two packages, keeping the dependency names
unchanged and verifying the selected versions are available before committing.
In `@src/util/lsp/client.ts`:
- Around line 65-75: The LSP client currently only logs worker failures in the
worker error handlers, which leaves the initialization promise and queued
requests unresolved when startup/import fails. Update src/util/lsp/client.ts in
the worker event handlers and the `#ready` initialization path so that worker
error/messageerror triggers rejection of the pending initialize promise and any
affected pending request entries via their stored reject callbacks. Ensure the
LSPClient class’s `#pending`, `#ready`, and `#request`() flow is cleaned up
consistently when the worker fails so later requests do not hang forever.
In `@src/util/lsp/globals.ts`:
- Around line 34-48: The existing-process branch in globals.ts is not
backfilling process.platform, so a preexisting partial globalThis.process can
leave platform undefined. Update the globals initialization logic around
g.process to assign a default platform of "browser" when it is missing, matching
the fresh-process branch behavior, alongside the other fallback fields like
browser, env, cwd, and nextTick.
In `@src/util/lsp/node-shims/fs-promises.ts`:
- Around line 12-23: The Stats shim in fs-promises is missing the
isSymbolicLink() method that exists on the fs.ts twin, so callers treating sync
and async Stats interchangeably can fail. Update the Stats class to include an
isSymbolicLink() method with the same behavior as the corresponding
implementation in fs.ts, and keep its shape aligned with the other Stats helpers
(isFile and isDirectory) so both wrappers expose the same API.
In `@vite.config.ts`:
- Around line 29-35: The worker bundle in the Vite config relies on the
Environment API resolution path, which may change across Vite upgrades. Update
the worker-specific setup in vite.config.ts around the worker.plugins/format
configuration to either add a safe fallback resolution path or constrain the
supported Vite version range for this build so the playground language server
worker remains stable. Refer to the worker bundle configuration and the
markoLspResolve/markoLspAssets plugin setup when making the change.
---
Nitpick comments:
In `@src/util/lsp/assets-plugin.ts`:
- Around line 66-77: `markoLspAssets()` is calling `collectAssets()` inside
`load()` every time the virtual module is loaded, which repeatedly reads the TS
lib and Marko type files from disk. Memoize the collected assets once per plugin
instance by caching the `collectAssets()` result within `markoLspAssets()` and
have `load()` reuse that cached value when returning the virtual module export.
- Around line 84-89: The asset seeding in collectAssets() is pulling in every
lib.*.d.ts from the TypeScript lib directory, which bloats the bundle beyond the
fixed tsconfig needs. Update collectAssets() (and the related asset-loading
logic around the referenced shared code) to include only the libs actually
required by the generated /tsconfig.json, namely the configured
DOM/DOM.Iterable/ESNext set plus any transitive dependencies, or compute and
cache that minimal set once instead of scanning all variants.
In `@src/util/lsp/node-shims/fs-promises.ts`:
- Around line 12-23: The `Stats` and `fileNotFound` logic is duplicated between
`fs.ts` and `fs-promises.ts`, so extract the shared `Stats` class and
`fileNotFound` helper into a common module (for example, a `shared` shim) and
have both `fs.ts` and `fs-promises.ts` import from it. Keep the existing `Stats`
behavior, including `isFile` and `isDirectory`, but remove the duplicate local
definitions so both shims stay consistent.
In `@src/util/lsp/server.ts`:
- Around line 89-101: The RPC message types are duplicated between client.ts and
server.ts and can drift apart. Move the source of truth for RequestMethod,
ServerMessage, and ClientMessage to server.ts and have client.ts import them
with a type-only import. Update any client-side local unions to use the shared
types so both sides stay compiler-checked against the same protocol definitions,
including the extra methods like findReferences and getColorPresentations.
In `@src/util/lsp/vfs.ts`:
- Around line 36-61: The directory listing helpers in vfs.ts are only using the
files map, so empty directories tracked by the directories set are omitted from
readDirectory and getDirectories. Update these functions to merge entries from
both files and directories when computing immediate children, using normalize
and ensureTrailingSlash consistently, and keep the existing de-duplication
behavior via the seen set so directoryExists and the child listing APIs stay in
sync.
- Around line 20-22: `deleteFile` currently removes the file from `files` but
leaves ancestor entries behind in `directories`, so stale empty directories can
still be reported by `directoryExists` and `getDirectories`. Update `deleteFile`
in `vfs.ts` to prune any now-empty ancestor directories after deleting the
normalized path, and make sure the cleanup stays consistent with how
`writeFile`/`registerDirs` adds directories and how
`getDirectories`/`directoryExists` read them.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ddf9750-87a2-4972-94d0-f65ad12062bb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (21)
cspell.jsonpackage.jsonsrc/routes/playground/tags/playground/tags/editor/editor.markosrc/routes/playground/tags/playground/tags/editor/editor.styles.module.scsssrc/types/stubs.d.tssrc/util/codemirror.tssrc/util/lsp/assets-plugin.tssrc/util/lsp/client.tssrc/util/lsp/codemirror.tssrc/util/lsp/globals.tssrc/util/lsp/node-shims/fs-promises.tssrc/util/lsp/node-shims/fs.tssrc/util/lsp/node-shims/module.tssrc/util/lsp/node-shims/url.tssrc/util/lsp/project-defaults.tssrc/util/lsp/server.tssrc/util/lsp/server.worker.tssrc/util/lsp/system.tssrc/util/lsp/vfs.tssrc/util/lsp/worker-error-report.tsvite.config.ts
| if (!g.process) { | ||
| g.process = { | ||
| browser: true, | ||
| platform: "browser", | ||
| env, | ||
| cwd: () => "/", | ||
| argv: [], | ||
| nextTick: (cb, ...args) => queueMicrotask(() => cb(...args)), | ||
| }; | ||
| } else { | ||
| g.process.browser ??= true; | ||
| g.process.env = { ...env, ...g.process.env }; | ||
| g.process.cwd ??= () => "/"; | ||
| g.process.nextTick ??= (cb, ...args) => queueMicrotask(() => cb(...args)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
process.platform isn't backfilled when process already exists.
The fresh-process branch sets platform: "browser", but the existing-process branch never assigns g.process.platform. If some other code has already defined a partial globalThis.process (e.g. a bundler polyfill) before this module runs, process.platform stays undefined instead of defaulting consistently.
🩹 Proposed fix
} else {
g.process.browser ??= true;
+ g.process.platform ??= "browser";
g.process.env = { ...env, ...g.process.env };
g.process.cwd ??= () => "/";
g.process.nextTick ??= (cb, ...args) => queueMicrotask(() => cb(...args));
}📝 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.
| if (!g.process) { | |
| g.process = { | |
| browser: true, | |
| platform: "browser", | |
| env, | |
| cwd: () => "/", | |
| argv: [], | |
| nextTick: (cb, ...args) => queueMicrotask(() => cb(...args)), | |
| }; | |
| } else { | |
| g.process.browser ??= true; | |
| g.process.env = { ...env, ...g.process.env }; | |
| g.process.cwd ??= () => "/"; | |
| g.process.nextTick ??= (cb, ...args) => queueMicrotask(() => cb(...args)); | |
| } | |
| if (!g.process) { | |
| g.process = { | |
| browser: true, | |
| platform: "browser", | |
| env, | |
| cwd: () => "/", | |
| argv: [], | |
| nextTick: (cb, ...args) => queueMicrotask(() => cb(...args)), | |
| }; | |
| } else { | |
| g.process.browser ??= true; | |
| g.process.platform ??= "browser"; | |
| g.process.env = { ...env, ...g.process.env }; | |
| g.process.cwd ??= () => "/"; | |
| g.process.nextTick ??= (cb, ...args) => queueMicrotask(() => cb(...args)); | |
| } |
🤖 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/util/lsp/globals.ts` around lines 34 - 48, The existing-process branch in
globals.ts is not backfilling process.platform, so a preexisting partial
globalThis.process can leave platform undefined. Update the globals
initialization logic around g.process to assign a default platform of "browser"
when it is missing, matching the fresh-process branch behavior, alongside the
other fallback fields like browser, env, cwd, and nextTick.
| class Stats { | ||
| constructor(private readonly kind: "file" | "dir") {} | ||
| mtimeMs = 0; | ||
| ctimeMs = 0; | ||
| size = 0; | ||
| isFile() { | ||
| return this.kind === "file"; | ||
| } | ||
| isDirectory() { | ||
| return this.kind === "dir"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stats here is missing isSymbolicLink(), present in the fs.ts twin.
If any caller treats sync/promise Stats interchangeably (a common pattern for dual sync/async fs wrappers), calling .isSymbolicLink() on this Stats will throw TypeError.
🩹 Proposed fix
class Stats {
constructor(private readonly kind: "file" | "dir") {}
mtimeMs = 0;
ctimeMs = 0;
size = 0;
isFile() {
return this.kind === "file";
}
isDirectory() {
return this.kind === "dir";
}
+ isSymbolicLink() {
+ return false;
+ }
}📝 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.
| class Stats { | |
| constructor(private readonly kind: "file" | "dir") {} | |
| mtimeMs = 0; | |
| ctimeMs = 0; | |
| size = 0; | |
| isFile() { | |
| return this.kind === "file"; | |
| } | |
| isDirectory() { | |
| return this.kind === "dir"; | |
| } | |
| } | |
| class Stats { | |
| constructor(private readonly kind: "file" | "dir") {} | |
| mtimeMs = 0; | |
| ctimeMs = 0; | |
| size = 0; | |
| isFile() { | |
| return this.kind === "file"; | |
| } | |
| isDirectory() { | |
| return this.kind === "dir"; | |
| } | |
| isSymbolicLink() { | |
| return false; | |
| } | |
| } |
🤖 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/util/lsp/node-shims/fs-promises.ts` around lines 12 - 23, The Stats shim
in fs-promises is missing the isSymbolicLink() method that exists on the fs.ts
twin, so callers treating sync and async Stats interchangeably can fail. Update
the Stats class to include an isSymbolicLink() method with the same behavior as
the corresponding implementation in fs.ts, and keep its shape aligned with the
other Stats helpers (isFile and isDirectory) so both wrappers expose the same
API.
4f99700 to
ed9b140
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util/lsp/assets-plugin.ts (1)
31-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
OPTIMIZE_SHIMSfromWORKER_SHIMSto avoid duplication.The four
fs/fs/promisesentries are hand-duplicated across both maps. If a shim file is ever renamed or a new fs-family alias added, it's easy to update one map and forget the other, silently breaking either the worker build or the dep-optimizer pre-bundle.♻️ Proposed refactor
-const OPTIMIZE_SHIMS: Record<string, string> = { - fs: shim("fs.ts"), - "node:fs": shim("fs.ts"), - "fs/promises": shim("fs-promises.ts"), - "node:fs/promises": shim("fs-promises.ts"), -}; +const OPTIMIZE_SHIMS: Record<string, string> = { + fs: WORKER_SHIMS.fs, + "node:fs": WORKER_SHIMS["node:fs"], + "fs/promises": WORKER_SHIMS["fs/promises"], + "node:fs/promises": WORKER_SHIMS["node:fs/promises"], +};Also applies to: 71-76
🤖 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/util/lsp/assets-plugin.ts` around lines 31 - 40, The `WORKER_SHIMS` and `OPTIMIZE_SHIMS` maps are duplicating the same `fs`/`fs/promises` shim entries, so update `assets-plugin.ts` to derive `OPTIMIZE_SHIMS` from `WORKER_SHIMS` instead of maintaining two separate copies. Use the existing `WORKER_SHIMS` symbol as the single source of truth and build the optimizer mapping from it, preserving the intended subset of aliases while keeping the worker build and dep-optimizer pre-bundle in sync.
🤖 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/util/lsp/codemirror.ts`:
- Around line 354-356: The markedToHtml helper currently returns unsanitized
HTML from marked.parse, which is then reused by the hover and completion-info
renderers. Update markedToHtml to sanitize the parsed markdown output before
returning it, using a sanitizer such as DOMPurify, so any LSP-provided
documentation is cleaned before it can reach innerHTML. Keep the fix centered in
markedToHtml so both call sites are covered automatically.
---
Nitpick comments:
In `@src/util/lsp/assets-plugin.ts`:
- Around line 31-40: The `WORKER_SHIMS` and `OPTIMIZE_SHIMS` maps are
duplicating the same `fs`/`fs/promises` shim entries, so update
`assets-plugin.ts` to derive `OPTIMIZE_SHIMS` from `WORKER_SHIMS` instead of
maintaining two separate copies. Use the existing `WORKER_SHIMS` symbol as the
single source of truth and build the optimizer mapping from it, preserving the
intended subset of aliases while keeping the worker build and dep-optimizer
pre-bundle in sync.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c71dffe-a3d0-40e1-99b9-3b66f421866b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (22)
cspell.jsonpackage.jsonsrc/routes/playground/tags/playground/tags/editor/editor.markosrc/routes/playground/tags/playground/tags/editor/editor.styles.module.scsssrc/types/stubs.d.tssrc/util/codemirror.tssrc/util/lsp/assets-plugin.tssrc/util/lsp/client.tssrc/util/lsp/codemirror.tssrc/util/lsp/globals.tssrc/util/lsp/node-shims/fs-promises.tssrc/util/lsp/node-shims/fs.tssrc/util/lsp/node-shims/module.tssrc/util/lsp/node-shims/url.tssrc/util/lsp/project-defaults.tssrc/util/lsp/server.tssrc/util/lsp/server.worker.tssrc/util/lsp/system.tssrc/util/lsp/vfs.tssrc/util/lsp/worker-error-report.tssrc/util/workspace.tsvite.config.ts
✅ Files skipped from review due to trivial changes (2)
- cspell.json
- src/types/stubs.d.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/util/lsp/node-shims/module.ts
- src/util/lsp/system.ts
- src/util/lsp/worker-error-report.ts
- src/util/lsp/server.worker.ts
- src/util/lsp/node-shims/url.ts
- vite.config.ts
- src/util/lsp/node-shims/fs-promises.ts
- src/util/codemirror.ts
- src/util/lsp/project-defaults.ts
- src/util/lsp/globals.ts
- src/routes/playground/tags/playground/tags/editor/editor.styles.module.scss
- src/util/lsp/vfs.ts
- src/util/lsp/server.ts
- src/util/lsp/node-shims/fs.ts
- src/routes/playground/tags/playground/tags/editor/editor.marko
- src/util/lsp/client.ts
- package.json
| function markedToHtml(md: string): string { | ||
| return marked.parse(md, { async: false }) as string; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the marked library sanitize HTML output by default in its current version?
💡 Result:
No, the marked library does not sanitize HTML output by default [1][2]. The library explicitly warns users that it does not perform sanitization and strongly recommends that developers use a third-party sanitization library—such as DOMPurify (recommended), sanitize-html, or insane—on the output generated by marked to prevent Cross-Site Scripting (XSS) attacks [1][3][4]. Older versions of the library included sanitize and sanitizer options, but these were removed in version 8.0.0 [5]. Maintaining the library's focus on performance and extensibility, the maintainers advise handling security via external tools or by using the postprocess hook to integrate a sanitizer directly into the marked pipeline [6][7].
Citations:
- 1: https://marked.js.org/
- 2: https://registry.npmjs.org/marked
- 3: https://github.com/chjj/marked/
- 4: https://github.com/markedjs/marked/blob/v18.0.0/docs/INDEX.md
- 5: https://github.com/markedjs/marked/blob/master/docs/USING_ADVANCED.md
- 6: Add optional
sanitizeroption for easier secure rendering with DOMPurify markedjs/marked#3943 - 7: Do I need to use sanitize the output for emails? markedjs/marked#2830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant regions around markedToHtml and the innerHTML sinks.
wc -l src/util/lsp/codemirror.ts
sed -n '200,380p' src/util/lsp/codemirror.ts
# Look for any sanitization already applied elsewhere in the file or repo.
rg -n "DOMPurify|sanitize\(|innerHTML|marked\.parse|markedToHtml" src/util/lsp/codemirror.ts src -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'Repository: marko-js/website
Length of output: 7020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/util/lsp/codemirror.ts
sed -n '200,380p' src/util/lsp/codemirror.ts
rg -n "DOMPurify|sanitize\(|innerHTML|marked\.parse|markedToHtml" src/util/lsp/codemirror.ts src -g'*.ts' -g'*.tsx' -g'*.js' -g'*.jsx'Repository: marko-js/website
Length of output: 7020
Sanitize markdown output before it reaches innerHTML.
marked.parse() preserves raw HTML, so both the hover and completion-info renderers can inject attacker-controlled markup from LSP documentation into the page. Sanitize the result here, for example with DOMPurify, so both sinks are covered.
🤖 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/util/lsp/codemirror.ts` around lines 354 - 356, The markedToHtml helper
currently returns unsanitized HTML from marked.parse, which is then reused by
the hover and completion-info renderers. Update markedToHtml to sanitize the
parsed markdown output before returning it, using a sanitizer such as DOMPurify,
so any LSP-provided documentation is cleaned before it can reach innerHTML. Keep
the fix centered in markedToHtml so both call sites are covered automatically.
Source: Linters/SAST tools
ed9b140 to
df1e18d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/util/lsp/assets-plugin.ts (2)
31-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fs-shim mapping between
WORKER_SHIMSandOPTIMIZE_SHIMS.The
fs/node:fs/fs/promises/node:fs/promisesentries are duplicated verbatim across the two maps. If a shim path or filename changes, it's easy to update one and miss the other, causing the optimizer and the regular resolver to diverge silently.♻️ Suggested consolidation
+const FS_SHIMS: Record<string, string> = { + fs: shim("fs.ts"), + "node:fs": shim("fs.ts"), + "fs/promises": shim("fs-promises.ts"), + "node:fs/promises": shim("fs-promises.ts"), +}; + const WORKER_SHIMS: Record<string, string> = { - fs: shim("fs.ts"), - "node:fs": shim("fs.ts"), - "fs/promises": shim("fs-promises.ts"), - "node:fs/promises": shim("fs-promises.ts"), + ...FS_SHIMS, url: shim("url.ts"), "node:url": shim("url.ts"), module: shim("module.ts"), "node:module": shim("module.ts"), }; ... -const OPTIMIZE_SHIMS: Record<string, string> = { - fs: shim("fs.ts"), - "node:fs": shim("fs.ts"), - "fs/promises": shim("fs-promises.ts"), - "node:fs/promises": shim("fs-promises.ts"), -}; +const OPTIMIZE_SHIMS: Record<string, string> = FS_SHIMS;Also applies to: 71-76
🤖 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/util/lsp/assets-plugin.ts` around lines 31 - 40, The `WORKER_SHIMS` and `OPTIMIZE_SHIMS` maps duplicate the same `fs`-related entries, which risks them drifting apart. Consolidate the shared `fs`/`node:fs`/`fs/promises`/`node:fs/promises` shim definitions into a single reusable source (for example a shared constant or helper) and have both maps reference it, keeping the existing `WORKER_SHIMS` and `OPTIMIZE_SHIMS` symbols in sync.
97-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
collectAssets()re-reads the disk on every virtual-module load with no caching.
load()callscollectAssets()unconditionally, which does several synchronousreaddirSync/readFileSynccalls (including scanning the entire TSlib/and Markotags/directories) each time the module is requested. Since these are static files that don't change during a dev session, this repeated I/O is unnecessary — memoize the result once and reuse it acrossload()invocations.⚡ Suggested caching
+let cachedAssets: Record<string, string> | undefined; + export function markoLspAssets(): Plugin { return { name: "marko-lsp-assets", resolveId(id) { if (id === VIRTUAL_ID) return "\0" + VIRTUAL_ID; }, load(id) { if (id !== "\0" + VIRTUAL_ID) return; - return `export default ${JSON.stringify(collectAssets())};`; + cachedAssets ??= collectAssets(); + return `export default ${JSON.stringify(cachedAssets)};`; }, }; }Also applies to: 110-185
🤖 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/util/lsp/assets-plugin.ts` around lines 97 - 108, `markoLspAssets()` is re-running the expensive `collectAssets()` scan on every `load()` of the virtual module. Cache the result once inside the plugin closure and have `load()` reuse that memoized value for `VIRTUAL_ID` instead of re-reading the filesystem each time. Keep the change localized to `markoLspAssets`, `load()`, and `collectAssets()` so the static asset list is computed only once per plugin instance.
🤖 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.
Nitpick comments:
In `@src/util/lsp/assets-plugin.ts`:
- Around line 31-40: The `WORKER_SHIMS` and `OPTIMIZE_SHIMS` maps duplicate the
same `fs`-related entries, which risks them drifting apart. Consolidate the
shared `fs`/`node:fs`/`fs/promises`/`node:fs/promises` shim definitions into a
single reusable source (for example a shared constant or helper) and have both
maps reference it, keeping the existing `WORKER_SHIMS` and `OPTIMIZE_SHIMS`
symbols in sync.
- Around line 97-108: `markoLspAssets()` is re-running the expensive
`collectAssets()` scan on every `load()` of the virtual module. Cache the result
once inside the plugin closure and have `load()` reuse that memoized value for
`VIRTUAL_ID` instead of re-reading the filesystem each time. Keep the change
localized to `markoLspAssets`, `load()`, and `collectAssets()` so the static
asset list is computed only once per plugin instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6351a0c5-7ec5-442b-9339-f8d29ea13baf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (22)
cspell.jsonpackage.jsonsrc/routes/playground/tags/playground/tags/editor/editor.markosrc/routes/playground/tags/playground/tags/editor/editor.styles.module.scsssrc/types/stubs.d.tssrc/util/codemirror.tssrc/util/lsp/assets-plugin.tssrc/util/lsp/client.tssrc/util/lsp/codemirror.tssrc/util/lsp/globals.tssrc/util/lsp/node-shims/fs-promises.tssrc/util/lsp/node-shims/fs.tssrc/util/lsp/node-shims/module.tssrc/util/lsp/node-shims/url.tssrc/util/lsp/project-defaults.tssrc/util/lsp/server.tssrc/util/lsp/server.worker.tssrc/util/lsp/system.tssrc/util/lsp/vfs.tssrc/util/lsp/worker-error-report.tssrc/util/workspace.tsvite.config.ts
✅ Files skipped from review due to trivial changes (4)
- src/types/stubs.d.ts
- cspell.json
- src/routes/playground/tags/playground/tags/editor/editor.styles.module.scss
- src/util/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/util/lsp/node-shims/module.ts
- src/util/lsp/server.worker.ts
- src/util/lsp/globals.ts
- src/util/lsp/worker-error-report.ts
- package.json
- src/util/lsp/project-defaults.ts
- src/util/lsp/node-shims/url.ts
- src/util/lsp/system.ts
- src/util/codemirror.ts
- vite.config.ts
- src/util/lsp/server.ts
- src/util/lsp/node-shims/fs.ts
- src/util/lsp/vfs.ts
- src/util/lsp/codemirror.ts
- src/util/lsp/node-shims/fs-promises.ts
- src/routes/playground/tags/playground/tags/editor/editor.marko
- src/util/lsp/client.ts
Give the playground editor real diagnostics, hover, completion and
go-to-definition for `.marko`, `.ts`/`.js` and `.css` by running the Marko
language server (`@marko/language-server`) in a Web Worker.
The website owns the browser build: `src/util/lsp/server.ts` assembles the
embedded services from `@marko/language-server/browser` and supplies the
environment -- a virtual filesystem (`vfs.ts`), a `ts.System` over it
(`system.ts`), Node-builtin shims (`node-shims/`), and the injected Marko
compiler (`project-defaults.ts`) -- then exposes them over a small JSON-RPC
to a main-thread client (`client.ts`) that syncs documents and drives
CodeMirror (`codemirror.ts`). A Vite plugin (`assets-plugin.ts`) seeds the
worker's virtual disk with TypeScript's `lib.*.d.ts` and the Marko type
definitions at build time, and maps the Node builtins the server touches to
browser shims (scoped to the client/worker so the Node server build is
untouched).
Custom tags resolve the way they do at runtime. The client keys documents on
the same `/tags/` root the build workspace uses, so Marko's tag discovery
treats sibling `.marko` files as mutually referenceable custom tags. The
compiler's taglib finder scans for them through its own `require("fs")`, which
the dependency optimizer would otherwise stub out, so that `fs` is shimmed
during pre-bundling too and the virtual disk lives on a `globalThis` singleton
(the pre-bundled compiler holds its own copy of the module and must see the
same files). A `<foo>` referencing a sibling `foo.marko` now gets tag-name and
attribute completion, attribute typing, hover, diagnostics and
go-to-definition.
A `.marko` file can also import a standalone `.ts` helper and run it: the
in-browser preview build resolves the extensionless import to its TypeScript
source and strips the types with the compiler's bundled Babel, so what the
language server type-checks is what the preview executes.
Completions open on trigger characters and apply auto-imports; hover popups
stay within the viewport; Cmd/Ctrl+click or F12 jumps to a definition,
switching tabs for a cross-file target. Quick fixes offered by the language
server appear in the diagnostic tooltip.
df1e18d to
d6c1c8f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/util/lsp/client.ts (1)
63-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWorker failures still only log;
#readyand pending requests hang.The annotated code shows the
error/messageerrorhandlers onlyconsole.error. A worker startup/import failure leaves the#readypromise (id 1) and every subsequent#request()permanently pending. This matches the previously flagged issue.Proposed fix
+ const failWorker = (reason: unknown) => { + for (const pending of this.#pending.values()) { + pending.reject(reason); + } + this.#pending.clear(); + }; - this.#worker.addEventListener("error", (e) => - console.error("[lsp] worker error", e.message, e.filename, e.lineno), - ); - this.#worker.addEventListener("messageerror", (e) => - console.error("[lsp] worker messageerror", e), - ); + this.#worker.addEventListener("error", (e) => { + console.error("[lsp] worker error", e.message, e.filename, e.lineno); + failWorker(new Error(e.message || "LSP worker failed")); + }); + this.#worker.addEventListener("messageerror", (e) => { + console.error("[lsp] worker messageerror", e); + failWorker(new Error("LSP worker message error")); + });🤖 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/util/lsp/client.ts` around lines 63 - 68, The worker error handlers in client.ts only log and leave `#ready` plus in-flight `#request()` calls hanging forever after a startup/import failure. Update the `#worker.addEventListener("error")` and `messageerror` handlers in `LspClient` to also fail the client state by rejecting `#ready` and any pending request promises, and make subsequent `#request()` calls reject immediately once the worker is in an errored state. Ensure the fix is wired through the same `LspClient`/`#worker` lifecycle used by `#ready` and `#request()`, not just the console logging path.
🤖 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.
Duplicate comments:
In `@src/util/lsp/client.ts`:
- Around line 63-68: The worker error handlers in client.ts only log and leave
`#ready` plus in-flight `#request()` calls hanging forever after a
startup/import failure. Update the `#worker.addEventListener("error")` and
`messageerror` handlers in `LspClient` to also fail the client state by
rejecting `#ready` and any pending request promises, and make subsequent
`#request()` calls reject immediately once the worker is in an errored state.
Ensure the fix is wired through the same `LspClient`/`#worker` lifecycle used by
`#ready` and `#request()`, not just the console logging path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efe5b84e-3de2-4dc6-899e-05d6d38e1fca
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (24)
cspell.jsonpackage.jsonsrc/routes/playground/tags/playground/tags/editor/editor.markosrc/routes/playground/tags/playground/tags/editor/editor.styles.module.scsssrc/types/stubs.d.tssrc/util/codemirror.tssrc/util/lsp/assets-plugin.tssrc/util/lsp/client.tssrc/util/lsp/codemirror.tssrc/util/lsp/globals.tssrc/util/lsp/node-shims/fs-promises.tssrc/util/lsp/node-shims/fs.tssrc/util/lsp/node-shims/module.tssrc/util/lsp/node-shims/url.tssrc/util/lsp/project-defaults.tssrc/util/lsp/server.tssrc/util/lsp/server.worker.tssrc/util/lsp/system.tssrc/util/lsp/vfs.tssrc/util/lsp/worker-error-report.tssrc/util/workspace.tssrc/util/workspace/main-plugin.tssrc/util/workspace/script-plugin.tsvite.config.ts
✅ Files skipped from review due to trivial changes (2)
- cspell.json
- src/types/stubs.d.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/util/lsp/worker-error-report.ts
- src/util/lsp/node-shims/module.ts
- src/util/lsp/node-shims/url.ts
- src/routes/playground/tags/playground/tags/editor/editor.styles.module.scss
- src/util/lsp/server.worker.ts
- src/util/lsp/system.ts
- src/util/lsp/globals.ts
- src/util/lsp/node-shims/fs-promises.ts
- src/util/lsp/project-defaults.ts
- package.json
- src/util/lsp/vfs.ts
- src/util/lsp/assets-plugin.ts
- src/util/lsp/server.ts
- src/util/codemirror.ts
- src/util/lsp/node-shims/fs.ts
- src/routes/playground/tags/playground/tags/editor/editor.marko
- src/util/lsp/codemirror.ts
- vite.config.ts
1 participant
Runs the Marko language server (
@marko/language-server@3.3.1) in a Web Worker so the playground editor has real language intelligence for.marko,.ts/.js, and.css.What you get
.,<,@,/) and applies auto-imports on acceptHow it works
The website owns the browser glue.
src/util/lsp/server.tsassembles the embedded services from@marko/language-server/browserand fills the environment seams (a virtual filesystem, an injectablets.Systemover it, Node-builtin shims, and the injected Marko compiler), then exposes them over a small JSON-RPC to a main-thread client that syncs documents and drives CodeMirror. A Vite plugin seeds the worker's virtual disk with TypeScript'slib.*.d.tsand the Marko type definitions at build time, scoped so the Node-builtin shims never leak into the client or server build.Depends on
@marko/language-server@3.3.1, which adds the./browserentry and standalone.ts/.jssupport.Generated by Claude Code