-
Notifications
You must be signed in to change notification settings - Fork 82
feat(webui): Integrate Monaco editor for future Presto SQL query input. #1108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 13 commits
9ca2d54
b501260
e27d62e
99095de
bdfe59d
bf9fae1
f4757af
0d3eba3
57de1a1
89fe75e
e3dc4a4
b6eb73a
6d8398a
126782e
4660e42
0b745e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import {useEffect} from "react"; | ||
|
||
import { | ||
Editor, | ||
EditorProps, | ||
useMonaco, | ||
} from "@monaco-editor/react"; | ||
import {language as sqlLanguage} from "monaco-editor/esm/vs/basic-languages/sql/sql.js"; | ||
|
||
import "./monaco-config"; | ||
|
||
|
||
type SqlEditorProps = Omit<EditorProps, "language">; | ||
|
||
/** | ||
* Monaco editor with highlighting and autocomplete for SQL syntax. | ||
* | ||
* @param props | ||
* @return | ||
*/ | ||
const SqlEditor = (props: SqlEditorProps) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a fix for this with css. so now fixed |
||
const monaco = useMonaco(); | ||
|
||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
useEffect(() => { | ||
if (null === monaco) { | ||
return () => { | ||
}; | ||
} | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
// Adds autocomplete suggestions for SQL keywords on editor load | ||
const provider = monaco.languages.registerCompletionItemProvider("sql", { | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
provideCompletionItems: (model, position) => { | ||
const word = model.getWordUntilPosition(position); | ||
const range = { | ||
startLineNumber: position.lineNumber, | ||
endLineNumber: position.lineNumber, | ||
startColumn: word.startColumn, | ||
endColumn: word.endColumn, | ||
}; | ||
const suggestions = sqlLanguage.keywords.map((keyword: string) => ({ | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
detail: "SQL Keyword", | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
insertText: `${keyword} `, | ||
kind: monaco.languages.CompletionItemKind.Keyword, | ||
label: keyword, | ||
range: range, | ||
})); | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return {suggestions: suggestions}; | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
}, | ||
}); | ||
|
||
return () => { | ||
provider.dispose(); | ||
}; | ||
}, [monaco]); | ||
|
||
return ( | ||
<Editor | ||
language={"sql"} | ||
|
||
// Use white background while loading (default is grey) so transition to editor with | ||
// white background is less jarring. | ||
loading={<div style={{backgroundColor: "white", height: "100%", width: "100%"}}/>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (just an idea - not strictly needed in this PR) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i tried it already! it looks worse since the grey lines in the skeleton appear and disapear. the plain white box just looks better |
||
options={{ | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fontSize: 14, | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
lineNumbers: "on", | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
lineNumbersMinChars: 2, | ||
minimap: {enabled: false}, | ||
overviewRulerBorder: false, | ||
placeholder: "Enter your SQL query", | ||
scrollBeyondLastLine: false, | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
wordWrap: "on", | ||
}} | ||
{...props}/> | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
}; | ||
|
||
export default SqlEditor; |
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,21 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
/* eslint-disable import/default */ | ||||||||||||||||||||||||||||||||||||||||||||||||
import {loader} from "@monaco-editor/react"; | ||||||||||||||||||||||||||||||||||||||||||||||||
import * as monaco from "monaco-editor/esm/vs/editor/editor.api"; | ||||||||||||||||||||||||||||||||||||||||||||||||
import EditorWorker from "monaco-editor/esm/vs/editor/editor.worker?worker"; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||
import "monaco-editor/esm/vs/editor/editor.all.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
/* eslint-disable import/default */ | |
import {loader} from "@monaco-editor/react"; | |
import * as monaco from "monaco-editor/esm/vs/editor/editor.api"; | |
import EditorWorker from "monaco-editor/esm/vs/editor/editor.worker?worker"; | |
import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js"; | |
import "monaco-editor/esm/vs/editor/editor.all.js"; | |
/* eslint-disable import/default, @stylistic/max-len */ | |
import {loader} from "@monaco-editor/react"; | |
import * as monaco from "monaco-editor/esm/vs/editor/editor.api"; | |
import EditorWorker from "monaco-editor/esm/vs/editor/editor.worker?worker"; | |
import "monaco-editor/esm/vs/basic-languages/sql/sql.contribution.js"; | |
import "monaco-editor/esm/vs/editor/contrib/clipboard/browser/clipboard.js"; | |
import "monaco-editor/esm/vs/editor/contrib/contextmenu/browser/contextmenu.js"; | |
import "monaco-editor/esm/vs/editor/contrib/find/browser/findController.js"; | |
import "monaco-editor/esm/vs/editor/contrib/wordHighlighter/browser/wordHighlighter.js"; | |
import "monaco-editor/esm/vs/editor/contrib/suggest/browser/suggestController.js"; | |
import "monaco-editor/esm/vs/editor/contrib/placeholderText/browser/placeholderText.contribution.js"; | |
/* eslint-enable @stylistic/max-len */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've found ways to lazy load the monaco assets but the code is not very organized at this point. i'll investigate further in the future
@coderabbitai create an issue to support lazy loading of the monaco assets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
I'll create an issue to track the lazy loading optimization for Monaco assets.
@junhaoliao I've created an issue to track the lazy loading optimization for Monaco Editor assets. This will help organize the future work to improve page load performance while maintaining the current SQL functionality.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for lazy loading Monaco assets
gh issue create \
--title "Optimize Monaco Editor with lazy loading of assets" \
--body "## Description
Currently, the Monaco Editor implementation loads all assets upfront, which could be optimized with lazy loading to improve initial page load performance.
## Context
The Monaco Editor was recently added in PR #1108 to support SQL syntax highlighting and autocomplete. While investigating optimization opportunities, lazy loading techniques were identified but the implementation needs to be better organized.
## Goals
- Implement lazy loading for Monaco Editor assets
- Improve initial page load performance
- Maintain current SQL autocomplete and syntax highlighting functionality
- Ensure code organization and maintainability
## References
- Original PR: https://github.com/y-scope/clp/pull/1108
- Discussion: https://github.com/y-scope/clp/pull/1108#discussion_r2220658679
## Related Files
- \`components/webui/client/src/components/SqlEditor/monaco-config.ts\`
- \`components/webui/client/vite.config.ts\`" \
--assignee junhaoliao
Length of output: 1084
@junhaoliao Perfect! I've successfully created issue #1120 to track the lazy loading optimization for Monaco Editor assets. The issue includes the context from this PR, your investigation findings, and references to the relevant files and discussion.
You can view the issue here: #1120
The issue is assigned to you and captures the goals of implementing lazy loading while maintaining the current SQL functionality and ensuring good code organization.
🧠 Learnings used
Learnt from: davemarco
PR: y-scope/clp#1108
File: components/webui/client/src/components/SqlEditor/monaco-config.ts:6-8
Timestamp: 2025-07-21T14:37:57.611Z
Learning: In Monaco Editor configurations for SQL editing, the import of "monaco-editor/esm/vs/editor/editor.all.js" is required for SQL autocomplete functionality to work properly, even when using "editor.api" and "sql.contribution.js". Removing "editor.all.js" breaks the autocomplete feature.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Redundant heavy imports inflate bundle size.
editor.api
already registers the editor core; pulling in editor.all.js
adds the full feature set again, doubling payload. Unless you explicitly need the legacy “all” bundle, drop it and rely on targeted feature contributions (sql.contribution.js
).
🤖 Prompt for AI Agents
In components/webui/client/src/components/SqlEditor/monaco-config.ts around
lines 6 to 8, the import of "monaco-editor/esm/vs/editor/editor.all.js" is
redundant and inflates the bundle size because "editor.api" already registers
the editor core. Remove the import of "editor.all.js" and rely solely on
targeted feature contributions like "sql.contribution.js" to reduce payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the autocomplete dosent work without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davemarco Thank you for the clarification! You're absolutely right - if the autocomplete functionality breaks without the editor.all.js
import, then it's definitely necessary to keep it. The bundle size trade-off is worth it for the functionality, especially since SQL autocomplete is a key feature mentioned in your PR objectives.
I appreciate you testing this and providing the practical feedback. The Monaco Editor's dependency requirements can be tricky, and your real-world experience is invaluable.
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/clp#1108
File: components/webui/client/src/components/SqlEditor/monaco-config.ts:6-8
Timestamp: 2025-07-21T14:37:57.611Z
Learning: In Monaco Editor configurations for SQL editing, the import of "monaco-editor/esm/vs/editor/editor.all.js" is required for SQL autocomplete functionality to work properly, even when using "editor.api" and "sql.contribution.js". Removing "editor.all.js" breaks the autocomplete feature.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
declare module "monaco-editor/esm/vs/basic-languages/sql/sql.js" { | ||
import {languages} from "monaco-editor/esm/vs/editor/editor.api"; | ||
|
||
|
||
interface SqlLanguageDefinition extends languages.IMonarchLanguage { | ||
keywords: string[]; | ||
} | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export const language: SqlLanguageDefinition; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import {CaretRightOutlined} from "@ant-design/icons"; | ||
import { | ||
Button, | ||
Tooltip, | ||
} from "antd"; | ||
|
||
import useSearchStore from "../../../SearchState/index"; | ||
|
||
|
||
/** | ||
* Renders a button to run the SQL query. | ||
* | ||
* @return | ||
*/ | ||
const RunButton = () => { | ||
const queryString = useSearchStore((state) => state.queryString); | ||
|
||
const isQueryStringEmpty = "" === queryString; | ||
const tooltipTitle = isQueryStringEmpty ? | ||
"Enter SQL query to run" : | ||
""; | ||
|
||
return ( | ||
<Tooltip title={tooltipTitle}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a consistent look, we should disable this button when the querystring is empty, just like we did with the submit button. |
||
<Button | ||
color={"green"} | ||
disabled={isQueryStringEmpty} | ||
icon={<CaretRightOutlined/>} | ||
size={"large"} | ||
variant={"solid"} | ||
> | ||
Run | ||
</Button> | ||
</Tooltip> | ||
); | ||
}; | ||
|
||
export default RunButton; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
/* Allows the editor to shrink when page width decreases */ | ||
.input { | ||
width: 100%; | ||
min-width: 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import {useCallback} from "react"; | ||
|
||
import SqlEditor from "../../../../../components/SqlEditor"; | ||
import useSearchStore from "../../../SearchState/index"; | ||
import styles from "./index.module.css"; | ||
|
||
|
||
/** | ||
* Renders SQL query input. | ||
* | ||
* @return | ||
*/ | ||
const SqlQueryInput = () => { | ||
const handleChange = useCallback((value: string | undefined) => { | ||
const {updateQueryString} = useSearchStore.getState(); | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
updateQueryString(value || ""); | ||
}, []); | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<div className={styles["input"] || ""}> | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<SqlEditor | ||
height={"150px"} | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
onChange={handleChange}/> | ||
</div> | ||
); | ||
}; | ||
|
||
export default SqlQueryInput; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
import { | ||
CLP_QUERY_ENGINES, | ||
CLP_STORAGE_ENGINES, | ||
SETTINGS_QUERY_ENGINE, | ||
SETTINGS_STORAGE_ENGINE, | ||
} from "../../../config"; | ||
import Dataset from "./Dataset"; | ||
import styles from "./index.module.css"; | ||
import RunButton from "./Presto/RunButton"; | ||
import SqlQueryInput from "./Presto/SqlQueryInput"; | ||
import QueryInput from "./QueryInput"; | ||
import SearchButton from "./SearchButton"; | ||
import TimeRangeInput from "./TimeRangeInput"; | ||
|
@@ -27,10 +31,21 @@ const SearchControls = () => { | |
return ( | ||
<form onSubmit={handleSubmit}> | ||
<div className={styles["searchControlsContainer"]}> | ||
{CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} | ||
<QueryInput/> | ||
<TimeRangeInput/> | ||
<SearchButton/> | ||
{SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.NATIVE ? | ||
( | ||
<> | ||
{CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} | ||
<QueryInput/> | ||
<TimeRangeInput/> | ||
<SearchButton/> | ||
</> | ||
) : | ||
( | ||
<> | ||
<SqlQueryInput/> | ||
<RunButton/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it at first, but it will complicate searchQueryStatus. Like right now searchQueryStatus is a separate component under SearchControls. I think it looks weird to have searchQueryStatus under the button. I though about putting searchQueryStatus next to the button, but it will just be mess to render searchQueryStatus in different locations for "presto" and "native". I thought just putting the button on the right makes "native" and "presto" have similiar layout simplifying the codebase |
||
</> | ||
)} | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</div> | ||
</form> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the bundle size after adding Monaco?
One reason we avoided using this
@monaco-editor/react
was that the bundle size increases significantly as all Monaco assets were packed into the bundle. Is it still the case today?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is seems reasonable here? https://bundlephobia.com/package/@monaco-editor/react@4.7.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i ran
npm run build
and this is the output:previously, it was
so we can see the package size increase is not significant possibly because only the
@monaco-editor/react
are included. however, i wonder where themonaco-editor
assets are addedUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default, the loader-config loads Monaco assets from a CDN (on my end it uses https://cdn.jsdelivr.net/npm/monaco-editor): https://github.com/suren-atoyan/monaco-react/blob/master/README.md#loader-config
This would mean that if a user accesses the webui without access to the Internet or behind a country / corporate managed firewall, they would not be able to use the search page.
can we follow https://github.com/suren-atoyan/monaco-react/blob/master/README.md#use-monaco-editor-as-an-npm-package to config the monaco assets imports? (there's a Vite specific section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on my end, if i just use their Vite example loader code, the bundle size bumped from 2.5MB to 15MB.
i think the issue is with
if we use https://github.com/y-scope/yscope-log-viewer/blob/main/src/components/Editor/MonacoInstance/bootstrap.ts as a reference and import only what we need, the bundle size increase should be less than 972.2KB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually never mind. we do need the whole bundle for the autocomplete to work. If you only want the editor, then we can't have autocomplete i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wont work since it will break the autocomplete, unless you find another thing to import that can fix. I was able to fix syntax highlighting but not autocomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this may fix, but not sure why import "monaco-editor/esm/vs/editor/editor.all.js"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now understand this 972.2KB is counting sizes of assets only in the "min" bundle and does not include any ESM APIs. Since we use the ESM APIs, we do need to import differently (more).
Let's see the comment at "monaco-config.ts" to reduce the scope of import. In the future, we should support lazy loading those.