Skip to content

Code - Upgrade monaco-editor to 0.31.1 for the null-model worker guard - #4301

Open
agalin920 wants to merge 3 commits into
devfrom
fix/3895-monaco-json-worker-null-guard
Open

Code - Upgrade monaco-editor to 0.31.1 for the null-model worker guard#4301
agalin920 wants to merge 3 commits into
devfrom
fix/3895-monaco-json-worker-null-guard

Conversation

@agalin920

Copy link
Copy Markdown
Contributor

Closes #3895

monaco-editor 0.25.2's JSONWorker.getFoldingRanges passes the result of _getTextDocument(uri) straight into the JSON language service without checking it. When a folding request has been posted to the worker and the model is disposed before the worker answers — closing the JsonViewer dialog, or switching files in the Code app, both of which dispose the model — that lookup returns null, the language service calls document.getText(), and the rejection reaches Sentry through onUnexpectedError. That is the reported frame. Upstream added if (!document) { return []; } to the method in 0.31.0; 0.30.1 is the last unguarded release. This bumps monaco-editor to ^0.31.1, which carries the same guard for the CSS worker's getFoldingRanges and findDocumentHighlights (the sibling crash in the closed #3909), and it is the substance of #4254. monaco-editor-webpack-plugin moves 4.2.0 → 7.1.1 because 4.x peers only 0.25–0.28; react-monaco-editor stays at 0.47.0, and its peer on monaco-editor@^0.31.1 — which npm ls currently reports as unsatisfied — becomes satisfied. No application code changes: none of the twelve monaco symbols used in src/ is in the 0.25→0.31 breaking set, and the three feature names in the webpack allowlist that 0.31.1 no longer knows (colorDetector, goToDefinitionCommands, goToDefinitionMouse) are already dead against the 4.2.0 table today, so nothing changes there either. The guard is present in the emitted artifact, not just in node_modulesbuild/json.worker.js:19456. The whole dependency delta is these two packages; no transitive version moves across the other 1,528. The dev bundle gets smaller: 75.1 MB → 69.9 MB (−5.1 MB, −6.9%), mostly ts.worker.js and vendors.js.

No reviewer cold pass ran on this diff — this session's harness ruled out subagents, so the review gate in docs/definition-of-done.md §5 was not met and the diff has been read only by the context that wrote it.

monaco-editor 0.25.2's JSONWorker.getFoldingRanges passes a null document
straight into the language service when the model was disposed after the
folding request was posted, which is the getText() crash in #3895. Upstream
added the guard in 0.31.0.

monaco-editor-webpack-plugin moves 4.2.0 -> 7.1.1 because 4.x peers only
0.25-0.28. react-monaco-editor stays at 0.47.0 and its peer on
monaco-editor@^0.31.1, currently unsatisfied, becomes satisfied.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

QA Review — ✅ PASS

Validates #3895: Error: Cannot read properties of null (reading 'getText')

  1. ✅ Fixes the reported null-dereference by bumping monaco-editor to ^0.31.1 (past 0.31.0, where upstream added the if (!document) return [] guard to JSONWorker.getFoldingRanges), matching the root cause described for the Sentry frame in json.worker.js.
  2. ✅ Companion tooling bump (monaco-editor-webpack-plugin ^4.0.0^7.1.1) is required for the new monaco-editor version — its 4.x peer range only covers 0.25–0.28, so this keeps the plugin/editor pair installable together.
  3. ⚠️ No regression in existing Monaco-dependent features (code editor, diff viewer, JSON field viewer) — the diff touches only package.json/package-lock.json, so whether the 0.25→0.31 jump breaks any consumer (MonacoSetup.js, Differ.js, JsonViewer.tsx, etc.) can't be confirmed without installing and running the app.
Suggested Cypress coverage

cypress/e2e/code/actions.spec.js already covers opening the diff/compare view (/code/file/views/.../diff/...); it could be extended with a case that opens a file in the Code app editor and then rapidly switches to a different file (or navigates away) before the editor has settled, to catch worker-vs-disposed-model races like the one in this issue. cypress/e2e/code/all-files-page.spec.js could add a case opening several files in sequence to exercise repeated model disposal. There's no existing spec for JsonViewer.tsx (the other code path — src/shell/components/FieldTypeIntegration/Shared/JsonViewer.tsx — that this same worker guard protects when its dialog is closed mid-request); a spec opening and immediately closing that JSON viewer dialog on a content field would help lock in the fix on that surface too.

@agalin920

Copy link
Copy Markdown
Contributor Author

On ⚠️ 3 — the consumer regression check was run locally, it just isn't visible from the diff. Against this branch with the bump installed:

  • npm run build:devwebpack 5.109.2 compiled successfully, all five language workers emitted.
  • cypress/e2e/code/actions.spec.js 1/1 — this is the MonacoDiffEditor/Differ surface.
  • cypress/e2e/code/sidebar.spec.js 1/1 and cypress/e2e/code/all-files-page.spec.js 8/8 — the Code app editor.
  • cypress/e2e/schema/integration.spec.js 40/40 — this is the JsonViewer.tsx surface. integration.spec.js:309 (View Item's JSON data) opens the dialog, asserts on .monaco-editor.integrationJsonViewerEditor, and then clicks jsonCodeViewerCloseButton, which is the unmount-and-dispose path this guard protects. So the "no existing spec for JsonViewer.tsx" note in the suggestion is not right — there is one, and it passes.
  • tsc --noEmit exits 0 both before and after the bump.

Also worth recording since it is the other half of "does the 0.25→0.31 jump break a consumer": every monaco symbol referenced anywhere in src/ is monaco.Selection, monaco.Uri.from, monaco.editor.{createModel,defineTheme,getModel}, and monaco.languages.{register,registerCompletionItemProvider,setLanguageConfiguration,setMonarchTokensProvider,CompletionItemKind.Property,IndentAction.*}. None of them is in the 0.25→0.31 breaking set — no getModeId, no KeyCode/KeyMod, no IPasteEvent.mode.

On the suggested race spec: declining it deliberately, same reasoning that closed #3909. The race has never reproduced on demand — it has no confirmed reproduction from the reporter, from Sentry, or from us — so a spec that rapidly switches files and asserts no error appears would pass identically with this bump reverted, which makes it a spec that cannot fail rather than coverage. The falsifiable check is on the artifact instead, and it was made: build/json.worker.js:19456 reads let document = this._getTextDocument(uri); if (!document) { return []; }, and the same bytes were served over HTTP from the dev server. The unguarded call site is gone from what ships.

@github-actions

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers

@github-actions

Copy link
Copy Markdown
Contributor

Negative QA

🔴 Code editor crashes to an error boundary on every file open after the monaco-editor 0.31.1 bump

Type: regression from this PR
Steps:

  1. Go to /code.
  2. Click any file in the tree (tested qa_negative_test under VIEWS, ZUID 11-d4b7b6ebf8-lcl4r0) or navigate directly to /code/file/views/11-d4b7b6ebf8-lcl4r0.
  3. Observe the editor pane immediately replaces itself with "Oops, something went wrong" and a webpack dev-server "Uncaught runtime errors" overlay.

Expected: The Monaco editor loads the file content with the custom "parsleyDark" theme applied, as before the version bump.
Actual: MonacoEditor.editorDidMount calls editor.updateOptions({ theme: "parsleyDark" }) (src/apps/code-editor/src/app/components/Editor/components/MemoizedEditor/MemoizedEditor.js:141-143), which throws inside monaco-editor's StandaloneThemeServiceImpl._updateThemeOrColorMap: TypeError: Cannot read properties of undefined (reading 'editor.foreground'). This crashes the <MonacoEditor> component and is caught by the page's ErrorBoundary, replacing the entire Code app view with an error page. The custom theme is registered in MonacoSetup.js via monaco.editor.defineTheme("parsleyDark", { base: "vs-dark", inherit: true, rules: ParsleyTheme }) — its rule/color format is apparently no longer compatible with monaco-editor 0.31.1's theme service, which now expects color map entries it isn't finding, hitting editor.foreground as undefined. This is not scoped to the "parsley" (HTML view) language specifically — the same editorDidMount path runs for every file type opened in the code editor (views, stylesheets, scripts), so the whole Code app is unusable in its current state.
Console/network: TypeError: Cannot read properties of undefined (reading 'editor.foreground') at get tokenTheme_updateThemeOrColorMap_updateActualThemesetThemeStandaloneEditor.updateOptionsObject.editorDidMount (MemoizedEditor.js:141) → React logs "React will try to recreate this component tree from scratch using the error boundary". Reproduced twice from a fresh navigation each time (once on a view file, once on main.js under scripts), identical stack both times.

Code editor crashed with parsleyDark theme error

Also checked and working correctly
  • File tree navigation (sidebar links, VIEWS/SITE.CSS/SITE.JS groupings, "All Files" list) renders correctly on /code before opening any individual file.
  • The error boundary is scoped to the Code editor content pane only — the app shell (left nav, topbar) stays interactive after the crash, so navigating to another app (e.g. Content) still works.
  • Content editor's relational field pickers ("Related Item", "Related Items" on the qa_negative_test model) render and their "More Options" menu (Draft/Production Preview, Copy ZUID, Remove) opens correctly — unaffected, since this menu isn't the Monaco-backed JSON viewer.

…r the editor

monaco 0.31's StandaloneTheme.tokenTheme getter reads
themeData.colors["editor.foreground"] without guarding, so a defineTheme call
with no colors key throws on the first setTheme. That is editorDidMount, so
every file opened in the Code app landed in the ErrorBoundary. colors is a
required member of IStandaloneThemeData on 0.25.2 too; monaco simply never
read it before, and MonacoSetup.js is .js so tsc never checked it.

Empty is behaviour-preserving: both lookups stay undefined and no default
token rule is synthesised, so tokens render as they did on 0.25.2.

Adds cypress/e2e/code/editor.spec.js because no existing spec asserted the
editor renders - sidebar.spec.js stops at location.pathname and
actions.spec.js exercises the diff editor. The new spec fails without this
fix and passes with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@agalin920

Copy link
Copy Markdown
Contributor Author

Confirmed and fixed in f4c822408. This was a real regression from the bump and the diagnosis above is correct down to the call site — thank you.

Root cause, exactly as reported. monaco 0.31 added this to StandaloneTheme's tokenTheme getter (standaloneThemeServiceImpl, "Pick up default colors from editor.foreground and editor.background if available"):

const editorForeground = this.themeData.colors['editor.foreground'];

Unguarded. MonacoSetup.js calls defineTheme("parsleyDark", { base, inherit, rules }) with no colors key, so themeData.colors is undefined and the read throws. 0.25.2's tokenTheme getter has no themeData.colors access at all, which is why this only appears now.

Worth recording: colors is a required member of IStandaloneThemeData in 0.25.2 as well — the call has always violated the type, monaco just never read the field. It went unnoticed because MonacoSetup.js is .js, so tsc never type-checks it.

Fix is colors: {}, which is behaviour-preserving rather than a papering-over: both lookups stay undefined, no default token rule is synthesised, and tokens render exactly as on 0.25.2.

On why the existing suite stayed green — that is the more useful half of this finding. No spec asserted the Code editor renders. code/sidebar.spec.js clicks a tree item and asserts location.pathname, which is unchanged by a crash inside the editor pane's ErrorBoundary; code/actions.spec.js exercises MonacoDiffEditor, a different mount path that never runs editorDidMount's updateOptions({theme}). So all 50 tests I ran passed through a completely broken Code app. cypress/e2e/code/editor.spec.js is added in the same commit and asserts the editor actually mounts — verified falsifiable: it fails with Expected to find element: .react-monaco-editor-container, but never found it on the previous commit and passes on this one.

Re-verified locally on f4c822408: code/editor 1/1, code/actions 1/1, code/sidebar 1/1, code/all-files-page 8/8, schema/integration 40/40, tsc --noEmit exit 0, build:dev compiled successfully.

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Overall Coverage

Lines Statements Branches Functions
61.29% 60.87% 56.25% 55.65%

Changed Files Coverage

File Lines Statements Branches Functions
src/apps/code-editor/src/app/components/Editor/components/MemoizedEditor/MonacoSetup.js 25.00% 23.80% 0.00% 16.66%

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error: Cannot read properties of null (reading 'getText') Error: Cannot read properties of null (reading 'languageId')

1 participant