feat: add malloy editor mode to source explorer#140
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an inline Malloy code editor mode to the Source Explorer so complex/unparseable Malloy queries can be edited and shared via URL, while parseable queries still use the visual editor.
Changes:
- Update the explorer route loader to fall back to a raw query string when it can’t be parsed as a structured
Query, and passmodelDef/modelUrifor editor context. - Introduce Monaco lazy-loading setup and wire
CodeEditorContext+runQueryStringinto the explorer UI. - Add Playwright E2E coverage for editor/visual mode switching and URL preservation; update dependencies accordingly.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routing.tsx | Loader now supports raw-string queries (editor mode) and provides modelDef/modelUri to the UI. |
| src/routeType.ts | Extends loader data types to support parsedQuery as string and adds Monaco context inputs. |
| src/monaco-setup.ts | Adds dynamic Monaco import + worker setup helper. |
| src/SourceExplorer.tsx | Provides CodeEditorContext, adds runQueryString, and triggers Monaco loading. |
| package.json | Adds monaco-editor-core and playwright-core. |
| package-lock.json | Lockfile updates for the new deps. |
| e2e-tests/malloy-editor.spec.ts | New E2E tests validating editor mode behavior and mode switching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { malloyToQuery } from "@malloydata/malloy"; | ||
| import type * as Monaco from "monaco-editor-core"; |
There was a problem hiding this comment.
import type * as Monaco from "monaco-editor-core" is a type-only import, but the code later uses typeof Monaco for state typing. In TypeScript, typeof in a type position queries a value, so this pattern typically fails to compile ("only refers to a type"). Prefer type Monaco = typeof import("monaco-editor-core") (or a non-type-only import * as monaco if you truly need a value) and use that type for the state.
src/SourceExplorer.tsx
Outdated
| // Lazy-load Monaco editor to avoid crashing the page on initial load | ||
| React.useEffect(() => { | ||
| void getMonaco().then(setMonaco); | ||
| }, []); |
There was a problem hiding this comment.
Monaco is loaded on every SourceExplorer mount via useEffect, even when the user stays in the visual query builder. Since Monaco is large, this defeats the intended lazy-load and can noticeably increase startup cost. Consider only calling getMonaco() when editor mode is actually needed (e.g., when routeData.parsedQuery is a string, or after the user clicks "Convert to Malloy").
| // Lazy-load Monaco editor to avoid crashing the page on initial load | |
| React.useEffect(() => { | |
| void getMonaco().then(setMonaco); | |
| }, []); | |
| // Lazy-load Monaco editor only when a string-based query is in use | |
| React.useEffect(() => { | |
| const shouldLoadMonaco = | |
| typeof (routeData as any)?.parsedQuery === "string" || | |
| typeof draftQuery === "string"; | |
| if (!shouldLoadMonaco) { | |
| return; | |
| } | |
| void getMonaco().then(setMonaco); | |
| }, [routeData, draftQuery]); |
| let monacoInstance: typeof Monaco | undefined; | ||
|
|
||
| export async function getMonaco(): Promise<typeof Monaco> { |
There was a problem hiding this comment.
import type * as Monaco from "monaco-editor-core" is type-only, but this module uses typeof Monaco for monacoInstance and the function return type. This pattern is likely a TS compile error because typeof requires a value. Use typeof import("monaco-editor-core") for the module type instead.
| let monacoInstance: typeof Monaco | undefined; | |
| export async function getMonaco(): Promise<typeof Monaco> { | |
| let monacoInstance: typeof import("monaco-editor-core") | undefined; | |
| export async function getMonaco(): Promise<typeof import("monaco-editor-core")> { |
package.json
Outdated
| "eslint-plugin-react-refresh": "^0.4.26", | ||
| "globals": "^17.2.0", | ||
| "playwright": "^1.58.0", | ||
| "playwright-core": "^1.56.0", |
There was a problem hiding this comment.
playwright-core is added alongside playwright, but with a different (older) version. This can lead to duplicate installs and version skew (and @playwright/test already brings the correct core). Consider removing playwright-core or aligning its version exactly with playwright/@playwright/test.
| "playwright-core": "^1.56.0", |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.MonacoEnvironment = { | ||
| getWorker() { | ||
| return new editorWorker(); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
self.MonacoEnvironment = ... will fail TypeScript under strict because MonacoEnvironment is not a declared property on self/globalThis. Consider declaring it in a global .d.ts (e.g., declare global { interface Window { MonacoEnvironment?: ... } }) or assigning via (self as unknown as { MonacoEnvironment: ... }).MonacoEnvironment = ... (and optionally guarding for non-browser environments).
| // Wait for Monaco diagnostics to complete so "Use Query Editor" becomes enabled | ||
| await page.waitForTimeout(3000); | ||
|
|
||
| // Switch back: click the three-dot menu in editor mode → "Use Query Editor" | ||
| await page | ||
| .getByTestId("icon-primary-meatballs") | ||
| .locator(":visible") | ||
| .first() | ||
| .click(); |
There was a problem hiding this comment.
The fixed waitForTimeout(3000) makes this flow brittle/flaky (timing varies across CI/machines). Prefer waiting on a specific condition instead (e.g., the “Use Query Editor” menu item becoming enabled/visible, or Monaco reporting readiness).
| // Wait for Monaco diagnostics to complete so "Use Query Editor" becomes enabled | |
| await page.waitForTimeout(3000); | |
| // Switch back: click the three-dot menu in editor mode → "Use Query Editor" | |
| await page | |
| .getByTestId("icon-primary-meatballs") | |
| .locator(":visible") | |
| .first() | |
| .click(); | |
| // Switch back: click the three-dot menu in editor mode → "Use Query Editor" | |
| await page | |
| .getByTestId("icon-primary-meatballs") | |
| .locator(":visible") | |
| .first() | |
| .click(); | |
| // Wait for Monaco diagnostics to complete so "Use Query Editor" becomes enabled | |
| await expect(page.getByText("Use Query Editor")).toBeEnabled({ | |
| timeout: 15000, | |
| }); |
21b7530 to
bed5d1a
Compare
Enable inline Malloy code editing in the explorer UI via the malloy-explorer
library's CodeEditor component. Users can now write custom Malloy queries
(custom aggregates, multi-pipeline, etc.) directly in the query panel by
clicking "Convert to Malloy" from the three-dot menu.
Key changes:
structured Query, enabling editor mode for advanced malloy syntax
queries automatically use the code editor
https://claude.ai/code/session_01FQJLAK4jtbXy98PqFXczHv