refactor(FR-2697): self-host Monaco Editor to eliminate jsDelivr CDN dependency#6959
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
Refactors Monaco Editor loading to be self-hosted (served from /vs) instead of using jsDelivr, improving support for offline/air-gapped deployments.
Changes:
- Add a centralized
loadMonacoEditor()helper to configure@monaco-editor/reactloader paths to/vs. - Update Monaco-using modals to lazy-load the editor via the shared helper.
- Serve/copy Monaco’s
min/vsassets: add/vsstatic mapping in dev (Craco) and copy assets intobuild/web/vsduring production builds.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| react/src/helper/monacoEditor.ts | New shared loader that configures Monaco to load from /vs before initializing. |
| react/src/components/VFolderTextFileEditorModal.tsx | Switch lazy import to use the centralized Monaco loader/config. |
| react/src/components/BrandingSettingItems/ThemeJsonConfigModal.tsx | Switch lazy import to use the centralized Monaco loader/config. |
| react/package.json | Add monaco-editor dependency so assets exist locally for self-hosting. |
| react/craco.config.cjs | Serve node_modules/monaco-editor/min/vs at /vs in dev. |
| package.json | Add copymonaco and run it during root build to ship /vs in production output. |
| pnpm-lock.yaml | Lockfile update for monaco-editor. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🔴 | Statements | 8.92% | 1831/20526 |
| 🔴 | Branches | 8.07% | 1165/14431 |
| 🔴 | Functions | 5.28% | 294/5569 |
| 🔴 | Lines | 8.65% | 1723/19915 |
Test suite run success
865 tests passing in 40 suites.
Report generated by 🧪jest coverage report action from 547b702
yomybaby
left a comment
yomybaby
left a comment
There was a problem hiding this comment.
Review: PR #6959 — refactor(FR-2697) self-host Monaco Editor
Summary
Scope is tight and the self-host migration looks functionally sound in the web path (dev server + prod). Two notable points worth verifying: (1) the Electron file:// protocol handler will resolve /vs/* by coincidence of its app/ auto-prefix, not by explicit design — worth a one-line comment so this doesn't silently break in a future refactor; (2) the Workbox size rationale in the PR body is a non-issue (the /vs/* files are never in the webpack asset graph and Workbox won't precache them at all), so the editor isn't service-worker-offline regardless of size — clarify or drop the note.
Findings
react/src/helper/monacoEditor.ts
Correctness / Electron — Should Fix (documentation): The helper sets paths.vs = '/vs', an absolute URL. Under Electron the page loads via file:///…/app/index.html, so /vs/loader.js resolves to file:///vs/loader.js. This happens to work only because electron-app/main.js's protocol.handle('file', …) strips the leading slash and auto-prefixes non-app/|resources/|manifest/ requests with app/, landing on app/vs/loader.js. Please add a sentence to the helper's top-comment documenting this dependency, so nobody changes the file protocol handler without realizing Monaco relies on it. If a sub-path deployment is ever introduced, '/vs' will also need to be read from BASE_URL.
Nit: modulePromise is very generic for a module-level identifier. Consider monacoModulePromise for readability.
package.json (root copymonaco script)
Suggestion: cp -R react/node_modules/monaco-editor/min/vs/. build/web/vs/ will fail loudly if monaco-editor isn't installed, which is the correct behavior. Consider making the failure message slightly more actionable — e.g., guard with test -d react/node_modules/monaco-editor/min/vs || { echo 'monaco-editor missing — run pnpm install'; exit 1; } && cp -R …. Nice-to-have.
Build ordering — OK: copymonaco runs before pnpm run -r --stream build, and react/package.json's build then does cp -r ./build/* ../build/web/ which does not touch build/web/vs/. Survives rm -rf build/web at the start of the build chain. No issue.
Electron packaging — OK: make dep_electron does cp -Rp build/web build/electron-app/app, so build/web/vs/ lands at build/electron-app/app/vs/. No Makefile change needed. Confirmed.
react/craco.config.cjs
Nit / Dev server static ordering: The new /vs entry sits after the project-root / entry. webpack-dev-server walks static entries in order, but requests to /vs/* would first probe the project root (/) for vs/<...>. A stray vs/ directory at the project root would shadow the Monaco AMD tree. Today no such directory exists; worth a one-line comment so future refactors don't accidentally add one.
PR description — Service worker note
Suggestion (no code change): The body says "ts.worker-*.js is ~6.7 MB, exceeding the Workbox maximumFileSizeToCacheInBytes: 5 MB per-file cap, so it will be skipped by the service-worker precache." The /vs/* files are copied outside webpack's asset graph (root copymonaco → build/web/vs/, not react/build/static/), so GenerateSW never considers them at all — neither the 7 MB TS worker nor the 40 KB loader.js. The editor is therefore not service-worker-offline regardless of the 5 MB cap; it's cached only by normal HTTP caching. Please clarify the PR body (and update spec.md follow-up note if applicable) — the current phrasing suggests most of /vs/* is precached, which is not the case.
Recommendation
Approve with suggestions. The migration is correctly implemented and the scope is disciplined. The only thing I'd ask for before merge is the one-line comment in monacoEditor.ts documenting the Electron file-protocol dependency, and a small tweak to the PR body so the Workbox/offline characterization isn't misleading. Everything else is optional polish.
yomybaby
force-pushed
the
04-24-refactor_fr-2697_self-host_monaco_editor_to_eliminate_jsdelivr_cdn_dependency
branch
3 times, most recently
from
c484d26 to
b9e7eb3
Compare
Merge activity
|
…dependency (#6959) Resolves #6958 (FR-2697) ## Summary `@monaco-editor/react` loads Monaco from the jsDelivr CDN by default, which breaks the file editor in **offline / air-gapped environments**. This switches Monaco to a path-based self-host served at `/resources/monaco/vs`. ``` scripts/verify.sh: === ALL PASS === Self-host verified: 16 network requests, all to localhost (zero jsDelivr) ``` ## What changed - **`react/package.json`** — add `monaco-editor@^0.55.1` (the version `@monaco-editor/loader` previously fetched from the CDN, pinned so the served assets match what users already had). - **root `package.json`** — new `copymonaco` script that copies `react/node_modules/monaco-editor/min/vs` to `build/web/resources/monaco/vs`; wired into the `build` chain. Electron consumes the same `build/web/` tree (`Makefile` `dep_electron` copies `build/web` → `build/electron-app/app`), so no Makefile change is needed. - **`react/craco.config.cjs`** — map `/resources/monaco/vs` → `react/node_modules/monaco-editor/min/vs` in the dev server static config. - **`react/src/helper/monacoEditor.ts`** (new) — shared lazy loader that calls `loader.config({ paths: { vs: '/resources/monaco/vs' } })` once before resolving to `@monaco-editor/react`. The config must run before the first `loader.init()`, so centralizing here removes the risk of a component-level miss. - **`VFolderTextFileEditorModal`**, **`ThemeJsonConfigModal`** — import through the shared loader helper instead of calling `import('@monaco-editor/react')` directly. No behavior change for end users. ## Why the `/resources/monaco/vs` URL prefix The AMD module prefix (`paths.vs`) must stay as `vs` because Monaco's internal module IDs are `vs/editor/editor.main`, `vs/language/...`, etc. — but the **URL prefix** is entirely our choice. A top-level `/vs` route is short but easy to collide with future app routes (e.g. a "virtual space" or "version selector" feature). Nesting Monaco under the existing static-asset namespace (`/resources/`, same place as `theme.schema.json` and `model-definition.schema.json`) keeps the app-routing surface clean and makes it clear that this tree is vendored static content. ## Why path-based self-host (not webpack bundle) - Preserves the existing lazy-load chunking — Monaco (~15 MB, mostly language workers) stays as separate AMD files outside the main webpack graph, so the main JS bundle size is unchanged. - Avoids introducing `monaco-editor-webpack-plugin` and a custom `MonacoEnvironment.getWorker` setup, both of which interact poorly with the current Craco/service-worker pipeline. - Offline-safe by construction: the AMD files are part of the deployed static tree, not a runtime CDN fetch. ## Notes / follow-ups - The `/resources/monaco/vs` tree is copied to `build/web/resources/monaco/vs/` by the root `copymonaco` step, which runs *outside* the webpack build. Those files are therefore not webpack assets and are not considered by the Workbox `GenerateSW` precache at all — Monaco relies on normal HTTP caching, not the service-worker precache. In the primary air-gapped scenario this is fine because the app server itself serves the tree. If reliable repeat-visit offline access to Monaco is later needed, add a Workbox `runtimeCaching` rule matching `/resources/monaco/vs/*` (e.g. `StaleWhileRevalidate`). The ~6.7 MB `ts.worker-*.js` is unused today: VFolder files and the theme config are plain text / JSON / YAML / TOML, and the TypeScript language service is never activated. - The absolute `/resources/monaco/vs` path assumes the app is served from the origin root in both web and Electron builds. In Electron, `electron-app/main.js`'s `file://` protocol handler rewrites root-absolute paths to the packaged `app/` directory, which is what makes `/resources/monaco/vs/loader.js` resolve to `app/resources/monaco/vs/loader.js`. If that handler is refactored, Monaco loading here must be revisited. Sub-path web deployments would also need the helper to derive the path from the deploy base URL. - After this PR ships, `monaco-yaml` can be re-evaluated: the worker-protocol issue previously documented in `.specs/draft-definition-file-validation/spec.md` was tied to the CDN-loaded Monaco; with self-hosted Monaco, `MonacoEnvironment.getWorker` can be wired correctly. ## Test plan - [x] `bash scripts/verify.sh` passes (`=== ALL PASS ===`). - [x] Production build: ran root `copymonaco` → `build/web/resources/monaco/vs/` populated (121 files, 15 MB, includes `loader.js`, `editor/editor.main.js`, language bundles). - [x] Runtime air-gap verification with `serve build/web`: opened a test page that triggers AMD `require(['vs/editor/editor.main'])` via `/resources/monaco/vs/loader.js`. Chrome DevTools network log shows **16 requests, all to `localhost` origin** — zero `cdn.jsdelivr.net` / external calls. Monaco fully renders. - [ ] Manually open the VFolder file editor and the Theme JSON config modal with DevTools Network filtered to `cdn.jsdelivr.net` — confirm no requests. - [ ] Electron packaging path: verify `build/electron-app/app/resources/monaco/vs/` is populated after `make dep`. **Checklist:** - [ ] Documentation — not needed, no user-visible behavior change - [ ] Minimum required manager version — not applicable - [ ] Specific setting for review — verify no `cdn.jsdelivr.net` requests when opening either Monaco-backed modal - [ ] Minimum requirements to check during review — offline smoke test recommended - [ ] Test case(s) to demonstrate before/after — DevTools Network log on both modals
graphite-app
Bot
force-pushed
the
04-24-refactor_fr-2697_self-host_monaco_editor_to_eliminate_jsdelivr_cdn_dependency
branch
from
b9e7eb3 to
547b702
Compare
graphite-app
Bot
deleted the
04-24-refactor_fr-2697_self-host_monaco_editor_to_eliminate_jsdelivr_cdn_dependency
branch
Resolves #6958 (FR-2697)
Summary
@monaco-editor/reactloads Monaco from the jsDelivr CDN by default, which breaks the file editor in offline / air-gapped environments. This switches Monaco to a path-based self-host served at/resources/monaco/vs.What changed
react/package.json— addmonaco-editor@^0.55.1(the version@monaco-editor/loaderpreviously fetched from the CDN, pinned so the served assets match what users already had).package.json— newcopymonacoscript that copiesreact/node_modules/monaco-editor/min/vstobuild/web/resources/monaco/vs; wired into thebuildchain. Electron consumes the samebuild/web/tree (Makefiledep_electroncopiesbuild/web→build/electron-app/app), so no Makefile change is needed.react/craco.config.cjs— map/resources/monaco/vs→react/node_modules/monaco-editor/min/vsin the dev server static config.react/src/helper/monacoEditor.ts(new) — shared lazy loader that callsloader.config({ paths: { vs: '/resources/monaco/vs' } })once before resolving to@monaco-editor/react. The config must run before the firstloader.init(), so centralizing here removes the risk of a component-level miss.VFolderTextFileEditorModal,ThemeJsonConfigModal— import through the shared loader helper instead of callingimport('@monaco-editor/react')directly. No behavior change for end users.Why the
/resources/monaco/vsURL prefixThe AMD module prefix (
paths.vs) must stay asvsbecause Monaco's internal module IDs arevs/editor/editor.main,vs/language/..., etc. — but the URL prefix is entirely our choice. A top-level/vsroute is short but easy to collide with future app routes (e.g. a "virtual space" or "version selector" feature). Nesting Monaco under the existing static-asset namespace (/resources/, same place astheme.schema.jsonandmodel-definition.schema.json) keeps the app-routing surface clean and makes it clear that this tree is vendored static content.Why path-based self-host (not webpack bundle)
monaco-editor-webpack-pluginand a customMonacoEnvironment.getWorkersetup, both of which interact poorly with the current Craco/service-worker pipeline.Notes / follow-ups
/resources/monaco/vstree is copied tobuild/web/resources/monaco/vs/by the rootcopymonacostep, which runs outside the webpack build. Those files are therefore not webpack assets and are not considered by the WorkboxGenerateSWprecache at all — Monaco relies on normal HTTP caching, not the service-worker precache. In the primary air-gapped scenario this is fine because the app server itself serves the tree. If reliable repeat-visit offline access to Monaco is later needed, add a WorkboxruntimeCachingrule matching/resources/monaco/vs/*(e.g.StaleWhileRevalidate). The ~6.7 MBts.worker-*.jsis unused today: VFolder files and the theme config are plain text / JSON / YAML / TOML, and the TypeScript language service is never activated./resources/monaco/vspath assumes the app is served from the origin root in both web and Electron builds. In Electron,electron-app/main.js'sfile://protocol handler rewrites root-absolute paths to the packagedapp/directory, which is what makes/resources/monaco/vs/loader.jsresolve toapp/resources/monaco/vs/loader.js. If that handler is refactored, Monaco loading here must be revisited. Sub-path web deployments would also need the helper to derive the path from the deploy base URL.monaco-yamlcan be re-evaluated: the worker-protocol issue previously documented in.specs/draft-definition-file-validation/spec.mdwas tied to the CDN-loaded Monaco; with self-hosted Monaco,MonacoEnvironment.getWorkercan be wired correctly.Test plan
bash scripts/verify.shpasses (=== ALL PASS ===).copymonaco→build/web/resources/monaco/vs/populated (121 files, 15 MB, includesloader.js,editor/editor.main.js, language bundles).serve build/web: opened a test page that triggers AMDrequire(['vs/editor/editor.main'])via/resources/monaco/vs/loader.js. Chrome DevTools network log shows 16 requests, all tolocalhostorigin — zerocdn.jsdelivr.net/ external calls. Monaco fully renders.cdn.jsdelivr.net— confirm no requests.build/electron-app/app/resources/monaco/vs/is populated aftermake dep.Checklist:
cdn.jsdelivr.netrequests when opening either Monaco-backed modal