mirror of
https://github.com/coder/coder.git
synced 2026-10-31 02:43:21 +08:00
Jake Howell
5008603d08
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Fixes [DEVEX-736](https://linear.app/codercom/issue/DEVEX-736/template-editor-crashes-becomes-unusable-when-opening-versions). ## Problem Opening template versions in the template editor leaks Monaco text models. Each version renders one `DiffEditor` per changed file, and `SyntaxHighlighter` sets `keepCurrentOriginalModel` / `keepCurrentModifiedModel`, which tells `@monaco-editor/react` **not** to dispose the underlying text models when the editor unmounts. Because no stable model paths are supplied, every visit creates fresh anonymous models that are never freed. Navigating between versions accumulates models without bound until the tab lags, spins, and eventually OOMs — matching the customer reports on 2.34.5. The flags themselves are still required: removing them makes the library dispose the models mid-teardown (before the `DiffEditorWidget` is torn down), which throws. ## Fix Keep the `keepCurrent*Model` flags, but move the diff editor into its own `DiffFile` component that owns the model cleanup: - `DiffFile` captures the `original`/`modified` models in `onMount` and disposes them in its own unmount effect. Because the effect lives with the diff editor, it runs whenever the diff editor unmounts — **including when a file switches diff → plain across versions while `SyntaxHighlighter` stays mounted** (the `key={filename}` case raised in review), not only on full unmount. - Disposal is deferred with `queueMicrotask` so it runs after React's commit and after `@monaco-editor/react` disposes the widget. Freeing the models in the same synchronous teardown throws `TextModel got disposed before DiffEditorWidget model got reset`. Non-diff files use the plain `Editor`, which already disposes its own model, so they are unaffected. No editor options or UI behavior change. ## Evidence Reproduced locally on a template with 32 versions (9 `.tf` files each), driving 192 in-app version navigations (single SPA session, no page reloads) and reading `monaco.editor.getModels().length` via temporary instrumentation. | | before (pass 1 → pass 6) | after (pass 1 → pass 6) | |---|---|---| | retained Monaco models | 532 → **3,142** (unbounded) | ~10–19 (flat) | | per-navigation render latency | 206ms → **611ms** | ~120–270ms (flat) | Before: models climb ~520 per full pass and never release (a sample taken with zero editors on screen still showed 522 live models). After: model count and render latency stay flat across the same workload. ## Testing Added `SyntaxHighlighter.stories.tsx` with a `play` regression test (`DisposesModelsOnDiffToggle`) that reproduces the exact leak: it toggles a single surviving `SyntaxHighlighter` between diff and plain three times and asserts `monaco.editor.getModels().length` drops when the diff is removed and returns to the baseline each cycle (no accumulation). - The test **fails on the unfixed behavior** (disposal removed): `expected 5 to be less than 4` — models grow instead of being freed. - With the fix it passes with **zero Monaco teardown errors**: ``` $ pnpm test:storybook src/components/SyntaxHighlighter/SyntaxHighlighter.stories.tsx Test Files 1 passed (1) Tests 3 passed (3) Errors 0 ``` - `pnpm check` (biome) — pass - `pnpm lint:types` (`tsc --noEmit`) — pass - Manual stress test: model count and render latency stay flat across 192 navigations (see Evidence). <details> <summary>Investigation & decision log</summary> **Root cause trace** - `site/src/pages/TemplateVersionPage/TemplateVersionPageView.tsx` → `TemplateFiles` renders every template file at once. - Each changed file → `SyntaxHighlighter` → a full Monaco `DiffEditor`. - `@monaco-editor/react@4.7.0` unmount cleanup does, in effect: ```js const model = editor.getModel(); keepCurrentOriginalModel || model.original.dispose(); keepCurrentModifiedModel || model.modified.dispose(); editor.dispose(); ``` With both `keepCurrent*` flags set, neither model is disposed, so they leak. - The flags exist to stop a storybook hang caused by that same disposal-order error, so they cannot simply be removed. **Options considered** 1. Remove the flags — fixes the leak but re-introduces the mid-teardown throw. Rejected. 2. Supply stable `originalModelPath`/`modifiedModelPath` so models are reused — the library reuses by URI, but its value-sync effect is skipped on the first render, so a reused model renders **stale content** for the new version. Rejected. 3. Keep the flags and dispose the captured models when the diff editor unmounts — targeted and correct for the diff → plain → diff case. **Chosen.** **Why the cleanup is scoped to a child component** The first iteration kept the effect in `SyntaxHighlighter` with an empty dependency array, so it only ran on full unmount. Review correctly flagged that a surviving instance switching diff → plain would then orphan the previous model pair. Scoping the effect to the `DiffFile` child ties it to the diff editor's own lifecycle, closing that gap. **Why `queueMicrotask`** Disposing synchronously in the effect cleanup races Monaco's own widget teardown and throws `TextModel got disposed before DiffEditorWidget model got reset`. Deferring one microtask lets the widget finish tearing down first. **Test-only editor options** The stories pass `renderGutterMenu: false` / `occurrencesHighlight: "off"` through `editorProps`. Those Monaco features register delayed disposables whose teardown throws under jsdom on unmount; disabling them keeps the test runner clean. They do not affect model count, and production keeps Monaco's default options unchanged. </details>