Replace Ace Editor with Monaco Editor #801

bennothommo · 2022-12-13T13:37:54Z

Fixes #431. Related: wintercms/wn-builder-plugin#73

This PR introduces the Monaco editor in replacement of the Ace Editor. At this stage for the PR, we are aiming for parity with the functionality previously provided by Ace, with the intention of increasing the functionality after this PR is merged.

Still to do:

Support all prior configuration options.
Re-implement all previous themes, or find suitable alternatives (see https://github.com/brijeshb42/monaco-themes)
Ensure code editor works in all current core locations (settings areas, CMS editor, Builder plugin)
Try and figure out how to remove the automated loading of the Codicon font, as it's being implemented in the widget's CSS assets.

Changes:

Auto-completing tags preference is no longer available, as Monaco doesn't have this as an option.
Autocompletion and code snippets preferences are no longer available, as these are more contextual and will be implemented based on what is being edited in the editor.
Code folding is now a simple boolean.
The Crimson Editor theme has been dropped - I couldn't find a complete replica of this theme, and it looks too similar to other themes in the list.

Summary by CodeRabbit

New Features
- Replaced legacy editor with a modern Monaco-based editor: minimap, bracket colorization, inline color previews, richer language services, and many new themes.
Improvements
- Consolidated editor preferences into a unified plugin for smoother settings.
- Modernized editor styling and toolbar; adjusted tab collapse icon colors and layout.
Bug Fixes
- Addressed duplicate render handling that could cause double render invocation.
Documentation
- Added a README describing the new editor integration.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Add most configuration options - Dispose and create editor depending on whether in view - Fix sizing issue

modules/backend/ServiceProvider.php

modules/backend/assets/ui/js/ui/WidgetHandler.js

modules/backend/controllers/preferences/_field_editor_preview.php

…d theme switch

…wip/monaco-editor

… events for editor

Markdown editor needs some more work, we might look at using TipTap or Milkdown / Crepe in the future.

Interacting with the PackageManager triggers a need to use the database connection to get the active theme, the database connection is not available until the boot process.

…et() function

…educe issues caused by timing issues with webpack loading

…plugin

Stale

For anyone else trying to track down self being clobbered, this is very handy: (function () { try { const desc = Object.getOwnPropertyDescriptor(window, 'self'); console.log('window.self descriptor:', desc); let _selfValue = window; Object.defineProperty(window, 'self', { configurable: true, get() { return _selfValue; }, set(v) { console.error('window.self set to:', v); console.trace('stack:'); // <- prints the exact script/line _selfValue = v; }, }); } catch (e) { console.warn('Failed to trap window.self:', e); } })();

Also added build cleanup step to build process to avoid leaving behind artifacts from previous builds.

coderabbitai

Actionable comments posted: 7

🤖 Fix all issues with AI agents

In
@modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/612.js:
- Around line 1-8: The bundle at variables R and A is an old Monaco SQL language
build (v0.34.1); update the monaco-editor dependency to the current stable
(e.g., 0.54.0), regenerate the editor language bundle (so R and A are rebuilt
with the latest tokenizer/keywords/operators/builtinFunctions), replace the
generated codeeditor.bundle/612.js artifact with the newly built output, and
commit the regenerated file (ensure package.json and lockfile reflect the
monaco-editor version change and run the same build script used to produce the
original bundle).

In @modules/backend/formwidgets/codeeditor/assets/js/codeeditor.js:
- Around line 644-649: The findAll method incorrectly passes a hard limit of 1
to this.model.findMatches, so it only returns a single match; update findAll
(method name: findAll) to call this.model.findMatches without the
limitResultCount parameter (or pass null/undefined) so it returns all matches
for searchString (constructed from search) with the existing flags (isRegex,
matchCase, etc.).
- Around line 1515-1526: The resolveCodeLens implementation in addCodeLens calls
resolver(model, codeLens, token) even when resolver is null, causing a
TypeError; change it to only invoke resolver when it's provided (e.g. use
optional chaining resolver?.(model, codeLens, token) ?? codeLens or check typeof
resolver === 'function' and call it, otherwise return codeLens) while keeping
provideCodeLenses unchanged; update the resolveCodeLens lambda in addCodeLens
accordingly.
- Around line 679-689: The alignment call uses search.startColumn which is
undefined when search is a String/RegExp; update the call in the
pushEditOperations block to pass a valid start column (use
found.range.startColumn as fallback or compute const startCol = search &&
search.startColumn ? search.startColumn : found.range.startColumn) so
alignText(replace, startCol) always receives a number; modify the invocation
around this.model.pushEditOperations / alignText to use that startCol.
- Around line 196-212: The dispose() method currently removes the window resize
listener but not the document click listener, causing accumulated click
handlers; update attachListeners() to store the click handler reference (e.g.,
assign the function to this.clickListener or register it in
this.callbacks.click) when calling document.addEventListener('click', ...), and
then update dispose() to call document.removeEventListener('click',
this.clickListener) (and clear this.clickListener) so the click listener is
properly removed during disposal; ensure the same function reference is used for
add/remove and that attachListeners and dispose both reference the same symbol
(attachListeners(), dispose(), this.clickListener or this.callbacks.click).
- Around line 1437-1466: normalizeKeyBinding currently only recognizes a few
hard-coded modifier orderings leaving key null for plain keys and many modifier
combos; change the string parsing to split the keyCode on '+' (e.g., parts =
keyCode.split('+')), iterate parts to set ctrl/alt/shift flags when part matches
those modifiers, and set keyBinding.key to the remaining non-modifier part
(joined or last non-empty piece) so plain keys like "Escape", single modifiers
like "Shift+X", and any order combos like "Ctrl+Alt+Shift+K" are all handled;
keep the existing branch that merges when keyCode is already an object.
- Around line 1248-1273: The fullscreen click handler in enableStatusBarActions
adds a new listener on every editor recreate; change it to register a single
reusable handler: create/store a named function (e.g.
this.callbacks.fullscreenClick) and before adding call
fullscreen.removeEventListener('click', this.callbacks.fullscreenClick) to clear
any prior listener, then assign and add the listener; ensure you reference the
same stored handler when removing so duplicated listeners are not accumulated
(also keep existing use of this.callbacks.fullScreenChange and the
editor.layout() call inside the stored handler).

🧹 Nitpick comments (3)

modules/backend/formwidgets/codeeditor/assets/css/codeeditor.css (1)
1-1: Consider using CSS custom properties for theme colors.

Hardcoded color values (#d1d6d9, rgba(0,0,0,.8), etc.) are scattered throughout. For better theming consistency—especially with Monaco's dark/light theme support—consider using CSS variables:
:root {
  --codeeditor-border-color: #d1d6d9;
  --codeeditor-toolbar-bg: rgba(0, 0, 0, 0.8);
}
This would align with the PR's theme-related changes and simplify future theme adjustments.
modules/backend/formwidgets/codeeditor/assets/js/codeeditor.js (2)
1047-1057: Redundant regex match after validation.

The color parsing now correctly returns early on invalid format (line 1052), but line 1054 redundantly calls color.match() again instead of reusing the validation result. Minor inefficiency.
♻️ Suggested optimization
-            if (!color.match(/^#(..)(..)(..)(..)$/)) {
+            const rgbaMatch = color.match(/^#(..)(..)(..)(..)$/);
+            if (!rgbaMatch) {
                 // Invalid color format - return original color
                 return color;
             }
-            const rgba = color.match(/^#(..)(..)(..)(..)$/).slice(1).map((c) => parseInt(c, 16));
+            const rgba = rgbaMatch.slice(1).map((c) => parseInt(c, 16));
             rgba[3] = (rgba[3] / 0xFF).toPrecision(2);
             return `rgba(${rgba.join(', ')})`;
788-791: Silent theme loading failure hides errors.

The catch block at lines 788-790 silently swallows errors, which could make debugging difficult when theme files are missing or malformed.
♻️ Suggested improvement
                 }).catch(() => {
-                    // Theme loading failed - silently fall back to default theme
+                    // Theme loading failed - log in development, fall back silently in production
+                    if (process.env.NODE_ENV !== 'production') {
+                        console.warn(`CodeEditor: Failed to load theme "${newTheme}", falling back to default`);
+                    }
                 });
Alternatively, consider emitting an event so consuming code can react:
this.events.fire('themeLoadError', newTheme);

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4294755 and 4f73c68.

📒 Files selected for processing (28)

modules/backend/formwidgets/CodeEditor.php
modules/backend/formwidgets/codeeditor/assets/css/codeeditor.css
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/129.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/237.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/309.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/504.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/566.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/612.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/641.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/683.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/74.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/744.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/754.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/758.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/847.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/897.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/919.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/952.js
modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/99.js
modules/backend/formwidgets/codeeditor/assets/js/build/css.worker.js
modules/backend/formwidgets/codeeditor/assets/js/build/editor.worker.js
modules/backend/formwidgets/codeeditor/assets/js/build/html.worker.js
modules/backend/formwidgets/codeeditor/assets/js/build/json.worker.js
modules/backend/formwidgets/codeeditor/assets/js/build/ts.worker.js
modules/backend/formwidgets/codeeditor/assets/js/codeeditor.js
modules/backend/formwidgets/codeeditor/assets/winter.mix.js
modules/backend/models/Preference.php

🧰 Additional context used

🪛 Biome (2.1.2)

modules/backend/formwidgets/codeeditor/assets/js/build/codeeditor.bundle/683.js

[error] 7-7: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Safe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

[error] 7-7: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Safe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

[error] 7-7: This case is falling through to the next case.

Add a break or return statement to the end of this case to prevent fallthrough.

(lint/suspicious/noFallthroughSwitchClause)

[error] 7-7: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Safe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

[error] 7-7: This variable is used before its declaration.

The variable is declared here: