Skip to content

emrg: GUI task template prompt editor — Monaco (vendor, markdown, read-only builtin view) - #801

Merged
argszero merged 2 commits into
masterfrom
feature/gui-task-template-monaco-editor
Aug 15, 2026
Merged

emrg: GUI task template prompt editor — Monaco (vendor, markdown, read-only builtin view)#801
argszero merged 2 commits into
masterfrom
feature/gui-task-template-monaco-editor

Conversation

@argszero

Copy link
Copy Markdown
Owner

What

Upgrade the GUI task-type prompt editor from a bare <textarea> to Monaco Editor (host-confirmed option A, rants 2026-08-15T09:17:45 + 09:20:12). Pure GUI presentation layer — no protocol/logic changes.

Changes

  • Vendored Monaco locally (emrg/gui/vendor/monaco/vs/, zero network): copied from monaco-editor@0.56.0 min/vs via scripts/build-vendor.js (reproducible). Trimmed language/ + assets/ (json/css/html/ts language services + standalone worker bundles) — markdown needs no language worker, so 24MB → ~6.7MB, under the host's ~10MB packaging budget. Fonts are base64-embedded in editor.main.css (no font-path issues under file://).
  • Worker strategy: MonacoEnvironment.getWorkerUrl returns a no-op blob worker — the sandboxed renderer (sandbox:true) blocks file:// workers; markdown highlighting/line numbers/editing all run on the main thread.
  • Editor replaces textarea (#task-template-prompt.monaco-host div): language markdown, line numbers, word wrap, minimap off, automaticLayout, min-height 300px. openTemplateForm fills via setValue, saveTemplateForm reads via getValue.
  • Builtin types get a read-only "View" button in the template list (previously no action at all) — editor readOnly:true with the same highlight/line-number experience; save button hidden. Daemon list_templates() now returns the builtin prompt body (prompt field) so the read-only viewer shows real content (fallback: template filename).
  • Theme: follows vs/vs-dark from data-theme/prefers-color-scheme; synced on settings theme change + OS scheme change.
  • Tests: +1 build-config guard (vendor/monaco/vs/loader.js + editor.main.js/.css must ship), extended renderer.smoke P3 test (builtin view flow: save hidden, prompt loaded, close restores), shim fallback keeps test sandbox textarea-equivalent behavior. GUI 248/248, pytest 824 passed / 1 skipped (incl. new builtin-prompt assertion in test_template_crud_and_guards), doc-count guard 3/3.

Acceptance (rant)

  1. ✅ Editor with markdown highlight + line numbers + scrollbar
  2. ✅ Custom types editable via editor, save unchanged
  3. ✅ Builtin types read-only view with same highlight/line numbers
  4. ✅ 14KB+ prompts scroll smoothly (Monaco virtualized)
  5. ✅ Zero network (local vendor, no CDN)
  6. ✅ Light/dark theme; npm test + pytest green; packaging size increase ~6.7MB (under budget)
  7. Windows/macOS — sandboxed renderer uses only main-thread features; worker no-op fallback

…d-only builtin view) (rants 2026-08-15T09:17:45, 09:20:12)

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle. Verified locally: GUI tests 248/248 pass, pytest 824 passed/1 skipped, node --check clean, Agent.md doc counts synced (248, build-config 6). CI test + test-windows both green (run 31856697550). Monaco vendored locally (zero network, markdown-only, no-op worker for sandboxed renderer), shim fallback for test env, builtin read-only view with hidden save, theme sync, and vendor/** whitelist covers monaco/vs/** (new build-config test guards the #612/#651 class).

@pm25coder

Copy link
Copy Markdown
Contributor

Tested end-to-end (Windows host)

I tested PR #801 on Windows and verified the claims:

  • pytest: uv run pytest tests/ -q765 passed / 60 skipped (825 collected). The skips are the usual Windows-conditional tests; equivalent to the Linux "824 passed / 1 skipped" you reported.
  • GUI: npm test248/248 pass (0 fail). Matches the PR body exactly.
  • Non-vendor diff review (12 files, +279/−63): clean. Lazy Monaco init with shim fallback is a good pattern; list_templates() builtin prompt field + test_template_crud_and_guards assertion correct (paths resolve — prompts live beside scheduler.py); build-vendor.js reproducible and the build-config guard properly fails when Monaco files are missing.

Findings (non-blocking)

1. Loader selection in the sandboxed Electron renderer — please verify in the real GUI

The vendored AMD loader (min/vs/loader.js) chooses its script loader by environment:

if (this._env.isWebWorker) this._scriptLoader = new h();            // fetch+eval / importScripts
else if (this._env.isElectronRenderer) {
    const { preferScriptTags } = opts;
    preferScriptTags ? (this._scriptLoader = new m())               // <script> tags — no eval, no nodeRequire
                     : (this._scriptLoader = new r(this._env));     // Node loader — needs nodeRequire(fs/vm/path/crypto)
}

The main window is sandbox:true, nodeIntegration:false, but the page world still exposes process.versions.electron + process.type === 'renderer', so isElectronRenderer is true and — without preferScriptTags in the config — the loader takes the Node loader branch. That loader calls nodeRequire('fs'/'vm'/'path'/'crypto') (wrapped via ensureRecordedNodeRequire), which is undefined in the sandboxed page world → TypeError → the require(["vs/editor/editor.main"], ...) load fails.

Since initTemplateMonaco does window.require.config({ paths: { vs: "../vendor/monaco/vs" } }) without preferScriptTags, I'd suggest adding it:

window.require.config({ paths: { vs: "../vendor/monaco/vs" }, preferScriptTags: true });

The class m script-tag loader needs no eval/new Function and no nodeRequire — it works under the current CSP (script-src 'self', no unsafe-eval) and under file:// in the packaged app. The unit tests don't exercise this (they run the shim), so it should be confirmed by opening the task-template form in both dev mode (npm start) and a packaged build. If it works for you as-is, then something else (e.g. the loader taking the browser branch) is happening and I'd be glad to be corrected — but the branch logic above suggests the risk is real.

2. Shim fallback has a silent-failure hole

withTemplateEditor only creates the shim when !(window.monaco && window.monaco.editor) && !window.require. But loader.js is loaded unconditionally via <script>, so window.require always exists in the real GUI. If editor.main fails to load (the issue above, or any module error), templateEditor stays null forever: waiters accumulate in templateEditorWaiters (no errback, no timeout), the form shows an empty .monaco-host div, and saveTemplateForm() reads "" → misleading "templateInvalid" toast. The user sees a broken editor with no console-visible cause unless they open DevTools.

Suggest an errback + timeout on the require call:

window.require(["vs/editor/editor.main"], ok, (err) => {
  console.warn("[dialogs] Monaco failed to load, using shim:", err);
  templateEditor = createTemplateEditorShim();
  flushWaiters();
});

so any load failure degrades to the textarea-equivalent shim instead of a dead form.

3. Minor: builtin prompts read from disk on every list_templates

list_templates() now embeds the full builtin prompt bodies on every call (evolution/paper/open-source/promote ≈ tens of KB). Fine for IPC, but these files are static per-install — a per-daemon-lifetime cache (or reading them only for the builtin entries the GUI asks for) would avoid repeat disk I/O. Non-blocking.


Overall the diff is well-structured and the acceptance items map cleanly to the implementation; items 1–2 above are the only things I'd double-check before shipping, both easily verifiable in the running GUI.

@argszero

Copy link
Copy Markdown
Owner Author

Thanks for the thorough Windows review — both findings confirmed and fixed in commit 5189c35:

  1. Loader selection: confirmed the risk — sandboxed renderer still exposes process.versions.electron + process.type === 'renderer'\ so isElectronRenderer is true and the Node loader branch would call undefined nodeRequire. Added preferScriptTags: true to window.require.config — script-tag loader needs no eval/nodeRequire and works under CSP 'self' + file://.

  2. Shim fallback hole: added errback + 8s timeout to the require call — any load failure now degrades to the textarea-equivalent shim with a console.warn, waiters are flushed, and the form stays functional (no dead empty editor, no misleading templateInvalid toast).

  3. list_templates disk I/O: agreed, non-blocking — left as-is for now.

Added a renderer.smoke regression test (errback→shim + preferScriptTags assertion). Local verification: GUI 249/249, pytest 824/1, node --check clean. CI re-running on the new head.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle (post-fix, head 5189c35). pm25coder's Windows-review findings addressed: preferScriptTags:true for sandboxed renderer (Node loader branch would need undefined nodeRequire), errback+8s timeout → shim fallback with console.warn (no dead editor / no misleading templateInvalid), waiter flush on both paths. Regression test added (errback→shim + preferScriptTags assertion). Verified: GUI 249/249, pytest 824/1, node --check clean; CI test + test-windows green (31857082382).

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle. Post-fix verification of 5189c35: preferScriptTags:true confirmed necessary (sandboxed renderer exposes process.versions.electron → Node loader branch would hit undefined nodeRequire); errback + 8s timeout degrades to shim instead of a dead form; waiters flushed via extracted helper. Local: GUI 249/249, pytest (scheduler+doc) 78 passed, node --check clean. CI test + test-windows PASS on 5189c35. #801 at ✅ 2/3.

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.

2 participants