Add a Monaco code editor surface (cmux edit) - #5638
lawrencecchen wants to merge 15 commits into
Conversation
Adds an `/editor` webviews surface backed by Monaco, as its own lazy `monaco-vendor` chunk (groundwork from the surface-split PR #5613): - `EditorApp` mounts Monaco via a React 19 callback ref (no useEffect), disposing editor + model on unmount. - Theme derived from the live cmux appearance (light + dark via defineTheme), strictly better than the dark-only reference PR. - One base editor.worker; editor.api + basic-languages Monarch highlighting on the main thread (no language-service workers / IntelliSense yet). - Build: base "./" for relative asset URLs (Monaco worker/font), monaco-vendor manualChunk, ES-module worker, main.tsx dispatcher + `/editor` route. Builds, typechecks, lints. NOT yet dogfoodable: the spike confirmed Monaco emits external CSS + codicon font + a worker, and the diff viewer custom scheme only registers .js/.mjs, so serving needs (1) runtime CSS injection, (2) CLI css/ttf enumeration + scheme MIME allowlist, (3) stable worker filename, (4) a Debug-menu open seam. See plans/feat-monaco-editor/DESIGN.md (hq) for the plan.
The webviews build has no HTML entry, so Vite does not auto-link monaco-vendor.css. The editor surface now links it at runtime, resolved relative to its own chunk URL (independent of where the host page is served); the CSS references its font as relative url(./codicon.ttf). Verified over plain HTTP (headless Chrome) that the editor mounts, applies the cmux dark theme, syntax-highlights TypeScript on the main thread, and spawns the base editor.worker without errors.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR integrates Monaco editor into the webviews application with build configuration, runtime environment setup, React components, 40+ language grammar definitions, webview routing, and a CLI ChangesMonaco editor webview integration
Sequence Diagram(s)sequenceDiagram
participant main as webviews/src/main.tsx
participant router as TanStack Router
participant surface as editorSurface
participant env as monacoEnvironment
participant editor as EditorApp
main->>main: resolveWebviewKind() → "editor"
main->>surface: dynamic import, call mountEditorSurface()
surface->>env: install globalThis.MonacoEnvironment
surface->>surface: injectMonacoStylesheet()
surface->>surface: preloadGrammarForPath(filePath)
surface->>router: createRoot, render RouterProvider
router->>editor: render EditorApp with theme/config
editor->>env: create model and editor instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge — the new cmux edit path reuses well-tested diff-viewer infrastructure with no new trust boundaries or serving changes beyond adding CSS to an already-guarded MIME allowlist. All changed paths are either CLI-side file prep or incremental extensions to the existing diff-viewer asset pipeline. File content is escaped through the battle-tested jsonScriptLiteral helper; the deflate/inflate round-trip is atomic and idempotent; the Monaco callback-ref cleanup is correct for React 19; React Compiler is enabled, preventing inline-object ref churn. The only finding is a stale JSDoc on preloadGrammarForPath. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as cmux CLI
participant FS as Filesystem tmp
participant App as cmux App
participant WKWebView
participant EditorSurface as editorSurface.tsx
User->>CLI: cmux edit file
CLI->>FS: Read file UTF-8
CLI->>FS: writeEditorHTML content via jsonScriptLiteral
CLI->>FS: ensureDiffViewerAssets copy and inflate deflate chunks
CLI->>App: browser.open_split with url diff_viewer_token diff_viewer_files
App->>WKWebView: Load editor HTML via custom scheme
WKWebView->>App: CmuxDiffViewerURLSchemeHandler serves mjs and css assets
WKWebView->>EditorSurface: main.mjs calls mountEditorSurface
EditorSurface->>WKWebView: fetch monaco-vendor.css then inject style
EditorSurface->>EditorSurface: preloadGrammarForPath registers Monarch grammar eagerly
EditorSurface->>EditorSurface: defineMonacoThemes dark and light
EditorSurface->>WKWebView: createRoot renders EditorApp
EditorSurface->>EditorSurface: forceTokenization then render true
Reviews (7): Last reviewed commit: "Address review-bot feedback on cmux edit" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Monaco's `?worker` import builds a worker bundle. Emit it as an ES module | ||
| // worker with a stable name (no hash) so `new Worker(url, {type:"module"})` | ||
| // works and the file overwrites in place in the diff viewer asset cache, | ||
| // matching the main bundle's stable-name policy above. | ||
| worker: { | ||
| format: "es", | ||
| rollupOptions: { | ||
| output: { | ||
| entryFileNames: "chunks/[name].mjs", | ||
| chunkFileNames: "chunks/[name].mjs", | ||
| assetFileNames: "assets/[name][extname]", | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The comment on lines 40-43 says the worker will be emitted as "an ES module worker with a stable name (no hash)" and names the expected path pattern chunks/[name].mjs, but the built artifact is assets/editor.worker-CdQrwHl8.js — in assets/, not chunks/, with a content hash appended, and the file body is IIFE format ((function(){"use strict";...})), not an ES module. The worker.rollupOptions.output directives are silently not applied by this version of Vite.
The PR description explicitly notes the stable-name goal is a follow-up, but the code comment presents the goal as already achieved. Until the follow-up lands, each rebuild produces a differently-named file, and the /tmp diff-viewer cache accumulates orphaned worker copies rather than overwriting in place as the comment implies.
| /** | ||
| * Boots the Monaco editor surface: reads its injected config, registers | ||
| * cmux-derived themes, then renders `EditorApp` through the shared router. | ||
| * Loaded as its own lazy chunk so other surfaces never pay for Monaco. | ||
| */ | ||
| /** | ||
| * Links Monaco's emitted stylesheet. The webviews build has no HTML entry, so | ||
| * Vite does not inject `monaco-vendor.css` automatically. The CSS sits next to | ||
| * the chunks (`assets/monaco-vendor.css`), and its font is a relative | ||
| * `url(./codicon.ttf)`, so resolving the link href from this chunk's own URL | ||
| * works regardless of where the host page is served. | ||
| */ | ||
| function linkMonacoStylesheet(): void { |
There was a problem hiding this comment.
mountEditorSurface but precedes linkMonacoStylesheet
There are two consecutive JSDoc comments before linkMonacoStylesheet. The first one ("Boots the Monaco editor surface…") clearly belongs on mountEditorSurface, which is left undocumented. The second JSDoc correctly describes linkMonacoStylesheet.
| /** | |
| * Boots the Monaco editor surface: reads its injected config, registers | |
| * cmux-derived themes, then renders `EditorApp` through the shared router. | |
| * Loaded as its own lazy chunk so other surfaces never pay for Monaco. | |
| */ | |
| /** | |
| * Links Monaco's emitted stylesheet. The webviews build has no HTML entry, so | |
| * Vite does not inject `monaco-vendor.css` automatically. The CSS sits next to | |
| * the chunks (`assets/monaco-vendor.css`), and its font is a relative | |
| * `url(./codicon.ttf)`, so resolving the link href from this chunk's own URL | |
| * works regardless of where the host page is served. | |
| */ | |
| function linkMonacoStylesheet(): void { | |
| /** | |
| * Links Monaco's emitted stylesheet. The webviews build has no HTML entry, so | |
| * Vite does not inject `monaco-vendor.css` automatically. The CSS sits next to | |
| * the chunks (`assets/monaco-vendor.css`), and its font is a relative | |
| * `url(./codicon.ttf)`, so resolving the link href from this chunk's own URL | |
| * works regardless of where the host page is served. | |
| */ | |
| function linkMonacoStylesheet(): void { |
| if (!element?.textContent) { | ||
| throw new Error("Missing cmux editor config"); | ||
| } | ||
| return JSON.parse(element.textContent); |
There was a problem hiding this comment.
JSON.parse in readConfig will crash the surface with an opaque error
If the cmux-editor-config element exists but contains malformed JSON (e.g. a partial write during Swift-side injection), JSON.parse throws and the promise-chain in main.tsx never catches it, leaving the webview silently broken. A try/catch with a clear message makes diagnosing this much easier.
| return JSON.parse(element.textContent); | |
| try { | |
| return JSON.parse(element.textContent); | |
| } catch { | |
| throw new Error("cmux editor config is not valid JSON"); | |
| } |
Makes the Monaco editor openable in the app, served through the diff viewer custom scheme (so the module worker works), with the file content + live cmux appearance injected as config. - CLI: `cmux edit <file>` writes an `editor` webviews page, registers the bundled assets, and opens a webview via `browser.open_split` (reusing the diff viewer token/allowlist flow). `writeEditor` / `writeEditorHTML` mirror the lean diff path. - Serving: include `.css` in the bundled-asset enumeration and register it as `text/css`; add `text/css` to the scheme handler's MIME allowlist. The editor surface fetches `monaco-vendor.css` and injects it as an inline `<style>` (allowed by the page CSP's `connect-src 'self'` + `style-src 'unsafe-inline'`) instead of an external `<link>`, so no CSP change is needed. The codicon font is skipped for v1 (core editing/highlighting does not need it). The worker (.js, same-origin) is already served and allowed by `script-src 'self'`. No app-side surface/SurfaceRole changes: the editor rides the existing browser.open_split + custom-scheme path.
The live editor/diff path uses the local HTTP server (127.0.0.1), not the custom scheme. Its manifest validator rejected the whole token because `monaco-vendor.css` failed the HTTP server's MIME allowlist, 500-ing every request. Add text/css to `diffViewerHTTPIsAllowedMimeType` + `...PathExtensionMatchesMimeType`, mirroring the scheme handler change. The HTTP server sets no CSP, so the fetch+inline CSS path works there unchanged.
Shrinks the committed git tree + .app bundle ~14.5MB -> 3.9MB. The bloat is the diff-vendor (10.3MB) and monaco-vendor (3.7MB) chunks, which are served only through the diff viewer HTTP server / custom scheme, never the agent-session file:// load. The build deflates chunks >1MB to `<name>.deflate` (raw DEFLATE via node, matching Swift `NSData.decompressed(using:.zlib)`); the CLI inflates them on copy into the per-token serving dir, so the scheme handler, HTTP server, agent-session loading, and CSP are all unchanged. `build-webviews-app.sh --check` now compares inflated CONTENT for `.deflate` files so the reproducibility gate is immune to deflate byte-variance across environments/zlib versions.
…ense) Every common language already highlights via its basic-languages Monarch grammar. JSON is the one common language with no Monarch grammar, so add the JSON language service + its 383KB worker. Deliberately skip the CSS/HTML/ TypeScript language services: their Monarch grammars already highlight, and their workers (especially the ~7MB TypeScript worker) only add IntelliSense and would balloon the bundle the previous commit shrank.
The editor showed no highlighting (every line was one default token). Root cause: `basic-languages/_.contribution.js` only *defines* `registerLanguage`; it is not an all-languages aggregate, so no grammar was ever registered, and collapsing all of Monaco into one chunk also dropped the lazy grammar imports. Now register each common language via its own `*.contribution.js` (Go, Rust, Python, Java, C/C++, C#, Ruby, PHP, Swift, Kotlin, Scala, SQL, YAML, XML, HTML, CSS/SCSS/LESS, shell, Dockerfile, Markdown, TS/JS, JSON, and ~20 more). Each lazy-loads its Monarch grammar as its own chunk on first open. manualChunks keeps Monaco *core* collapsed but lets the grammars + JSON service split so their lazy `import()` works. Verified over HTTP: python/json now tokenize with distinct token colors.
The cmux WKWebView pane can report 0 height when the Monaco editor is created, and Monaco's `automaticLayout` observer occasionally misses that first sizing, leaving the editor showing zero lines even though the page is correct (the same URL renders fine at full size). Drive `editor.layout()` from a ResizeObserver on the container so the editor lays out as soon as the pane has a size.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa0692570a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fontFamily: appearance.fontFamily, | ||
| fontSize: appearance.fontSize, | ||
| lineHeight: appearance.lineHeight, | ||
| readOnly: Boolean(config.payload?.readOnly), |
There was a problem hiding this comment.
When cmux edit <file> opens this surface, the CLI injects only the file contents/path/title and no save endpoint or readOnly flag, so this line makes Monaco editable by default (Boolean(undefined) is false). In that scenario users can modify the buffer in the pane but no onDidChangeModelContent/save path writes those edits back to the original file, so closing or reloading the surface silently discards changes; default this to read-only (or wire persistence) until a real save flow exists.
Useful? React with 👍 / 👎.
![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
🤖 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 `@CLI/cmux_open.swift`:
- Around line 930-953: The argument parser loop for cmux edit silently accepts
malformed invocations (unknown flags, missing option values, extra positional
args, and filenames beginning with '-') so update the parsing in the while loop
that reads commandArgs (variables: index, arg, windowArg, workspaceArg,
surfaceArg, focus, filePathArg) to: 1) treat "--" as end-of-options so a
following arg starting with '-' becomes the filePathArg; 2) validate option
flags that require a value (--window, --workspace, --surface) and throw CLIError
with the Usage message if the value is missing; 3) reject any unknown flag (arg
starting with '-' that isn't one of the known flags) by throwing CLIError; and
4) reject additional positional arguments by throwing CLIError if filePathArg is
already set and another non-flag positional appears; ensure all error cases use
the existing CLIError(message:) pattern and keep focus handling as-is.
- Around line 956-960: The current use of try? String(contentsOf: fileURL,
encoding: .utf8) conflates I/O/read errors with encoding failures; change the
logic to explicitly perform a read that surfaces I/O errors (e.g., use try
Data(contentsOf: fileURL) or a do/catch around String(contentsOf:)) and then
convert the data to a UTF-8 string, throwing the existing CLIError only when the
conversion fails; update the code around the fileURL checks (the guard that
currently uses try? String(contentsOf:...)) so that read errors propagate and
only a nil String(data:..., encoding: .utf8) triggers the “not UTF-8” message.
- Around line 1001-1005: The JSON branch currently sets response["path"] =
editor.fileURL.path which exposes the temp/generated HTML path; change it to
return the original source file path used for editing (i.e. set response["path"]
to the original source file variable used elsewhere such as sourceFileURL.path
or inputFileURL.path), falling back to editor.fileURL.path only if that original
source variable is nil/unavailable; keep response, payload and the other keys
(response["url"], response["title"]) unchanged.
In `@Resources/markdown-viewer/webviews-app/chunks/hcl.mjs`:
- Line 1: The keywords array on the language object t contains entries with
trailing spaces ("if ", "else ", "endif ", "for ") so they never match; edit the
t.keywords list to remove the trailing spaces (use "if","else","endif","for") so
the tokenizer (tokenizer/root and terraform rules that reference "`@keywords`")
can properly recognize and highlight these keywords.
In `@webviews/src/surfaces/editorSurface.tsx`:
- Around line 43-44: The code reads and inlines CSS without verifying the HTTP
response; change the flow so you first await fetch(href) into response, then
check response.ok (and optionally that response.headers.get('content-type')
indicates text/css) before calling response.text(); if the response is not ok
(or content-type is unexpected) log/handle the error and skip injecting the body
to avoid inlining HTML error pages into the <style> — update the block around
the variables response, css, and href in editorSurface.tsx accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ca09602b-f408-4c6f-aa83-fcb17b173e31
📒 Files selected for processing (61)
CLI/cmux.swiftCLI/cmux_open.swiftResources/markdown-viewer/webviews-app/assets/json.worker-BoL8UZqY.jsResources/markdown-viewer/webviews-app/chunks/bat.mjsResources/markdown-viewer/webviews-app/chunks/clojure.mjsResources/markdown-viewer/webviews-app/chunks/coffee.mjsResources/markdown-viewer/webviews-app/chunks/cpp.mjsResources/markdown-viewer/webviews-app/chunks/csharp.mjsResources/markdown-viewer/webviews-app/chunks/css.mjsResources/markdown-viewer/webviews-app/chunks/dart.mjsResources/markdown-viewer/webviews-app/chunks/diff-vendor.mjsResources/markdown-viewer/webviews-app/chunks/diff-vendor.mjs.deflateResources/markdown-viewer/webviews-app/chunks/dockerfile.mjsResources/markdown-viewer/webviews-app/chunks/editorSurface.mjsResources/markdown-viewer/webviews-app/chunks/elixir.mjsResources/markdown-viewer/webviews-app/chunks/fsharp.mjsResources/markdown-viewer/webviews-app/chunks/go.mjsResources/markdown-viewer/webviews-app/chunks/graphql.mjsResources/markdown-viewer/webviews-app/chunks/handlebars.mjsResources/markdown-viewer/webviews-app/chunks/hcl.mjsResources/markdown-viewer/webviews-app/chunks/html.mjsResources/markdown-viewer/webviews-app/chunks/ini.mjsResources/markdown-viewer/webviews-app/chunks/java.mjsResources/markdown-viewer/webviews-app/chunks/javascript.mjsResources/markdown-viewer/webviews-app/chunks/jsonMode.mjsResources/markdown-viewer/webviews-app/chunks/julia.mjsResources/markdown-viewer/webviews-app/chunks/kotlin.mjsResources/markdown-viewer/webviews-app/chunks/less.mjsResources/markdown-viewer/webviews-app/chunks/lua.mjsResources/markdown-viewer/webviews-app/chunks/markdown.mjsResources/markdown-viewer/webviews-app/chunks/mdx.mjsResources/markdown-viewer/webviews-app/chunks/monaco-vendor.mjs.deflateResources/markdown-viewer/webviews-app/chunks/mysql.mjsResources/markdown-viewer/webviews-app/chunks/objective-c.mjsResources/markdown-viewer/webviews-app/chunks/perl.mjsResources/markdown-viewer/webviews-app/chunks/pgsql.mjsResources/markdown-viewer/webviews-app/chunks/php.mjsResources/markdown-viewer/webviews-app/chunks/powershell.mjsResources/markdown-viewer/webviews-app/chunks/protobuf.mjsResources/markdown-viewer/webviews-app/chunks/python.mjsResources/markdown-viewer/webviews-app/chunks/r.mjsResources/markdown-viewer/webviews-app/chunks/ruby.mjsResources/markdown-viewer/webviews-app/chunks/rust.mjsResources/markdown-viewer/webviews-app/chunks/scala.mjsResources/markdown-viewer/webviews-app/chunks/scss.mjsResources/markdown-viewer/webviews-app/chunks/shell.mjsResources/markdown-viewer/webviews-app/chunks/solidity.mjsResources/markdown-viewer/webviews-app/chunks/sql.mjsResources/markdown-viewer/webviews-app/chunks/swift.mjsResources/markdown-viewer/webviews-app/chunks/typescript.mjsResources/markdown-viewer/webviews-app/chunks/xml.mjsResources/markdown-viewer/webviews-app/chunks/yaml.mjsResources/markdown-viewer/webviews-app/main.mjsSources/Panels/BrowserPanel.swiftscripts/build-webviews-app.shwebviews/src/editor/EditorApp.tsxwebviews/src/editor/monacoEnvironment.tswebviews/src/editor/monacoLanguages.tswebviews/src/main.tsxwebviews/src/surfaces/editorSurface.tsxwebviews/vite.config.mjs
| @@ -0,0 +1 @@ | |||
| const e={comments:{lineComment:"#",blockComment:["/*","*/"]},brackets:[["{","}"],["[","]"],["(",")"]],autoClosingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"',notIn:["string"]}],surroundingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"'}]},t={defaultToken:"",tokenPostfix:".hcl",keywords:["var","local","path","for_each","any","string","number","bool","true","false","null","if ","else ","endif ","for ","in","endfor"],operators:["=",">=","<=","==","!=","+","-","*","/","%","&&","||","!","<",">","?","...",":"],symbols:/[=><!~?:&|+\-*\/\^%]+/,escapes:/\\(?:[abfnrtv\\"']|x[0-9A-Fa-f]{1,4}|u[0-9A-Fa-f]{4}|U[0-9A-Fa-f]{8})/,terraformFunctions:/(abs|ceil|floor|log|max|min|pow|signum|chomp|format|formatlist|indent|join|lower|regex|regexall|replace|split|strrev|substr|title|trimspace|upper|chunklist|coalesce|coalescelist|compact|concat|contains|distinct|element|flatten|index|keys|length|list|lookup|map|matchkeys|merge|range|reverse|setintersection|setproduct|setunion|slice|sort|transpose|values|zipmap|base64decode|base64encode|base64gzip|csvdecode|jsondecode|jsonencode|urlencode|yamldecode|yamlencode|abspath|dirname|pathexpand|basename|file|fileexists|fileset|filebase64|templatefile|formatdate|timeadd|timestamp|base64sha256|base64sha512|bcrypt|filebase64sha256|filebase64sha512|filemd5|filemd1|filesha256|filesha512|md5|rsadecrypt|sha1|sha256|sha512|uuid|uuidv5|cidrhost|cidrnetmask|cidrsubnet|tobool|tolist|tomap|tonumber|toset|tostring)/,terraformMainBlocks:/(module|data|terraform|resource|provider|variable|output|locals)/,tokenizer:{root:[[/^@terraformMainBlocks([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["type","","string","","string","","@brackets"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["identifier","","string","","string","","@brackets"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)(=)(\{)/,["identifier","","string","","operator","","@brackets"]],{include:"@terraform"}],terraform:[[/@terraformFunctions(\()/,["type","@brackets"]],[/[a-zA-Z_]\w*-*/,{cases:{"@keywords":{token:"keyword.$0"},"@default":"variable"}}],{include:"@whitespace"},{include:"@heredoc"},[/[{}()\[\]]/,"@brackets"],[/[<>](?!@symbols)/,"@brackets"],[/@symbols/,{cases:{"@operators":"operator","@default":""}}],[/\d*\d+[eE]([\-+]?\d+)?/,"number.float"],[/\d*\.\d+([eE][\-+]?\d+)?/,"number.float"],[/\d[\d']*/,"number"],[/\d/,"number"],[/[;,.]/,"delimiter"],[/"/,"string","@string"],[/'/,"invalid"]],heredoc:[[/<<[-]*\s*["]?([\w\-]+)["]?/,{token:"string.heredoc.delimiter",next:"@heredocBody.$1"}]],heredocBody:[[/([\w\-]+)$/,{cases:{"$1==$S2":[{token:"string.heredoc.delimiter",next:"@popall"}],"@default":"string.heredoc"}}],[/./,"string.heredoc"]],whitespace:[[/[ \t\r\n]+/,""],[/\/\*/,"comment","@comment"],[/\/\/.*$/,"comment"],[/#.*$/,"comment"]],comment:[[/[^\/*]+/,"comment"],[/\*\//,"comment","@pop"],[/[\/*]/,"comment"]],string:[[/\$\{/,{token:"delimiter",next:"@stringExpression"}],[/[^\\"\$]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","@popall"]],stringInsideExpression:[[/[^\\"]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","@pop"]],stringExpression:[[/\}/,{token:"delimiter",next:"@pop"}],[/"/,"string","@stringInsideExpression"],{include:"@terraform"}]}};export{e as conf,t as language}; | |||
There was a problem hiding this comment.
Fix unreachable HCL keywords caused by trailing spaces.
Line 1 defines "if ", "else ", "endif ", and "for " with trailing spaces, but identifiers are tokenized without trailing whitespace, so these entries never match and won’t be highlighted as keywords.
Suggested patch
-keywords:["var","local","path","for_each","any","string","number","bool","true","false","null","if ","else ","endif ","for ","in","endfor"],
+keywords:["var","local","path","for_each","any","string","number","bool","true","false","null","if","else","endif","for","in","endfor"],📝 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.
| const e={comments:{lineComment:"#",blockComment:["/*","*/"]},brackets:[["{","}"],["[","]"],["(",")"]],autoClosingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"',notIn:["string"]}],surroundingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"'}]},t={defaultToken:"",tokenPostfix:".hcl",keywords:["var","local","path","for_each","any","string","number","bool","true","false","null","if ","else ","endif ","for ","in","endfor"],operators:["=",">=","<=","==","!=","+","-","*","/","%","&&","||","!","<",">","?","...",":"],symbols:/[=><!~?:&|+\-*\/\^%]+/,escapes:/\\(?:[abfnrtv\\"']|x[0-9A-Fa-f]{1,4}|u[0-9A-Fa-f]{4}|U[0-9A-Fa-f]{8})/,terraformFunctions:/(abs|ceil|floor|log|max|min|pow|signum|chomp|format|formatlist|indent|join|lower|regex|regexall|replace|split|strrev|substr|title|trimspace|upper|chunklist|coalesce|coalescelist|compact|concat|contains|distinct|element|flatten|index|keys|length|list|lookup|map|matchkeys|merge|range|reverse|setintersection|setproduct|setunion|slice|sort|transpose|values|zipmap|base64decode|base64encode|base64gzip|csvdecode|jsondecode|jsonencode|urlencode|yamldecode|yamlencode|abspath|dirname|pathexpand|basename|file|fileexists|fileset|filebase64|templatefile|formatdate|timeadd|timestamp|base64sha256|base64sha512|bcrypt|filebase64sha256|filebase64sha512|filemd5|filemd1|filesha256|filesha512|md5|rsadecrypt|sha1|sha256|sha512|uuid|uuidv5|cidrhost|cidrnetmask|cidrsubnet|tobool|tolist|tomap|tonumber|toset|tostring)/,terraformMainBlocks:/(module|data|terraform|resource|provider|variable|output|locals)/,tokenizer:{root:[[/^@terraformMainBlocks([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["type","","string","","string","","@brackets"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["identifier","","string","","string","","@brackets"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)(=)(\{)/,["identifier","","string","","operator","","@brackets"]],{include:"@terraform"}],terraform:[[/@terraformFunctions(\()/,["type","@brackets"]],[/[a-zA-Z_]\w*-*/,{cases:{"@keywords":{token:"keyword.$0"},"@default":"variable"}}],{include:"@whitespace"},{include:"@heredoc"},[/[{}()\[\]]/,"@brackets"],[/[<>](?!@symbols)/,"@brackets"],[/@symbols/,{cases:{"@operators":"operator","@default":""}}],[/\d*\d+[eE]([\-+]?\d+)?/,"number.float"],[/\d*\.\d+([eE][\-+]?\d+)?/,"number.float"],[/\d[\d']*/,"number"],[/\d/,"number"],[/[;,.]/,"delimiter"],[/"/,"string","@string"],[/'/,"invalid"]],heredoc:[[/<<[-]*\s*["]?([\w\-]+)["]?/,{token:"string.heredoc.delimiter",next:"@heredocBody.$1"}]],heredocBody:[[/([\w\-]+)$/,{cases:{"$1==$S2":[{token:"string.heredoc.delimiter",next:"@popall"}],"@default":"string.heredoc"}}],[/./,"string.heredoc"]],whitespace:[[/[ \t\r\n]+/,""],[/\/\*/,"comment","@comment"],[/\/\/.*$/,"comment"],[/#.*$/,"comment"]],comment:[[/[^\/*]+/,"comment"],[/\*\//,"comment","@pop"],[/[\/*]/,"comment"]],string:[[/\$\{/,{token:"delimiter",next:"@stringExpression"}],[/[^\\"\$]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","@popall"]],stringInsideExpression:[[/[^\\"]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","@pop"]],stringExpression:[[/\}/,{token:"delimiter",next:"@pop"}],[/"/,"string","@stringInsideExpression"],{include:"@terraform"}]}};export{e as conf,t as language}; | |
| const e={comments:{lineComment:"#",blockComment:["/*","*/"]},brackets:[["{","}"],["[","]"],["(",")"]],autoClosingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"',notIn:["string"]}],surroundingPairs:[{open:"{",close:"}"},{open:"[",close:"]"},{open:"(",close:")"},{open:'"',close:'"'}]},t={defaultToken:"",tokenPostfix:".hcl",keywords:["var","local","path","for_each","any","string","number","bool","true","false","null","if","else","endif","for","in","endfor"],operators:["=",">=","<=","==","!=","+","-","*","/","%","&&","||","!","<",">","?","...",":"],symbols:/[=><!~?:&|+\-*\/\^%]+/,escapes:/\\(?:[abfnrtv\\"']|x[0-9A-Fa-f]{1,4}|u[0-9A-Fa-f]{4}|U[0-9A-Fa-f]{8})/,terraformFunctions:/(abs|ceil|floor|log|max|min|pow|signum|chomp|format|formatlist|indent|join|lower|regex|regexall|replace|split|strrev|substr|title|trimspace|upper|chunklist|coalesce|coalescelist|compact|concat|contains|distinct|element|flatten|index|keys|length|list|lookup|map|matchkeys|merge|range|reverse|setintersection|setproduct|setunion|slice|sort|transpose|values|zipmap|base64decode|base64encode|base64gzip|csvdecode|jsondecode|jsonencode|urlencode|yamldecode|yamlencode|abspath|dirname|pathexpand|basename|file|fileexists|fileset|filebase64|templatefile|formatdate|timeadd|timestamp|base64sha256|base64sha512|bcrypt|filebase64sha256|filebase64sha512|filemd5|filemd1|filesha256|filesha512|md5|rsadecrypt|sha1|sha256|sha512|uuid|uuidv5|cidrhost|cidrnetmask|cidrsubnet|tobool|tolist|tomap|tonumber|toset|tostring)/,terraformMainBlocks:/(module|data|terraform|resource|provider|variable|output|locals)/,tokenizer:{root:[[/^`@terraformMainBlocks`([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["type","","string","","string","","`@brackets`"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)(\{)/,["identifier","","string","","string","","`@brackets`"]],[/(\w+[ \t]+)([ \t]*)([\w-]+|"[\w-]+"|)([ \t]*)([\w-]+|"[\w-]+"|)(=)(\{)/,["identifier","","string","","operator","","`@brackets`"]],{include:"`@terraform`"}],terraform:[[/@terraformFunctions(\()/,["type","`@brackets`"]],[/[a-zA-Z_]\w*-*/,{cases:{"`@keywords`":{token:"keyword.$0"},"`@default`":"variable"}}],{include:"`@whitespace`"},{include:"`@heredoc`"},[/[{}()\[\]]/,"`@brackets`"],[/[<>](?!`@symbols`)/,"`@brackets`"],[/@symbols/,{cases:{"`@operators`":"operator","`@default`":""}}],[/\d*\d+[eE]([\-+]?\d+)?/,"number.float"],[/\d*\.\d+([eE][\-+]?\d+)?/,"number.float"],[/\d[\d']*/,"number"],[/\d/,"number"],[/[;,.]/,"delimiter"],[/"/,"string","`@string`"],[/'/,"invalid"]],heredoc:[[/<<[-]*\s*["]?([\w\-]+)["]?/,{token:"string.heredoc.delimiter",next:"`@heredocBody`.$1"}]],heredocBody:[[/([\w\-]+)$/,{cases:{"$1==$S2":[{token:"string.heredoc.delimiter",next:"`@popall`"}],"`@default`":"string.heredoc"}}],[/./,"string.heredoc"]],whitespace:[[/[ \t\r\n]+/,""],[/\/\*/,"comment","`@comment`"],[/\/\/.*$/,"comment"],[/#.*$/,"comment"]],comment:[[/[^\/*]+/,"comment"],[/\*\//,"comment","`@pop`"],[/[\/*]/,"comment"]],string:[[/\$\{/,{token:"delimiter",next:"`@stringExpression`"}],[/[^\\"\$]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","`@popall`"]],stringInsideExpression:[[/[^\\"]+/,"string"],[/@escapes/,"string.escape"],[/\\./,"string.escape.invalid"],[/"/,"string","`@pop`"]],stringExpression:[[/\}/,{token:"delimiter",next:"`@pop`"}],[/"/,"string","`@stringInsideExpression`"],{include:"`@terraform`"}]}};export{e as conf,t as language}; |
🤖 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 `@Resources/markdown-viewer/webviews-app/chunks/hcl.mjs` at line 1, The
keywords array on the language object t contains entries with trailing spaces
("if ", "else ", "endif ", "for ") so they never match; edit the t.keywords list
to remove the trailing spaces (use "if","else","endif","for") so the tokenizer
(tokenizer/root and terraform rules that reference "`@keywords`") can properly
recognize and highlight these keywords.
Files could open unhighlighted in the cmux WKWebView even though they highlight in Chrome: the lazy Monarch grammar loads after the model exists and re-tokenizes asynchronously, and the WKWebView does not reliably repaint that async update. Preload the grammar for the file's inferred language before creating the editor, so tokenization is registered before the first render (no race). Keeps the lazy loaders as a fallback for any unmapped language.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328dcefaf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| if command == "open" { try runOpenCommand(commandArgs: commandArgs, socketPath: resolvedSocketPath, explicitPassword: socketPasswordArg, jsonOutput: jsonOutput, idFormat: try resolvedIDFormat(jsonOutput: jsonOutput, raw: idFormatArg)); return } | ||
| if command == "diff" { try runDiffCommand(commandArgs: commandArgs, socketPath: resolvedSocketPath, explicitPassword: socketPasswordArg, jsonOutput: jsonOutput, idFormat: try resolvedIDFormat(jsonOutput: jsonOutput, raw: idFormatArg)); return } | ||
| if command == "edit" { try runEditCommand(commandArgs: commandArgs, socketPath: resolvedSocketPath, explicitPassword: socketPasswordArg, jsonOutput: jsonOutput, idFormat: try resolvedIDFormat(jsonOutput: jsonOutput, raw: idFormatArg)); return } |
There was a problem hiding this comment.
Adding the edit dispatch here is not enough because run() calls shouldOpenAsPathArgument(command) before this point, and that helper treats any non-registered command name that exists on disk as a path. Since edit was not added to topLevelCommandNames (checked the set near CLI/cmux.swift:4990), running cmux edit <file> from a directory that contains an edit file or folder opens that path instead of invoking the editor command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Resources/markdown-viewer/webviews-app/chunks/editorSurface.mjs`:
- Line 2: The preload map F is missing entries that match the registered
language ids used by r(...) and B(): add keys for "json" (pointing to the json
loader used by z()/jsonMode), "c" (pointing to the same loader as "cpp" i.e.
./cpp.mjs), and rename or add "proto" to match the registered id (currently F
has "protobuf") so it points to ./protobuf.mjs; update F so its keys mirror the
ids passed to r({... id: "..." ...}) and then rebuild/regenerate the bundled
file so B(o) can find F[languageId] for .json, .c/.h and .proto files.
In `@webviews/src/editor/monacoLanguages.ts`:
- Around line 118-143: Wrap the dynamic grammar load/registration inside
preloadGrammarForPath in a try-catch: call loader() and the subsequent
monaco.languages.setMonarchTokensProvider /
monaco.languages.setLanguageConfiguration inside the try, and on error catch and
log the failure (including the error and the languageId/extension) and then
return so the editor can fall back to lazy loading; reference the GRAMMARS map,
the loader() invocation, the returned grammar object, and the monaco
registration calls when adding the error handling.
In `@webviews/src/surfaces/editorSurface.tsx`:
- Around line 69-72: preloadGrammarForPath(filePath) must be made best-effort so
a failing grammar import never prevents mounting; change
mountEditorSurface/createRoot render path to start the editor immediately and
invoke preloadGrammarForPath(filePath) without awaiting it (or await it but
catch and swallow/log errors) so failures do not reject mountEditorSurface;
ensure errors from preloadGrammarForPath are caught and logged via the same
logger used in editorSurface.tsx so the existing lazy fallback language path
still functions if the warmup fails.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5eddb2b0-9166-45a8-ad30-77319848ea32
📒 Files selected for processing (3)
Resources/markdown-viewer/webviews-app/chunks/editorSurface.mjswebviews/src/editor/monacoLanguages.tswebviews/src/surfaces/editorSurface.tsx
| export async function preloadGrammarForPath(filePath: string): Promise<void> { | ||
| const dot = filePath.lastIndexOf("."); | ||
| const extension = dot >= 0 ? filePath.slice(dot) : ""; | ||
| const probe = monaco.editor.createModel( | ||
| "", | ||
| undefined, | ||
| monaco.Uri.parse(`inmemory://cmux-grammar-probe/probe${extension}`), | ||
| ); | ||
| const languageId = probe.getLanguageId(); | ||
| probe.dispose(); | ||
| const loader = GRAMMARS[languageId]; | ||
| if (!loader) { | ||
| return; | ||
| } | ||
| const grammar = await loader(); | ||
| monaco.languages.setMonarchTokensProvider( | ||
| languageId, | ||
| grammar.language as monaco.languages.IMonarchLanguage, | ||
| ); | ||
| if (grammar.conf) { | ||
| monaco.languages.setLanguageConfiguration( | ||
| languageId, | ||
| grammar.conf as monaco.languages.LanguageConfiguration, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add error handling around grammar loading and registration.
The dynamic import at line 132 and the registration calls at lines 133-141 can throw if a grammar module fails to load or is malformed. Without error handling, this would propagate to the call site in editorSurface.tsx:72 and potentially prevent the editor from mounting. Adding a try-catch allows the editor to fall back to lazy grammar loading (from the contribution imports) while logging the failure.
🛡️ Proposed error handling
export async function preloadGrammarForPath(filePath: string): Promise<void> {
const dot = filePath.lastIndexOf(".");
const extension = dot >= 0 ? filePath.slice(dot) : "";
const probe = monaco.editor.createModel(
"",
undefined,
monaco.Uri.parse(`inmemory://cmux-grammar-probe/probe${extension}`),
);
const languageId = probe.getLanguageId();
probe.dispose();
const loader = GRAMMARS[languageId];
if (!loader) {
return;
}
+ try {
const grammar = await loader();
monaco.languages.setMonarchTokensProvider(
languageId,
grammar.language as monaco.languages.IMonarchLanguage,
);
if (grammar.conf) {
monaco.languages.setLanguageConfiguration(
languageId,
grammar.conf as monaco.languages.LanguageConfiguration,
);
}
+ } catch (error) {
+ console.error(`Failed to preload grammar for language "${languageId}":`, error);
+ // Fall back to lazy grammar loading from contribution imports.
+ }
}📝 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.
| export async function preloadGrammarForPath(filePath: string): Promise<void> { | |
| const dot = filePath.lastIndexOf("."); | |
| const extension = dot >= 0 ? filePath.slice(dot) : ""; | |
| const probe = monaco.editor.createModel( | |
| "", | |
| undefined, | |
| monaco.Uri.parse(`inmemory://cmux-grammar-probe/probe${extension}`), | |
| ); | |
| const languageId = probe.getLanguageId(); | |
| probe.dispose(); | |
| const loader = GRAMMARS[languageId]; | |
| if (!loader) { | |
| return; | |
| } | |
| const grammar = await loader(); | |
| monaco.languages.setMonarchTokensProvider( | |
| languageId, | |
| grammar.language as monaco.languages.IMonarchLanguage, | |
| ); | |
| if (grammar.conf) { | |
| monaco.languages.setLanguageConfiguration( | |
| languageId, | |
| grammar.conf as monaco.languages.LanguageConfiguration, | |
| ); | |
| } | |
| } | |
| export async function preloadGrammarForPath(filePath: string): Promise<void> { | |
| const dot = filePath.lastIndexOf("."); | |
| const extension = dot >= 0 ? filePath.slice(dot) : ""; | |
| const probe = monaco.editor.createModel( | |
| "", | |
| undefined, | |
| monaco.Uri.parse(`inmemory://cmux-grammar-probe/probe${extension}`), | |
| ); | |
| const languageId = probe.getLanguageId(); | |
| probe.dispose(); | |
| const loader = GRAMMARS[languageId]; | |
| if (!loader) { | |
| return; | |
| } | |
| try { | |
| const grammar = await loader(); | |
| monaco.languages.setMonarchTokensProvider( | |
| languageId, | |
| grammar.language as monaco.languages.IMonarchLanguage, | |
| ); | |
| if (grammar.conf) { | |
| monaco.languages.setLanguageConfiguration( | |
| languageId, | |
| grammar.conf as monaco.languages.LanguageConfiguration, | |
| ); | |
| } | |
| } catch (error) { | |
| console.error(`Failed to preload grammar for language "${languageId}":`, error); | |
| // Fall back to lazy grammar loading from contribution imports. | |
| } | |
| } |
🤖 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 `@webviews/src/editor/monacoLanguages.ts` around lines 118 - 143, Wrap the
dynamic grammar load/registration inside preloadGrammarForPath in a try-catch:
call loader() and the subsequent monaco.languages.setMonarchTokensProvider /
monaco.languages.setLanguageConfiguration inside the try, and on error catch and
log the failure (including the error and the languageId/extension) and then
return so the editor can fall back to lazy loading; reference the GRAMMARS map,
the loader() invocation, the returned grammar object, and the monaco
registration calls when adding the error handling.
| // Load the file's Monarch grammar before mounting so the editor tokenizes | ||
| // synchronously on first render (the WKWebView does not reliably repaint the | ||
| // lazy async re-tokenization). | ||
| await preloadGrammarForPath(filePath); |
There was a problem hiding this comment.
Don't let grammar warmup block the entire editor surface.
await preloadGrammarForPath(filePath) runs before createRoot(...).render(...). If a grammar chunk import rejects, mountEditorSurface() rejects and the editor never mounts at all, even though highlighting is only an enhancement. Make this preload best-effort so the existing lazy language path can still render the surface.
Proposed fix
// Load the file's Monarch grammar before mounting so the editor tokenizes
// synchronously on first render (the WKWebView does not reliably repaint the
// lazy async re-tokenization).
- await preloadGrammarForPath(filePath);
+ try {
+ await preloadGrammarForPath(filePath);
+ } catch {
+ // Best-effort only: keep the editor mount path alive even if a grammar
+ // chunk fails to warm up.
+ }📝 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.
| // Load the file's Monarch grammar before mounting so the editor tokenizes | |
| // synchronously on first render (the WKWebView does not reliably repaint the | |
| // lazy async re-tokenization). | |
| await preloadGrammarForPath(filePath); | |
| // Load the file's Monarch grammar before mounting so the editor tokenizes | |
| // synchronously on first render (the WKWebView does not reliably repaint the | |
| // lazy async re-tokenization). | |
| try { | |
| await preloadGrammarForPath(filePath); | |
| } catch { | |
| // Best-effort only: keep the editor mount path alive even if a grammar | |
| // chunk fails to warm up. | |
| } |
🤖 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 `@webviews/src/surfaces/editorSurface.tsx` around lines 69 - 72,
preloadGrammarForPath(filePath) must be made best-effort so a failing grammar
import never prevents mounting; change mountEditorSurface/createRoot render path
to start the editor immediately and invoke preloadGrammarForPath(filePath)
without awaiting it (or await it but catch and swallow/log errors) so failures
do not reject mountEditorSurface; ensure errors from preloadGrammarForPath are
caught and logged via the same logger used in editorSurface.tsx so the existing
lazy fallback language path still functions if the warmup fails.
…dded Highlighting rendered inconsistently in the cmux WKWebView (correct in Chrome). Root causes + fixes: - The per-language `.contribution.js` lazy loaders set the Monarch tokens provider via a late async import after the model existed, and the WKWebView did not repaint that async re-tokenization. Register each common language's id + extensions manually and load its grammar eagerly (awaited) before the editor is created, so the provider is the only one set and is in place before the first render. Removes the lazy loaders entirely. - Embedded-language grammars (HTML embeds CSS/JS, Markdown embeds fenced code) fell back to one token per line because their nested tokenizers' languages were not registered. Preload the embedded grammars alongside the host. - ResizeObserver drives editor.layout (the pane reports 0 height at create). Verified in-app (socket DOM probe) across python, go, rust, java, c/c++, sql, yaml, html, markdown, json. A freshly-opened surface can still briefly render plain before its first tokenization paints; re-opening or interacting settles it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 47e4b1b. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e4b1bfbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Load the file's Monarch grammar before mounting so the editor tokenizes | ||
| // synchronously on first render (the WKWebView does not reliably repaint the | ||
| // lazy async re-tokenization). | ||
| await preloadGrammarForPath(filePath); |
There was a problem hiding this comment.
In restored/custom-scheme editor surfaces, opening common file types such as .js, .html, or .py can leave the pane blank because their generated grammar imports ask Vite's preload helper to add ../assets/monaco-vendor.css as an external <link rel="stylesheet">; the custom-scheme page CSP only allows inline styles, so that preload rejects, and this awaited preloadGrammarForPath prevents EditorApp from rendering at all. Since Monaco CSS is already fetched and inlined just above, catch/ignore grammar preload failures caused by the stylesheet preload or remove the CSS dependency from those dynamic imports before blocking the mount.
Useful? React with 👍 / 👎.
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 `@webviews/src/editor/monacoLanguages.ts`:
- Around line 29-30: preloadGrammarForPath currently returns early for paths
without a dot and therefore never loads the Dockerfile grammar; update
preloadGrammarForPath to detect the basename "Dockerfile" (and other common
extensionless names if desired) and explicitly load the "dockerfile" grammar via
the existing grammar loader for id "dockerfile". Then update EditorApp where you
create the model (or immediately after) to pass the resolved language id or call
monaco.editor.setModelLanguage(model, "dockerfile") when the path basename is
"Dockerfile" so Monaco uses the dockerfile grammar even though there is no file
extension; reference preloadGrammarForPath and the EditorApp model creation /
language assignment code to make these changes.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b9dba513-64d7-4f07-b116-4abf385dc58a
📒 Files selected for processing (2)
Resources/markdown-viewer/webviews-app/chunks/editorSurface.mjswebviews/src/editor/monacoLanguages.ts
| { id: "dockerfile", extensions: [".dockerfile"], grammar: () => import("monaco-editor/esm/vs/basic-languages/dockerfile/dockerfile.js") }, | ||
| { id: "elixir", extensions: [".ex", ".exs"], grammar: () => import("monaco-editor/esm/vs/basic-languages/elixir/elixir.js") }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file
ls -la webviews/src/editor/monacoLanguages.ts
# Show the relevant portions with line numbers
sed -n '1,220p' webviews/src/editor/monacoLanguages.ts | nl -ba
# Search for preloadGrammarForPath usage and any extensionless handling elsewhere
rg -n "preloadGrammarForPath|byExtension|LANGUAGES|languages\.register|filenames" webviews/src/editor/monacoLanguages.ts
rg -n "preloadGrammarForPath\(" -S webviews/src/editor webviews/src | head -n 50
# Also search where editor decides language from URI/path
rg -n "languageFrom|uri.*language|model.*language|setLanguage|monaco\.editor\.setModelLanguage|getLanguage" webviews/src/editor webviews/src | head -n 80Repository: manaflow-ai/cmux
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File =="
sed -n '1,260p' webviews/src/editor/monacoLanguages.ts | cat -n
echo "== Key symbols search =="
rg -n "preloadGrammarForPath|byExtension|LANGUAGES|languages\.register|filenames" webviews/src/editor/monacoLanguages.ts
echo "== Call site search (wider) =="
rg -n "preloadGrammarForPath\(" webviews/src/editor webviews/src | head -n 50
echo "== Editor model language inference search (wider) =="
rg -n "setModelLanguage|setLanguage|getLanguage|languageFrom|monaco\.editor" webviews/src/editor webviews/src | head -n 120Repository: manaflow-ai/cmux
Length of output: 12620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' webviews/src/surfaces/editorSurface.tsx | cat -n
echo "== Search for preloadGrammarForPath call inputs =="
rg -n "preloadGrammarForPath\(" webviews/src/surfaces/editorSurface.tsx webviews/src/editor webviews/src | cat
echo "== Inspect EditorApp model creation (path/URI -> language) =="
sed -n '1,120p' webviews/src/editor/EditorApp.tsx | cat -nRepository: manaflow-ai/cmux
Length of output: 7756
🌐 Web query:
Monaco Editor monaco.languages.register filenames option
💡 Result:
The monaco.languages.register function does not have a filenames option [1][2]. If you are looking to associate a language with specific file types, you should use the extensions or filenames (where applicable in broader configuration) properties within the registration object, though the standard registration API primarily uses extensions and aliases [3]. Here is a summary of how language associations are typically handled: 1. Registering a language: When calling monaco.languages.register, you define the language's identity. Associations with file patterns are often defined via the extensions property [3]: monaco.languages.register({ id: 'my-language', extensions: ['.my-ext', '.other-ext'] }); 2. Handling specific filenames: If you need to associate a specific filename (like.eslintrc or Dockerfile) with a language, the Monaco Editor core API typically relies on the file's URI when creating a model [4]. When you create a model, you provide the URI, and Monaco uses the registered language metadata to automatically match the file's extension or name to the appropriate language [4]: const model = monaco.editor.createModel(content, undefined, monaco.Uri.file('path/to/my.filename')); 3. Clarification on "filenames" options: - Monaco Editor Webpack Plugin: If you encountered a reference to a filename option in a Monaco context, it is likely referring to the MonacoWebpackPlugin configuration, which uses a filename property to set custom templates for worker script files (e.g., [name].worker.js) [5][6]. This is a build-tool configuration, not a language registration option [5]. - Third-party extensions: Some third-party libraries (like those providing completion or LSP features) may introduce their own registration options that include a filename property to provide context for completion providers, but this is not part of the standard Monaco Editor API [7]. If you are trying to ensure a file is treated as a certain language regardless of its extension, use monaco.editor.setModelLanguage(model, 'language-id') after the model is created [4].
Citations:
- 1: https://microsoft.github.io/monaco-editor/typedoc/modules/editor_editor_api.languages.html
- 2: https://microsoft.github.io/monaco-editor/typedoc/modules/languages_register_all.html
- 3: https://www.npmjs.com/package/@volar/monaco
- 4: https://stackoverflow.com/questions/56681345/how-to-dynamically-set-language-according-to-file-extension-in-monaco-editor
- 5: https://registry.npmjs.org/monaco-editor-webpack-plugin
- 6: https://github.com/microsoft/monaco-editor/blob/main/webpack-plugin/README.md
- 7: https://monacopilot.dev/configuration/register-options.html
Handle extensionless filenames like Dockerfile in Monaco language resolution
dockerfile is registered only for the .dockerfile extension (so Dockerfile won’t match), and preloadGrammarForPath() returns early when the path has no . (so the Dockerfile grammar never loads). Since EditorApp creates the model with an undefined language id, Monaco will still infer from extensions only—leaving Dockerfile as plain text. (Also, monaco.languages.register doesn’t support a filenames option.)
Fix by (1) detecting Dockerfile from the path basename in preloadGrammarForPath() and loading the dockerfile grammar, and (2) in EditorApp either pass the resolved language id to monaco.editor.createModel or call monaco.editor.setModelLanguage(model, "dockerfile") for that basename.
🤖 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 `@webviews/src/editor/monacoLanguages.ts` around lines 29 - 30,
preloadGrammarForPath currently returns early for paths without a dot and
therefore never loads the Dockerfile grammar; update preloadGrammarForPath to
detect the basename "Dockerfile" (and other common extensionless names if
desired) and explicitly load the "dockerfile" grammar via the existing grammar
loader for id "dockerfile". Then update EditorApp where you create the model (or
immediately after) to pass the resolved language id or call
monaco.editor.setModelLanguage(model, "dockerfile") when the path basename is
"Dockerfile" so Monaco uses the dockerfile grammar even though there is no file
extension; reference preloadGrammarForPath and the EditorApp model creation /
language assignment code to make these changes.
Highlighting rendered plain non-deterministically in cmux's offscreen-IOSurface WKWebView because Monaco tokenizes via a throttled async scheduler the WKWebView starves. Force synchronous tokenization (model.tokenization.forceTokenization) of the viewport after mount, then re-render, so every language highlights deterministically. JSON used the JSON language service, which tokenizes via a separate async path (unaffected by forceTokenization), so it stayed plain. Replace it with a small Monarch grammar so JSON highlights through the same deterministic path as every other language. Drops the 383KB JSON worker (bundle 4.6MB -> 4.2MB). Verified in-app (socket DOM probe, repeated): python, go, rust, c/c++, java, sql, yaml, html, markdown, json all highlight consistently.
lawrencecchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b8d01626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| index += 1 | ||
| } | ||
| guard let filePathArg else { | ||
| throw CLIError(message: "Usage: cmux edit <file> [--window <w>] [--workspace <ws>] [--surface <s>] [--focus]") |
There was a problem hiding this comment.
For localized builds, this new cmux edit usage/error path is emitted as hard-coded English; the repo guideline in AGENTS.md requires user-facing command/help text and errors to have entries for every supported locale. Japanese users hitting the usage, missing-file, or non-UTF8 paths added by this command will get untranslated text, so please wrap the new edit messages/status output in localized strings and add the corresponding Localizable.xcstrings entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
webviews/src/editor/monacoLanguages.ts (2)
57-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle extensionless filenames (e.g.,
Dockerfile) during grammar preload.Line 57 registers
dockerfile, but Line 142-145 exits early for extensionless paths, so common filenames likeDockerfilewon’t preload or resolve todockerfileand render as plain text.💡 Minimal fix
const byExtension = new Map<string, LanguageDef>(); const byId = new Map<string, LanguageDef>(); +const byBasename = new Map<string, LanguageDef>(); for (const language of LANGUAGES) { monaco.languages.register({ id: language.id, extensions: language.extensions }); byId.set(language.id, language); for (const extension of language.extensions) { byExtension.set(extension, language); } } + +for (const [basename, id] of Object.entries({ dockerfile: "dockerfile" })) { + const language = byId.get(id); + if (language) byBasename.set(basename, language); +} export async function preloadGrammarForPath(filePath: string): Promise<void> { - const dot = filePath.lastIndexOf("."); - if (dot < 0) { - return; - } - const language = byExtension.get(filePath.slice(dot).toLowerCase()); + const normalized = filePath.replace(/\\/g, "/"); + const basename = normalized.slice(normalized.lastIndexOf("/") + 1).toLowerCase(); + const dot = basename.lastIndexOf("."); + const language = + (dot >= 0 ? byExtension.get(basename.slice(dot)) : undefined) ?? + byBasename.get(basename); if (!language) { return; }Also applies to: 141-147
🤖 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 `@webviews/src/editor/monacoLanguages.ts` at line 57, The grammar preload logic skips extensionless filenames (so files named "Dockerfile" never map to the registered language id "dockerfile"); modify the path-to-language resolution in monacoLanguages.ts (the function that currently returns early for extensionless paths) to also check the basename when no extension exists and map known filenames (e.g., "Dockerfile") to their language id ("dockerfile") before exiting; update the preload/resolve logic used by the grammar registration so extensionless common filenames are detected and trigger import("monaco-editor/esm/vs/basic-languages/dockerfile/dockerfile.js").
117-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not let grammar import failures abort editor mount.
At Line 122, a failed dynamic import rejects
preloadGrammarForPath(Line 153/155 path), which can prevent the editor surface from mounting at all. Degrade to plain text instead of failing startup.💡 Minimal fix
async function loadGrammarById(id: string): Promise<void> { const language = byId.get(id); if (!language) { return; } - const grammar = await language.grammar(); - monaco.languages.setMonarchTokensProvider( - id, - grammar.language as monaco.languages.IMonarchLanguage, - ); - if (grammar.conf) { - monaco.languages.setLanguageConfiguration( - id, - grammar.conf as monaco.languages.LanguageConfiguration, - ); - } + try { + const grammar = await language.grammar(); + monaco.languages.setMonarchTokensProvider( + id, + grammar.language as monaco.languages.IMonarchLanguage, + ); + if (grammar.conf) { + monaco.languages.setLanguageConfiguration( + id, + grammar.conf as monaco.languages.LanguageConfiguration, + ); + } + } catch { + // Keep editor boot resilient; fallback is unhighlighted/plain tokenization. + } }Also applies to: 150-156
🤖 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 `@webviews/src/editor/monacoLanguages.ts` around lines 117 - 123, The dynamic grammar import can throw and currently bubbles up, preventing the editor from mounting; modify loadGrammarById (and the caller preloadGrammarForPath) to catch errors from language.grammar() and from the await call so failures do not reject startup: if language.grammar() throws, log/debug the error, skip calling monaco.languages.setMonarchTokensProvider (leave the language as plain text) and return gracefully (do not rethrow); ensure preloadGrammarForPath wraps its call to loadGrammarById in a try/catch and similarly degrades to plain text instead of propagating the error.
🤖 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 `@webviews/src/editor/monacoLanguages.ts`:
- Line 57: The grammar preload logic skips extensionless filenames (so files
named "Dockerfile" never map to the registered language id "dockerfile"); modify
the path-to-language resolution in monacoLanguages.ts (the function that
currently returns early for extensionless paths) to also check the basename when
no extension exists and map known filenames (e.g., "Dockerfile") to their
language id ("dockerfile") before exiting; update the preload/resolve logic used
by the grammar registration so extensionless common filenames are detected and
trigger import("monaco-editor/esm/vs/basic-languages/dockerfile/dockerfile.js").
- Around line 117-123: The dynamic grammar import can throw and currently
bubbles up, preventing the editor from mounting; modify loadGrammarById (and the
caller preloadGrammarForPath) to catch errors from language.grammar() and from
the await call so failures do not reject startup: if language.grammar() throws,
log/debug the error, skip calling monaco.languages.setMonarchTokensProvider
(leave the language as plain text) and return gracefully (do not rethrow);
ensure preloadGrammarForPath wraps its call to loadGrammarById in a try/catch
and similarly degrades to plain text instead of propagating the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47a0bd73-1c26-44e2-9262-d8fe56061875
📒 Files selected for processing (5)
Resources/markdown-viewer/webviews-app/chunks/editorSurface.mjsResources/markdown-viewer/webviews-app/chunks/monaco-vendor.mjs.deflatewebviews/src/editor/EditorApp.tsxwebviews/src/editor/monacoEnvironment.tswebviews/src/editor/monacoLanguages.ts
# Conflicts: # Resources/markdown-viewer/webviews-app/chunks/diffSurface.mjs
Addresses autoreview P1: the editor defaulted to writable with no save path, so edits were silently discarded on close. Default readOnly to true (a future save feature can pass readOnly: false).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Autoreview flagged a format mismatch (raw deflate vs zlib-wrapped). Apple's `.zlib` algorithm actually decodes raw DEFLATE, so node `deflateRawSync` output round-trips correctly through `NSData.decompressed(using:.zlib)` — verified directly against the committed diff-vendor.mjs.deflate and end-to-end in dogfood. Comment-only; documents the non-obvious invariant for reviewers.
- CLI: stricter arg parsing (reject unknown flags / missing option values / extra positionals, support --); distinguish read errors from non-UTF-8 and directories; return the source file path (not the temp viewer page) in --json. - editorSurface: guard JSON.parse of the injected config; skip CSS injection on a non-OK fetch; move the displaced mountEditorSurface JSDoc back to its function. - vite.config: correct the worker-output comment (the ?worker file is hashed). (HCL trailing-space grammar finding is in Monaco's vendored chunk, not ours.)
Brings PRs manaflow-ai#5638 + manaflow-ai#5761 from manaflow-ai/cmux, re-ported onto current upstream after two months of drift: - `cmux edit <file>` opens a Monaco surface with syntax highlighting for 45 languages, find-in-file, and goto-line. - Read-write: save bridge, dirty tracking, and SHA-256 on-disk conflict detection (refuses to clobber a file changed behind the editor). Neither PR ever merged upstream; both were left open with no human review since 2026-06-12. Verified end-to-end on this tree: save writes, a save with a stale baseline is refused, force-overwrite succeeds.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Adds a Monaco code editor as a first-class surface in the
webviews/React harness, openable withcmux edit <file>. Built on the surface-split groundwork from #5613.What's here
cmux edit <file>opens a themed Monaco editor inside cmux (rides the existing diff-viewer custom-scheme +browser.open_splitflow; no new SurfaceRole). CLIwriteEditormirrors the lean diff path; the editor page injects the file content + live cmux appearance as config.cmux-dark/cmux-lightviadefineTheme, light + dark).--checkmade content-aware).Notable fixes for cmux's WKWebView
text/css; the editor fetches + inlines Monaco's stylesheet (CSP-safe), no font/CSP changes..contributionloaders), preload embedded grammars (html→css/js, markdown→code),ResizeObserverlayout, and force synchronous tokenization (model.tokenization.forceTokenization) after mount. JSON uses a small inline Monarch grammar (dropped the 383 KB JSON service worker).Verification
bun run typecheck,lint:ci,verify:tanstack-router,build-webviews-app.sh --check, React Compiler guard: all pass.mono) socket DOM probe: python, go, rust, c/c++, java, sql, yaml, html, markdown, json all highlight consistently across repeated clean runs.Follow-ups
cmux.jsoneditor config section.🤖 Generated with Claude Code
Summary by CodeRabbit
editcommand for file editingNote
Medium Risk
New CLI path reads local files and opens web surfaces through the diff-viewer allowlist/HTTP server; changes broaden what the local server can serve (CSS) but follow the existing split-open model.
Overview
Adds
cmux edit <file>, which reads a UTF-8 file, generates aneditorHTML page (file path, content, title, live appearance config), and opens it via the existing diff-viewer custom URL +browser.open_splitflow—with optional--window/--workspace/--surface/--focusand JSON output fields for path, viewer path, and URL.Diff-viewer asset serving is extended so Monaco can load styles: allow
text/css, classify.cssassets correctly in the allowlist, and includecss(and.deflatelogical names) when enumerating/copying bundled webview assets, with raw DEFLATE inflation on copy and mtime-based skip logic for inflated targets.Reviewed by Cursor Bugbot for commit 6b1159e. Bugbot is set up for automated code reviews on this repo. Configure here.