Skip to content

chore(frontend): upgrade frontend to monaco-languageclient v10#4997

Open
aglinxinyuan wants to merge 25 commits intoapache:mainfrom
aglinxinyuan:chore/monaco-lsp-v10
Open

chore(frontend): upgrade frontend to monaco-languageclient v10#4997
aglinxinyuan wants to merge 25 commits intoapache:mainfrom
aglinxinyuan:chore/monaco-lsp-v10

Conversation

@aglinxinyuan
Copy link
Copy Markdown
Contributor

@aglinxinyuan aglinxinyuan commented May 9, 2026

What changes were proposed in this PR?

Brings the frontend's Monaco / VS Code language-client stack up to monaco-languageclient v10 (from v8.8.3) and the matching @codingame/monaco-vscode-* v25.1.2 ecosystem. v10 split the old single-wrapper API into three composable pieces (MonacoVscodeApiWrapper + EditorApp + LanguageClientWrapper), so the editor component is rewritten and the build/test pipeline picks up the codingame v25 quirks the new packages introduce. Adjacent dep cleanups land here too — several codingame side-deps and the R UDF editor become superfluous once v10 is in place.

API shape: v8 → v10

v8 (before)                                      v10 (after)
─────────────────                                ──────────────────────────────────
new MonacoEditorLanguageClientWrapper(           const api  = new MonacoVscodeApiWrapper(...)
  // single config with editor + LSP + workers   const app  = new EditorApp(...)
  // all in one bag                              const lc   = new LanguageClientWrapper(...)
).start(htmlContainer)                           await api.start();         // global vscode-api init
                                                 await app.start(container);// mount editor
                                                 await lc.start();          // open WebSocket to LS

The editor component is rewritten against the new shape, plus a whenReady() handshake on the default language extensions and a post-mount forceTokenization loop to fix a TextMate first-paint race that otherwise renders every token as the default mtk1 class.

Worker bundling: why three trampolines

new Worker(new URL("./relative", import.meta.url)) is the only shape webpack 5 treats as a worker entry point AND that esbuild can resolve at spec pre-bundle time. Inlining new URL("@codingame/...", import.meta.url) makes webpack happy but esbuild treats the spec as a literal relative URL and the spec pre-bundle fails. A fileReplacements-swapped factory file works but adds an angular.json delta. Three thin re-export trampolines satisfy both bundlers with zero angular.json change.

What gets dropped

  • @codingame/monaco-vscode-textmate-service-override — now a transitive of the v25 wrapper; not needed at top level
  • @codingame/monaco-vscode-theme-defaults-default-extension — same; pulled in transitively by the language extensions
  • @codingame/monaco-vscode-r-default-extension — upstream stopped publishing; R UDF editor retired
  • vscode (package alias + resolutions) — v10 no longer needs the alias for type-only vscode.Uri references
  • webpack-bundle-analyzer + analyze script — dead; script never ran in CI
  • content-disposition + @types/content-disposition — dead require, never read

Any related issues, documentation, discussions?

Closes #4996.

How was this PR tested?

  • yarn install resolves cleanly
  • yarn build exits 0; worker chunks identical hashes/sizes (~1.3 MB textmate, ~518 KB ext-host, ~220 KB editor)
  • yarn test stays at 63 / 269 spec parity

Manual smoke-test plan:

  • Open a workflow with a Python UDF — confirm syntax highlighting renders correctly on first open
  • Set / clear / step through a breakpoint with the debugger panel
  • Verify the AI-assistant "Add Type Annotation" / "Add All Type Annotations" actions still work
  • Check that two browser tabs editing the same workflow show each other's coeditor cursors
  • Run a Java UDF and confirm the editor shows Java syntax (no LSP, just grammar)

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.7)

Bumps monaco-languageclient 8.8.3 -> 10.7.0 and the @codingame/monaco-vscode-*
family 8.0.4 -> 25.1.2. Drops monaco-editor-wrapper entirely; the editor
component now drives the underlying triplet (MonacoVscodeApiWrapper +
LanguageClientWrapper + EditorApp) directly.

Notable changes:

* code-editor.component.ts rewritten against the v10 API. The vscode-api
  wrapper is started once per process; the default-extension activations
  await each extension's whenReady() so the TextMate grammars register
  before the editor opens, and a forceTokenization pass on first paint
  guarantees colours land before Monaco's initial render.
* Worker bootstraps live in src/.../workers/. The codingame-shipped worker
  files re-export internals via single-line imports that webpack only emits
  as static assets, so the trampolines force webpack to bundle the worker
  dep tree under the new Worker(new URL(...)) pattern.
* custom-webpack.config.js gains a oneOf rule that runs codingame CSS
  through css-loader with exportType: css-style-sheet, asset/resource for
  the bundled svg/ttf/png/woff, and an alias that redirects the css/style
  loader runtime back to its real install (codingame's deeper namespace
  breaks the relative paths css-loader generates). Patches a known
  empty-path crash in license-webpack-plugin.
* jsdom polyfill adds Constructable Stylesheets, matchMedia, CSS.escape,
  and registers a Node ESM loader hook (jsdom-css-loader-hook.mjs) that
  short-circuits .css imports to an empty module - needed because
  @angular/build:unit-test pre-bundles specs with externalPackages:true,
  so server.deps.inline never sees the codingame chain.
* All five previously-excluded specs (code-editor, codearea-custom-template,
  time-travel, versions-list, operator-property-edit-frame) run again.
* LICENSE-binary regenerated from dist/3rdpartylicenses.json (119 -> 123
  packages: more codingame sub-modules; vscode-textmate / vscode-oniguruma
  / monaco-editor-wrapper dropped; dompurify added).

Verified: yarn build, yarn test (63 files, 269 tests, parity with main),
yarn format:ci. Browser smoke test against a running backend confirmed
Python TextMate highlighting, editor mount, and worker spawn.
The package's npm tarball ships an older BSD-3-Clause LICENSE file but
the d3-path project's upstream LICENSE (https://github.com/d3/d3-path)
is ISC. license-webpack-plugin reported BSD-3-Clause off the tarball,
which is the historical artifact rather than the project's current
license. Move it back into the ISC bucket alongside the rest of the d3
ecosystem.
* download.service.ts had `var contentDisposition = require("content-disposition")`
  that was never read or called — removed the dead require, then dropped both
  `content-disposition` and `@types/content-disposition` from package.json.
* `webpack-bundle-analyzer` was only referenced from the `analyze` npm script
  and never invoked from CI. Dropped both the dep and the script.
The v10 commit listed every codingame service-override package texera
might touch, but six of them are pulled in transitively by
monaco-languageclient itself (or by another codingame override) and were
never imported from texera's source. Drop the redundant declarations:

  - configuration-service-override   (mlc dep)
  - editor-service-override          (mlc dep)
  - extensions-service-override      (mlc dep)
  - languages-service-override       (mlc dep)
  - theme-service-override           (mlc dep)
  - keybindings-service-override     (pulled in via views- and
                                      workbench-service-override)

Bundle output and LICENSE-binary are unchanged — only package.json gets
shorter. Verified with yarn build + yarn test (63 files, 269 tests).
The trampoline at `workers/textmate.worker.ts` deep-imports
`@codingame/monaco-vscode-textmate-service-override/worker`, but the
package is also a direct dependency of `monaco-languageclient@10.7.0`,
so the resolution still works without an explicit declaration. Drop the
dep and let mlc pin it.
…ension

`code-editor.component.ts` `import("@codingame/monaco-vscode-theme-defaults-default-extension")`
still resolves: the package is a direct dependency of
`monaco-languageclient@10.7.0`. Same pattern as the previous textmate
override drop — let mlc pin it.
Drops the R syntax-highlighting branch from the code editor + the
@codingame/monaco-vscode-r-default-extension dependency. The backend
RUDF / RUDFSource operators still exist; opening their code editor now
falls through to the default Java highlighting (degraded but functional).

* code-editor.component.ts: remove the R language detection branch and
  the .r file-suffix mapping; drop the dynamic-import of the R default
  extension from the bootstrap chain.
* code-debugger.component.ts: drop the side-effect import of the R
  default extension.
* package.json + LICENSE-binary: drop the codingame R extension.

Verified: yarn build, yarn test (63 / 269 — parity).
Both consumers — texera's own `dependencies.vscode` and
`monaco-languageclient@10.7.0`'s `dependencies.vscode` — already alias
to `npm:@codingame/monaco-vscode-extension-api`, so yarn unifies them
without a `resolutions` entry. Verified yarn.lock still resolves
`vscode@npm:` to the codingame extension api.

The `monaco-editor` resolution stays — it's load-bearing because
`monaco-breakpoints` declares `monaco-editor: ^0.39.0` (vanilla) and
without forcing the alias yarn would install two competing Monaco
copies.
No texera source file imports from `vscode`. The package was only there
to align with the codingame convention (`vscode` aliased to
`@codingame/monaco-vscode-extension-api`). `monaco-languageclient@10.7.0`
already declares the same alias as one of its dependencies, so the
package still installs transitively for any dep chain that needs it.
The codingame worker bundles were previously pulled in via three single-line
trampoline files in `code-editor-dialog/workers/`, each re-exporting one
upstream worker entry. Webpack 5 recognises `new Worker(new URL("@pkg/...",
import.meta.url))` natively and bundles the full dep tree into a worker
chunk, so the trampolines aren't needed for the production build.

The test pipeline blocks the truly-inline form: `@angular/build:unit-test`
runs `@angular/build:application` (esbuild) regardless of `customWebpackConfig`,
and esbuild resolves `new URL(spec, import.meta.url)` literally relative to
the source file rather than through node_modules. To keep tests buildable,
the URL refs live in a single sibling file that's swapped for a no-op stub
via `fileReplacements` on a new `gui:build-test` target.

  Before                                 After
  ────────────────────────────────────── ──────────────────────────────────────
  workers/editor.worker.ts               codingame-worker-factory.ts
  workers/extension-host.worker.ts       codingame-worker-factory.stub.ts
  workers/textmate.worker.ts
  (3 opaque trampolines)                 (1 readable factory + 1 test stub)

Worker chunks are byte-identical to the pre-refactor build (same content
hashes, same sizes: 1.3MB textmate / 518KB ext-host / 220KB editor).
Tests stay at 63 / 269 parity.
The Node ESM loader hook that strips transitive `.css` imports during specs
used to live in its own `src/jsdom-css-loader-hook.mjs` sidecar, registered
via `module.register(pathToFileURL(...))`. `module.register` accepts any
module URL though, including `data:` URLs — folding the hook source into
the registration call lets the sidecar file go away entirely.

The vitest.config.ts cross-reference comment that pointed at the hook is
also dropped since it duplicates what the polyfill file already documents.

Tests stay at 63 / 269 parity.
Tighten dead/redundant patches that crept in during the v10 migration:

* Drop the unused `getEnhancedMonacoEnvironment` import — its only consumer
  lives in `codingame-worker-factory.ts` now.
* Replace the 3-way `||` operator-type check with a `static readonly Set`
  + `.has(...)`; fold `generateLanguageTitle` into `setLanguage`.
* `ngOnDestroy`: drop `isDefined(monacoBinding)` (optional chaining) and
  `isDefined(workflowVersionStreamSubject)` (always assigned).
* `initializeMonacoEditor`: drop the redundant
  `switchMap(editor => of(editor ?? editorApp?.getEditor()))` —
  `startEditor()` already returns `editorApp.getEditor()` and `catchError`
  covers the timeout fallback. Remove the now-unused `switchMap` import.
* `acceptCurrentAnnotation`: drop the redundant inner
  `if (currentRange && currentSuggestion)` — the early-return guard above
  already proves both fields are truthy.
* `getCoeditorCursorStyles`: lift `clientId`/`color` to locals (each was
  repeated up to 3× in the same string).
* Remove the orphaned `// NOTE: dynamic imports ...` comment from the
  imports block; the function it references documents the rationale.

Net: -17 lines (650 -> 633). Build exits 0; tests stay at 63 / 269 parity.
Comparing against the TypeFox monaco-languageclient `verify/angular`
example surfaced two clean-ups:

* Drop the dynamic `import("@codingame/monaco-vscode-theme-defaults-default-extension")`
  — that package is no longer a declared dep (removed earlier in this branch);
  the dynamic import only resolved by transitive luck and would silently break
  if any sibling package stops pulling it in.

* Rewrite `ensureVscodeApiStarted` and the diff-editor `from(promise.then(...))`
  pipeline as plain `async`/`await`. The chained `.then().then().then()` shape
  was a holdover; the upstream example does the wrappers linearly.
  `apiWrapperStartPromise` now uses the `??=` idiom so the singleton-init
  is one expression instead of an `if` block.

Worker-factory note: upstream uses `monacoWorkerFactory: configureDefaultWorkerFactory`
which only works on Vite (it sets `getWorkerUrl`, depends on
`@codingame/esbuild-import-meta-url-plugin`). Our webpack pipeline still
needs `registerCodingameWorkers` to set `getWorker` directly so webpack-5
can statically bundle the worker chunks — verified the package paths it
uses match upstream's `defineDefaultWorkerLoaders` verbatim.

Net: -13 lines (633 -> 620). Build exits 0; tests stay at 63 / 269 parity.
…sting test config

Earlier in this branch the worker-factory swap lived on a brand-new
`gui:build-test` target backed by `@angular/build:application`. That was
overkill — `@angular-builders/custom-webpack:browser` already accepts
`fileReplacements` in its schema, and the build target already had a
`configurations.test` slot that the unit-test runner reads through
`gui:build:test`. Moved the replacement entry into that existing slot,
dropped the new build target, and reverted the test target's `buildTarget`
back to `gui:build:test`.

angular.json diff vs main shrinks from ~21 lines (new target + buildTarget
rename + orphan-block removal) to +6 lines (one fileReplacements array).

Build exits 0; tests stay at 63 / 269.
…ngular.json delta

Earlier the worker URL refs were inlined as `new URL("@codingame/...",
import.meta.url)` inside `codingame-worker-factory.ts`, with a
`fileReplacements` swap on `gui:build:test` redirecting to a no-op stub
during specs (esbuild can't resolve package-specifier URLs the way
webpack does). Going back to a relative-path `new URL("./workers/X",
import.meta.url)` pattern lets esbuild resolve the spec against the
filesystem and webpack still treats it as a worker entry.

  Net diff vs main
  ─────────────────────────────────────────────────────────────────────
  angular.json              0 lines  (was +6)
  tsconfig.app.json         +4 lines (the 3 trampolines listed in `files`)
  src/.../workers/*.ts      3 trampoline files (one-line side-effect imports)
  src/.../codingame-...     deleted (factory + stub)

So the worker plumbing now costs +1 source file (3 trampolines vs 2 factory
files) but `angular.json` becomes a no-op vs main and tests + build keep
the same parity (63 / 269 specs, byte-identical worker chunks).
Copilot AI review requested due to automatic review settings May 9, 2026 07:07
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file frontend Changes related to the frontend GUI labels May 9, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Upgrades Texera’s frontend Monaco/VS Code language-client integration to monaco-languageclient v10 and the matching @codingame/monaco-vscode-* v25 stack, updating editor initialization, worker bundling, and build/test plumbing while cleaning up now-unneeded dependencies (including retiring the R UDF editor path).

Changes:

  • Rewrite the code editor integration to the v10 wrapper triplet (MonacoVscodeApiWrapper + EditorApp + LanguageClientWrapper) and add worker trampolines.
  • Update webpack + test environment to handle codingame v25 CSS/assets and jsdom gaps (Constructable Stylesheets, CSS imports under Node ESM, etc.).
  • Dependency/license list updates and removal of unused deps/scripts (e.g., content-disposition, webpack-bundle-analyzer, R extension).

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts Reworked Monaco/LSP startup for monaco-languageclient v10, worker wiring, tokenization workaround.
frontend/src/app/workspace/component/code-editor-dialog/workers/editor.worker.ts Trampoline worker entry for codingame editor worker.
frontend/src/app/workspace/component/code-editor-dialog/workers/extension-host.worker.ts Trampoline worker entry for codingame extension host worker.
frontend/src/app/workspace/component/code-editor-dialog/workers/textmate.worker.ts Trampoline worker entry for codingame textmate worker.
frontend/custom-webpack.config.js Webpack rules/aliases for codingame v25 assets/CSS + license-webpack-plugin workaround.
frontend/src/jsdom-svg-polyfill.ts Vitest/jsdom polyfills + Node ESM loader hook to swallow transitive .css imports.
frontend/vitest.config.ts Removes now-unneeded dependency inlining workaround for CSS imports.
frontend/src/tsconfig.app.json Adds worker trampoline files to compilation inputs.
frontend/src/app/workspace/component/code-editor-dialog/code-debugger.component.ts Removes R extension import to match retired R support.
frontend/src/app/dashboard/service/user/download/download.service.ts Drops unused content-disposition require.
frontend/package.json Bumps monaco-languageclient/codingame deps and removes unused packages/scripts.
frontend/LICENSE-binary Updates third-party license list for new dependency graph.
frontend/yarn.lock Lockfile updates reflecting dependency upgrades/removals.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts Outdated
Comment thread frontend/custom-webpack.config.js Outdated
`format:ci` flagged two backtick-delimited string literals with no
interpolation in `getCoeditorCursorStyles`. Switched them to double
quotes so the file matches prettier-eslint's expected output and the
CI gate passes.

No behaviour change.
CI's `check_binary_deps.py` flagged seven packages claimed by
`LICENSE-binary` that the production build no longer bundles. The bullets
are leftover from earlier dep cleanups in this branch — `r@1.0.0` came
from retiring R UDF support, `content-disposition` from dropping the
dead `require` in `download.service.ts`, and the rest
(`base64-js`, `buffer`, `ieee754`, `path-browserify`, `safe-buffer`)
are browserify shims that became unused once the codingame v25 stack
took over from v8.

`check_binary_deps.py --ignore-transitive-version npm frontend/dist/3rdpartylicenses.json`
now reports `OK: 113 npm packages match LICENSE-binary.`
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 43.33333% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.73%. Comparing base (75ff07e) to head (45fd824).

Files with missing lines Patch % Lines
...ponent/code-editor-dialog/code-editor.component.ts 43.33% 32 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #4997   +/-   ##
=========================================
  Coverage     42.72%   42.73%           
  Complexity     2186     2186           
=========================================
  Files          1031     1031           
  Lines         38152    38179   +27     
  Branches       4004     4002    -2     
=========================================
+ Hits          16302    16317   +15     
- Misses        20831    20846   +15     
+ Partials       1019     1016    -3     
Flag Coverage Δ *Carryforward flag
access-control-service 39.53% <ø> (ø) Carriedforward from 75ff07e
agent-service 33.72% <ø> (ø) Carriedforward from 75ff07e
amber 43.22% <ø> (ø) Carriedforward from 75ff07e
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from 75ff07e
config-service 0.00% <ø> (ø) Carriedforward from 75ff07e
file-service 32.18% <ø> (ø) Carriedforward from 75ff07e
frontend 33.12% <43.33%> (+0.04%) ⬆️
python 88.90% <ø> (ø) Carriedforward from 75ff07e
workflow-compiling-service 47.72% <ø> (ø) Carriedforward from 75ff07e

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…earing

Probed each block by removing it and re-running yarn build:ci /
check_binary_deps.py / yarn test. Dropped the parts that webpack 5
defaults or codingame v25 already cover, kept the parts that produce
real `Module parse failed` / license-check failures when removed.

Removed (verified dead via probe):
* license-webpack-plugin WebpackFileSystem monkey-patch (38 lines).
  `handleMissingLicenseText: () => null` already absorbs the ENOENT cases.
* `module.parser.javascript.url: true`. Webpack 5 enables this by default.
* `resolve.alias` for `@codingame/css-loader` -> `css-loader` (13 lines).
  codingame v25 no longer trips the path-resolution quirk that was
  emitting `node_modules/@codingame/css-loader/...` import specs.
* `excludedPackageTest: (name) => !name`. Subsumed by handleMissingLicenseText.

Kept (verified load-bearing):
* `asset/resource` rule for codingame `.svg/.ttf/.png/.woff*`. Without it
  webpack tries to parse the raw assets as JS and the build hits
  `Module parse failed: Unexpected token (1:0)` on the first .svg.
* CSS `oneOf`: codingame -> css-loader with `exportType: 'css-style-sheet'`,
  monaco-breakpoints -> style-loader + css-loader. Dropping either branch
  fails the build (codingame Constructable Stylesheets / breakpoints CSS).
* LicenseWebpackPlugin with custom `renderLicenses` -> bin/licensing/
  check_binary_deps.py reads the resulting JSON array shape.

Also pulled `path` to a top-level constant with a `nodeModule(...)` helper
(was repeating `require("path").resolve(__dirname, "node_modules/...")`)
and rewrote `renderLicenses` with optional chaining + a simpler sort.

Net: 154 -> 84 lines (-69).
yarn build:ci EXIT 0 with byte-identical worker chunks; check_binary_deps.py
reports `OK: 113 npm packages match LICENSE-binary`; yarn test stays at
63 / 269 parity.
Three Copilot review comments on apache#4997 — all valid, addressed in this
commit. (A fourth comment about an unused `const fs = require("fs")` in
custom-webpack.config.js was already resolved by de4d47b when the
license-webpack-plugin patch block was deleted as dead code.)

* Timeout scope: previously `LANGUAGE_SERVER_CONNECTION_TIMEOUT_MS`
  wrapped the entire `startEditor()` promise, including
  `ensureVscodeApiStarted()` and `EditorApp.start()`. On first load
  codingame v25 init can easily exceed 1s, so the timeout could fire
  before the editor mount completes and the rxjs `catchError` fallback
  would emit `undefined` (filtered out → MonacoBinding/actions/debugger
  never attach). Pulled the timeout out of the rxjs pipeline and into a
  `Promise.race` around `languageClientWrapper.start()` only; the editor
  mount is now always awaited to completion. Drops the unused `timeout`
  / `catchError` / `of` rxjs imports.

* Sticky rejection cache: `apiWrapperStartPromise` is shared across
  every CodeEditorComponent instance. If wrapper / extension activation
  ever rejected, the rejected promise would be returned forever and no
  subsequent open could retry. Wrapped the IIFE body in try/catch and
  reset the cache before re-throwing.

* Awareness-driven CSS injection: `getCoeditorCursorStyles` interpolates
  `coeditor.clientId` and `coeditor.color` (both writeable by any peer
  via yjs awareness) into a `<style>` tag passed through
  `bypassSecurityTrustHtml`. Anything that breaks out of the tag would
  land in the page as raw HTML. Added two tight allow-list regexes
  (digits-only id, hex / `rgb(a)` / `hsl(a)` colour) and bail out to an
  empty stylesheet if either fails.

Build + license + tests verified locally:
  yarn build:ci           EXIT 0   (worker chunks byte-identical)
  check_binary_deps.py    OK: 113 npm packages match
  yarn test               63 / 269 (parity)
Sweep over the files this PR touched looking for cleanups that didn't fit
into the focused commits:

* `code-debugger.component.ts`: drop the two static
  `import "@codingame/monaco-vscode-{python,java}-default-extension"` side-
  effect imports. They duplicate the dynamic imports inside
  `CodeEditorComponent.ensureVscodeApiStarted()` (which already loads them
  after `MonacoVscodeApiWrapper.start()` and awaits each `whenReady()`),
  and are documented elsewhere in this file as tree-shaken by the Angular
  build pipeline anyway, so the static form is dead either way.

* `jsdom-svg-polyfill.ts`:
  * The top-level docblock was talking about SVG geometry stubs, but the
    very next code in the file was the Node ESM `.css` loader hook —
    confusing for anyone landing in the file. Replaced the top docblock
    with a one-paragraph file overview and moved the SVG paragraph to
    where the SVG stubs actually are.
  * Merged the two `Document.prototype` lookups (`docProtoForCss` for
    `adoptedStyleSheets`, `docProto` for `queryCommandSupported`) into a
    single `docProto` block. Same constant, same prototype, two patches.
  * Tightened the `requestIdleCallback` block comment to a single
    leading-line comment to match the style of the SVG / loader-hook
    sections.

No behaviour change. yarn build:ci EXIT 0; check_binary_deps.py OK; yarn
test stays at 63 / 269.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Comment on lines 21 to 25
import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy";
import { SafeStyle } from "@angular/platform-browser";
import "@codingame/monaco-vscode-python-default-extension";
import "@codingame/monaco-vscode-r-default-extension";
import "@codingame/monaco-vscode-java-default-extension";
import { isDefined } from "../../../common/util/predicate";
import * as monaco from "monaco-editor";
import { MonacoBreakpoint } from "monaco-breakpoints";
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already done — see e8e67b5. Both import "@codingame/monaco-vscode-{python,java}-default-extension" lines were dropped from code-debugger.component.ts, and the centralized ensureVscodeApiStarted() in code-editor.component.ts (where the editor stack is actually instantiated) is the only place loading them now. The static side-effect imports on the debugger were leftover duplicates and are gone.

Comment thread frontend/src/jsdom-svg-polyfill.ts Outdated
Comment on lines 63 to 64
// run under Vitest browser mode rather than jsdom (tracked in #4861).
function fakeMatrix() {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — fixed in 33eea83.

Vitest re-evaluates setupFiles once per spec file, so the unconditional module.register(...) was chaining a fresh .css ESM hook for every spec. Guarded the registration behind a Symbol.for("texera.cssLoaderHookRegistered") flag on globalThis so the hook is installed exactly once per process; subsequent setup invocations short-circuit before calling register.

Another sweep for things that didn't quite pull their weight:

* `code-editor.component.ts`:
  * Drop the `apiWrapperStarted` static boolean; `apiWrapperStartPromise`
    is now the single source of truth — once it resolves, every later
    `await` returns immediately, and the failure branch already clears
    it back to `undefined` so a retry runs the full start sequence
    again. The boolean was just a sync fast-path masking the same
    semantic.
  * Simplify `getFileSuffixByLanguage`: the `javascript` case was
    unreachable (the constructor only ever calls
    `setLanguage("python" | "java")`, and R UDF support was retired
    earlier in this branch). Collapse to a one-line ternary with a
    `.py` default.

* `editor.worker.ts`: trim the canonical-rationale comment from 7 lines
  to 3. The full reasoning is already documented in the
  `monacoWorkerFactory` block in code-editor.component.ts; the worker
  file only needs a pointer.

* `jsdom-svg-polyfill.ts`: convert the remaining `/** ... */` block
  comments (CSSStyleSheet, CSS namespace, matchMedia, WebSocket,
  ngZorro icon suppression) to leading `//` comments to match the style
  of the SVG / loader-hook / requestIdleCallback sections that already
  use them. The mixed style was visually noisy in a long polyfill file.
  Same content, just one comment shape throughout.

No behaviour change.
yarn build:ci EXIT 0; check_binary_deps.py OK: 113; yarn test 63 / 269.
Aggressive line-count pass on the two heaviest PR-touched files.

* `jsdom-svg-polyfill.ts`:
  * Add a single `G = globalThis as Record<string, any>` accessor and
    use it everywhere instead of repeating
    `(globalThis as unknown as { Foo?: { prototype: ... } }).Foo`
    five times. Each cast site shrinks from ~3 lines to one identifier.
  * Add `installIfMissing(proto, fns)` helper that takes a prototype +
    map of `name -> impl` and only patches the missing slots. Replaces
    six hand-rolled `if (typeof proto.X !== "function") proto.X = ...`
    branches across the SVG / CSSStyleSheet / Document / CSS sections.
  * Compact `fakeMatrix` / `fakeTransform` to drive their no-op stubs
    from a `for-of` over the method-name list rather than a fan-out of
    11 + 6 individual assignments.
  * Use `??=` for the `CSS` namespace + `requestIdleCallback` /
    `cancelIdleCallback` initialisation; collapse the matchMediaStub
    object literal onto fewer lines.

* `code-editor.component.ts`: trim the 14-line `monacoWorkerFactory`
  comment to 7 lines — same content, less prose. The component file is
  the canonical home of the trampoline rationale; the worker files
  point back at it.

Net for this commit: 2 files, +87 / -134 (-47). Cumulative for the
PR's two cleanup commits: +163 / -278 (-115). No behaviour change.
yarn build:ci EXIT 0 with byte-identical worker chunks; check_binary_deps.py
OK: 113; yarn test stays at 63 / 269.
…dies

* `code-editor.component.ts`:
  * Inline the `setLanguage()` method into the constructor — it had
    exactly one call site there. Saves the wrapper.
  * Inline `getFileSuffixByLanguage()` at its two call sites as a
    one-line ternary. With only `python` / `java` reachable post-R-UDF,
    the helper was longer than the inline form.

* `jsdom-svg-polyfill.ts`:
  * Pull the `fakeMatrix` / `fakeTransform` method-name lists into
    space-separated string constants (`MATRIX_METHODS` /
    `TRANSFORM_METHODS`) and `.split(" ")` in the loop. Prettier was
    expanding the array literals to one item per line; the string form
    fits on one line each.
  * Convert the no-op method bodies in `InertCSSStyleSheet` and
    `InertWebSocket` from block-form (`replace(): Promise<void> {
    return Promise.resolve(); }`) to arrow-function fields (`replace =
    () => Promise.resolve()`). Identical semantics for these stubs;
    prettier keeps the arrow form on one line.

Net: 2 files, +24 / -50 (-26).
yarn build:ci EXIT 0; check_binary_deps.py OK: 113; yarn test 63 / 269.
… construction

Two new Copilot review comments on apache#4997, plus a small drive-by:

* `jsdom-svg-polyfill.ts`: vitest re-evaluates `setupFiles` once per spec
  file, so the unconditional `module.register(...)` was chaining a fresh
  `.css` ESM hook for every spec — slower (and noisier) module resolution
  across the run. Guard the registration with a `Symbol.for(...)`-keyed
  flag on `globalThis` so the hook installs exactly once per process.

* `code-editor.component.ts` (Copilot — code-debugger static imports):
  already addressed in e8e67b5; reply on the thread.

* `code-editor.component.ts` drive-by: `acceptCurrentAnnotation` was
  building a `monaco.Selection` from `currentRange` only to pass it to
  `insertTypeAnnotations`, which in turn only read `endLineNumber` /
  `endColumn` — fields `monaco.Range` already exposes. Dropped the
  `Selection` construction; `insertTypeAnnotations` now takes the
  `Range` directly. Switched the offset fallback to `??` so a real `0`
  offset isn't masked.

yarn build:ci EXIT 0; check_binary_deps.py OK: 113; yarn test 63 / 269.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade frontend to monaco-languageclient v10

3 participants