Skip to content

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

Open
agalin920 wants to merge 4 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 4 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

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

QA Review — ⚠️ INCONCLUSIVE

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

  1. ✅ Upgrade past the unguarded monaco-editor release that causes the null-model getFoldingRanges crash — package.json/package-lock.json bump monaco-editor 0.25.2 → 0.31.1 and monaco-editor-webpack-plugin 4.2.0 → 7.1.1 to satisfy its peer range.
  2. ⚠️ Editor continues to render/theme correctly after the major version bump — MonacoSetup.js adds a previously-absent colors: {} to defineTheme, implying a breaking change was hit, but nothing in the diff exercises theme rendering to confirm it's fully resolved.
  3. ✅ Regression coverage added to catch editor-mount breakage — new cypress/e2e/code/editor.spec.js asserts the Monaco container and view-lines render when opening a view file.
Suggested Cypress coverage

cypress/e2e/code/editor.spec.js (new in this PR) only checks that the editor mounts. To lock in the actual crash scenario from #3895, add a case that opens a file, waits for the JSON/folding worker to be engaged (e.g. collapse/expand a folding region), then immediately closes the JsonViewer dialog or navigates to a different file before the worker response returns — asserting no uncaught exception surfaces (via a cy.on("uncaught:exception") failure guard) and the app remains interactive. A second case switching rapidly between two code files would cover the "model disposed mid-request" path called out as the root cause.

@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

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Negative QA

No reproducible edge-case failures found on the surfaces this PR changes.

Also checked and working correctly
  • Cold-loading the Code app and opening a parsley/HTML view (/code/file/views/...) — Monaco mounts cleanly with the parsleyDark theme (dark background, syntax highlighting) and no console errors, confirming the colors: {} guard in MonacoSetup.js prevents the pre-upgrade crash.
  • Opening existing LESS (main.less) and JavaScript (main.js) files — correct language mode, no console errors.
  • Creating a new file via "Custom File Type/Endpoint" with a .json extension and typing deliberately invalid JSON ({"a": <b>x</b>, "b": {{a.b}}, "c":) — the JSON language worker still initializes correctly post-upgrade: syntax-error squiggles and autocomplete ("Empty array"/"Empty object") both work, no worker crash.
  • Save and Save & Publish on the new JSON file, including immediately reopening Diff Versions afterward — version incremented correctly (v1 → v2), diff editor rendered the red/green line diff correctly with no console errors.
  • Opening the built-in Find widget (Ctrl+F) and the editor right-click context menu ("Change All Occurrences") on a parsley view — both render with correct theme colors, no unstyled/invisible widget.
  • Rapidly switching between four different open files of different types/languages (JSON → JS → LESS → JSON) back-to-back without waiting for network settle — no console errors, no stale/mismatched editor content, model disposal handled cleanly.
  • Manually invoking the parsley completion provider (globals. + Ctrl+Space) — provider fires without error (returned only the built-in word-based suggestion since this instance's clippings global model has no matching fields; no crash or console error).
  • Opening a permission-locked view file ("loader", shown with a lock icon in the sidebar) — loads without error.

…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>
Comment on lines +9 to +13
cy.get(".react-monaco-editor-container", { timeout: 20000 }).should(
"exist"
);
cy.get(".monaco-editor .view-lines", { timeout: 20000 }).should(
"be.visible"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Class-based selectors violate the data-cy-only Cypress rule
CLAUDE.md is explicit: "class-based selectors … are flake risks" and "data-cy is the only selector strategy." Both .react-monaco-editor-container and .monaco-editor .view-lines are internal class names emitted by react-monaco-editor/Monaco and can silently break if the library renames them. Add data-cy="monacoEditor" to the wrapper <div> (or the ReactMonacoEditor host element) inside MemoizedEditor, then replace these two selects with cy.getBySelector("monacoEditor").should("exist") and cy.getBySelector("monacoEditor").find(".view-lines").should("be.visible")— the inner .view-lines assertion still relies on a Monaco internal but at least is scoped inside a codebase-controlled anchor.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments

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