feat(webui): Move run button under SQL query input; Modify SQL editor theming to look like AntD input. #1248
Conversation
WalkthroughMoves QueryStatus into SearchControls, updates import paths, renames SearchQueryStatus → QueryStatus, adjusts SearchControls layout and styles, removes the previous SearchQueryStatus usage from SearchPage, tweaks Presto button sizing/width, and revises SqlEditor Monaco themes and editor options plus a lint comment change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SP as SearchPage
participant SC as SearchControls
participant QS as QueryStatus
participant SS as SearchState
User->>SP: Open Search page
SP->>SC: Render SearchControls (engine-specific)
SC->>QS: Instantiate QueryStatus
QS->>SS: Subscribe/select status/progress
SS-->>QS: Emit status updates
QS-->>SC: Provide status UI/state
SC-->>SP: Controls + Status rendered
note right of SC: QueryStatus moved inside SearchControls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
davemarco
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx (2)
26-35: Drop useMemo; compute Math.max inline for simplicityuseMemo here is unnecessary; Math.max on two numbers is trivial and won’t impact render cost. Removing it simplifies the component without behaviour change.
Apply this diff:
- const numResults = useMemo( - () => Math.max(numSearchResultsTimeline, numSearchResultsTable), - [ - numSearchResultsTimeline, - numSearchResultsTable, - ] - ); + const numResults = Math.max(numSearchResultsTimeline, numSearchResultsTable);And update the import to remove useMemo:
-import {useMemo} from "react"; +// no React hooks required here
61-62: Improve readability of large countsFormat the number with locale separators so large result counts are easier to scan.
Apply this diff:
- {numResults} + {numResults.toLocaleString()}components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx (1)
22-36: QueryStatus: unify status output and improve accessibilityThe
QueryStatuscomponent currently shows “0 results” before a query or after failure and doesn’t announce updates to screen readers. We need to:
- Gate the entire status when in
DEFAULTorQUERY_ID_PENDINGand there’s nosearchJobId, so we don’t render “0 results” prematurely.- Add an explicit failure message when
searchUiState === SEARCH_UI_STATE.FAILED.- Wrap the status in an ARIA live region (
aria-live="polite" aria-atomic="true") so that changes are announced.- Render one coherent
<Text>for QUERYING/DONE—including the coloured result count—rather than separate elements.Apply the following refactor in
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx:const QueryStatus = () => { const { searchJobId, searchUiState, } = useSearchStore(); + // Only show status once we have a job ID or on failure + const shouldShow = + searchUiState === SEARCH_UI_STATE.QUERYING || + searchUiState === SEARCH_UI_STATE.DONE || + searchUiState === SEARCH_UI_STATE.FAILED; + + if (!shouldShow && !searchJobId) { + return null; + } return ( - <div className={styles["status"]}> - {(searchUiState === SEARCH_UI_STATE.QUERYING || - searchUiState === SEARCH_UI_STATE.DONE) && ( - <Text type={"secondary"}> - Search job # - {searchJobId} - {" "} - found - {" "} - </Text> - )} - <Results/> - <Text type={"secondary"}> results</Text> - </div> + <div + className={styles["status"]} + aria-live="polite" + aria-atomic="true" + > + {(searchUiState === SEARCH_UI_STATE.QUERYING || + searchUiState === SEARCH_UI_STATE.DONE) && ( + <Text type="secondary"> + Search job #{searchJobId} found <Results/> results + </Text> + )} + + {searchUiState === SEARCH_UI_STATE.FAILED && ( + <Text type="danger">Search failed</Text> + )} + </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsxcomponents/webui/client/src/pages/SearchPage/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
components/webui/client/src/config/index.ts (4)
SETTINGS_QUERY_ENGINE(26-26)CLP_QUERY_ENGINES(24-24)CLP_STORAGE_ENGINES(25-25)SETTINGS_STORAGE_ENGINE(27-27)components/webui/common/index.ts (1)
CLP_QUERY_ENGINES(148-148)
🔇 Additional comments (5)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx (1)
8-9: Import path update looks correctThe relative path adjustment to ../../SearchState aligns with the new directory depth under SearchControls/QueryStatus. No issues spotted.
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx (1)
3-4: Import path changes are consistent with the new structureSwitching to ../../SearchState resolves correctly from SearchControls/QueryStatus. Looks good.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (3)
27-27: JSDoc update matches the new responsibility.Comment accurately reflects that SearchControls now includes query status UI.
50-57: PRESTO layout change meets the PR objective.Run button moved beneath the SQL editor with QueryStatus adjacent—matches the stated UX. Nice.
12-12: Component rename fully propagated — no leftover references
I’ve verified that all instances ofSearchQueryStatushave been removed and replaced with the newQueryStatuscomponent:
- No occurrences of
SearchQueryStatusremain.- The only
QueryStatusimports are:
- In
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(expected usage)- In
components/webui/client/src/router.tsx(the generic UI variant, not related to the old SearchQueryStatus)There’s no double‐rendering or dead code related to the rename. Everything looks good for approval.
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Outdated
Show resolved
Hide resolved
| padding-top: 4px; | ||
| margin-bottom: -4px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid negative margins; prefer layout alignment over spacing hacks
Using a negative bottom margin to visually align text with the button can cause overlap with adjacent clickable areas and unpredictable wrapping. Consider aligning via flex and baseline instead of padding/margin tweaks.
Apply this diff to improve robustness:
.status {
- margin-left: 2px;
- padding-top: 4px;
- margin-bottom: -4px;
+ margin-left: 2px;
+ padding-top: 4px;
+ margin-bottom: 0;
+ display: inline-flex;
+ align-items: baseline; /* aligns number with button text baseline */
+ gap: 4px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| padding-top: 4px; | |
| margin-bottom: -4px; | |
| .status { | |
| margin-left: 2px; | |
| padding-top: 4px; | |
| margin-bottom: 0; | |
| display: inline-flex; | |
| align-items: baseline; /* aligns number with button text baseline */ | |
| gap: 4px; | |
| } |
🤖 Prompt for AI Agents
In
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.module.css
around lines 3-4, replace the negative bottom margin and extra padding with a
layout-based solution: remove margin-bottom: -4px; and adjust padding-top as
needed, then ensure the parent/container uses display:flex with
align-items:baseline (or center) and appropriate gap or padding to vertically
align the text with the button; this eliminates the negative margin hack and
prevents overlap while keeping visual alignment.
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
- The ui doesn't look the best, let's discuss and have a debugging session in the standup.
- Can we also change the font size + line number in this PR as we discussed before?
|
@hoophalab , we should merge #1244 before reviewing this PR. I think the UI will look better after #1244 is merged. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx (2)
31-38: Harden count computation and improve number readability.Avoid relying on store defaults being numeric—
undefinedinMath.maxyieldsNaN. Also format large counts for readability.- const numResults = useMemo( - () => Math.max(numSearchResultsMetadata, numSearchResultsTimeline, numSearchResultsTable), - [ - numSearchResultsMetadata, - numSearchResultsTimeline, - numSearchResultsTable, - ] - ); + const numResults = useMemo( + () => + Math.max( + numSearchResultsMetadata ?? 0, + numSearchResultsTimeline ?? 0, + numSearchResultsTable ?? 0, + ), + [numSearchResultsMetadata, numSearchResultsTimeline, numSearchResultsTable], + ); ... - {numResults} + {numResults.toLocaleString()}Also applies to: 60-65
3-7: Use a type‑only import forGetPropsto keep the runtime bundle lean.-import { - GetProps, - Typography, -} from "antd"; +import type {GetProps} from "antd"; +import {Typography} from "antd";
♻️ Duplicate comments (1)
components/webui/client/src/pages/SearchPage/index.tsx (1)
25-27: Update PR summary or gate QueryStatus—this change impacts all engines, not PRESTO-only.
<SearchControls/>now containsQueryStatusand is rendered for every engine. The PR description says “Change affects only Presto,” which is no longer accurate unlessQueryStatusis engine‑gated insideSearchControls.If
QueryStatustruly should be PRESTO‑only, wrap it in a PRESTO check withinSearchControls. Otherwise, please update the PR summary to reflect the broader impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx
🔇 Additional comments (4)
components/webui/client/src/pages/SearchPage/index.tsx (1)
23-23: PRESTO‑gated progress bar looks good.The progress bar remains correctly gated to PRESTO usage.
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/Results.tsx (1)
8-9: Import path realignment is correct.The relative paths to
SearchStatewere updated appropriately for the new directory layout.components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx (2)
3-4: Import path updates look correct.
16-16: Rename from SearchQueryStatus → QueryStatus is consistent and scoped.The component rename and default export update are cohesive and do not alter the public surface beyond the name.
Also applies to: 42-42
| {SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO && <ProgressBar/>} | ||
| <div className={styles["searchPageContainer"]}> | ||
| <div> | ||
| <SearchControls/> | ||
| <SearchQueryStatus/> | ||
| </div> | ||
| <SearchControls/> | ||
| {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} | ||
| <SearchResultsTable/> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor readability: factor out the engine check to avoid repetition.
Extracting a local isPresto flag improves readability and keeps the engine condition consistent across the component.
useUpdateStateWithMetadata();
- return (
+ const isPresto = SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO;
+ return (
<>
- {SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO && <ProgressBar/>}
+ {isPresto && <ProgressBar/>}
<div className={styles["searchPageContainer"]}>
<SearchControls/>
- {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>}
+ {false == isPresto && <SearchResultsTimeline/>}
<SearchResultsTable/>
</div>
</>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO && <ProgressBar/>} | |
| <div className={styles["searchPageContainer"]}> | |
| <div> | |
| <SearchControls/> | |
| <SearchQueryStatus/> | |
| </div> | |
| <SearchControls/> | |
| {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO && <SearchResultsTimeline/>} | |
| <SearchResultsTable/> | |
| useUpdateStateWithMetadata(); | |
| const isPresto = SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.PRESTO; | |
| return ( | |
| <> | |
| {isPresto && <ProgressBar/>} | |
| <div className={styles["searchPageContainer"]}> | |
| <SearchControls/> | |
| {false == isPresto && <SearchResultsTimeline/>} | |
| <SearchResultsTable/> | |
| </div> | |
| </> | |
| ); |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/index.tsx around lines 23 to 27,
the code repeats the engine check SETTINGS_QUERY_ENGINE ===
CLP_QUERY_ENGINES.PRESTO in multiple JSX spots; introduce a local boolean const
(e.g. isPresto) at the top of the component by evaluating that expression once,
then replace the repeated checks with the isPresto variable so the JSX reads
{isPresto && <ProgressBar/>} and {!isPresto && <SearchResultsTimeline/>},
improving readability and ensuring a single source of truth for the engine
condition.
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx
Show resolved
Hide resolved
| <div className={styles["statusRow"]}> | ||
| <QueryStatus/> | ||
| </div> |
There was a problem hiding this comment.
| <div className={styles["statusRow"]}> | |
| <QueryStatus/> | |
| </div> | |
| <div className={styles["buttonAndStatusRow"]}> | |
| <QueryStatus/> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css (2)
6-10: Fill available width; prefer scalable spacingAdd width: 100% so the container spans the editor width; optionally switch gap to rem to respect user font scaling.
.prestoSearchControlsContainer { display: flex; flex-direction: column; - gap: 10px; + gap: 0.625rem; + width: 100%; }
12-17: Baseline alignment + wrapping; drop space-between to avoid filler div hackBaseline keeps button/status text aligned across browsers; wrapping prevents long IDs from forcing horizontal scroll. Removing justify-content: space-between lets us push the button with margin-left: auto instead of an extra flex spacer.
.buttonAndStatusRow { display: flex; - align-items: flex-start; /* Align items to the top */ - justify-content: space-between; /* Put space between status and button */ - gap: 10px; + align-items: baseline; + gap: 0.625rem; + flex-wrap: wrap; }Add a small utility class to support the TSX change (push button right):
.mlAuto { margin-left: auto; }components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
34-45: Non‑Presto layout deviates from PR scope and UX; group button+status and remove extra wrapperPR description says “Change affects only Presto,” but this also adds QueryStatus for non‑Presto. If intentional, update the PR summary; otherwise revert. UX‑wise, place status beside the run button for parity and drop the anonymous outer div.
- {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO ? - ( - <div> - <div className={styles["searchControlsContainer"]}> - {CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} - <QueryInput/> - <TimeRangeInput/> - <SearchButton/> - </div> - <QueryStatus/> - </div> - ) : + {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO ? ( + <> + <div className={styles.searchControlsContainer}> + {CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} + <QueryInput/> + <TimeRangeInput/> + </div> + <div className={styles.buttonAndStatusRow}> + <QueryStatus/> + <SearchButton/> + </div> + </> + ) :
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
components/webui/client/src/config/index.ts (4)
SETTINGS_QUERY_ENGINE(26-26)CLP_QUERY_ENGINES(24-24)CLP_STORAGE_ENGINES(25-25)SETTINGS_STORAGE_ENGINE(27-27)components/webui/common/index.ts (1)
CLP_QUERY_ENGINES(148-148)
🪛 GitHub Actions: clp-lint
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx
[error] 51-51: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space after '{'.
[error] 51-51: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space before '}'.
[error] 54-54: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space after '{'.
[error] 54-54: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space before '}'.
[error] 54-54: ESLint: '@stylistic/jsx-tag-spacing' violation: A space is forbidden before closing bracket.
[error] 55-55: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space after '{'.
[error] 55-55: ESLint: '@stylistic/object-curly-spacing' violation: There should be no space before '}'.
[error] 55-55: ESLint: '@stylistic/jsx-tag-spacing' violation: A space is forbidden before closing bracket.
🔇 Additional comments (2)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
12-12: Good: co-locating QueryStatus with controlsImport is correct and matches the new composition.
27-27: JSDoc aligns with intentComment accurately reflects that status is rendered with the controls.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/webui/client/src/components/SqlEditor/monaco-loader.ts (1)
12-21: Guardselfassignment for SSR/test environmentsAvoid ReferenceError when this module is evaluated outside a browser/worker.
-self.MonacoEnvironment = { +if ("undefined" != typeof self) { + // Worker config for browser/worker contexts only + self.MonacoEnvironment = { /** * Creates a web worker for Monaco Editor. * * @return */ - getWorker () { - return new EditorWorker(); - }, -}; + getWorker () { + return new EditorWorker(); + }, + }; +}components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
22-24: Ensure Enter Key Triggers Search
The form’s submit handler currently only callsev.preventDefault()and never invokes the search logic. As a result, pressing Enter inside the input will do nothing. To address this:• In components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (lines 22–24 and 33):
– UpdatehandleSubmitto call the actual search action (for example, dispatching the query or invokingrunSearch()), afterev.preventDefault().
– Verify that the “Search” button inside this<form>hastype="submit"so that hitting Enter triggersonSubmit={handleSubmit}.Please implement these changes and confirm that pressing Enter in the search input executes the query.
♻️ Duplicate comments (4)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx (1)
22-36: Guard against missing job ID and add live region semantics (prevents “undefined” flicker, improves a11y)Show status only when
searchJobIdis present; expose updates viarole="status"andaria-live. This repeats prior feedback.- return ( - <div> - {(searchUiState === SEARCH_UI_STATE.QUERYING || - searchUiState === SEARCH_UI_STATE.DONE) && ( + return ( + <div role="status" aria-live="polite" aria-atomic="true"> + {(null != searchJobId && "" !== searchJobId && + (searchUiState === SEARCH_UI_STATE.QUERYING || + searchUiState === SEARCH_UI_STATE.DONE)) && ( <Text type={"secondary"}> Search job {" "} {searchJobId} {" "} found {" "} </Text> )} <Results/> <Text type={"secondary"}> results</Text> </div> );components/webui/client/src/pages/SearchPage/SearchControls/index.module.css (2)
6-10: Ensure the PRESTO container spans available widthPrevents odd spacing when children wrap.
.prestoSearchControlsContainer { display: flex; flex-direction: column; gap: 10px; + width: 100%; }
12-17: Row alignment: use baseline and allow wrapping for stability; avoid “space-between” gapsBaseline aligns text with button labels across browsers; wrapping prevents long job IDs from forcing horizontal scroll.
.buttonAndStatusRow { display: flex; - align-items: flex-start; - justify-content: space-between; - gap: 10px; + align-items: baseline; + justify-content: flex-start; + flex-wrap: wrap; + gap: 0.625rem; /* 10px */ }components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
34-47: Scope mismatch vs PR summary; align non-PRESTO layout or update PR descriptionPR says “affects only Presto,” but non-PRESTO now renders
QueryStatusinsideSearchControls. If intentional, update the PR summary; if not, revert or match PRESTO layout (status beside button).- {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO ? - ( - <div> - <div className={styles["searchControlsContainer"]}> - {CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} - <QueryInput/> - <TimeRangeInput/> - <SearchButton/> - </div> - <div className={styles["status"]}> - <QueryStatus/> - </div> - </div> - ) : + {SETTINGS_QUERY_ENGINE !== CLP_QUERY_ENGINES.PRESTO ? + ( + <> + <div className={styles["searchControlsContainer"]}> + {CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} + <QueryInput/> + <TimeRangeInput/> + </div> + <div className={styles["buttonAndStatusRow"]}> + <SearchButton/> + <div className={styles["status"]}> + <QueryStatus/> + </div> + </div> + </> + ) :
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
components/webui/client/src/components/SqlEditor/index.tsx(2 hunks)components/webui/client/src/components/SqlEditor/monaco-loader.ts(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsx(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx(3 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsxcomponents/webui/client/src/components/SqlEditor/monaco-loader.tscomponents/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsxcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-07-21T14:37:57.648Z
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.648Z
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.
Applied to files:
components/webui/client/src/components/SqlEditor/monaco-loader.ts
📚 Learning: 2025-08-26T13:45:46.445Z
Learnt from: davemarco
PR: y-scope/clp#1244
File: components/webui/client/package.json:29-29
Timestamp: 2025-08-26T13:45:46.445Z
Learning: The "color" package in components/webui/client/package.json is used in the SqlEditor component to convert Ant Design token colors to hex format for Monaco editor themes, specifically for the disabled state styling.
Applied to files:
components/webui/client/src/components/SqlEditor/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (2)
components/webui/client/src/config/index.ts (4)
SETTINGS_QUERY_ENGINE(26-26)CLP_QUERY_ENGINES(24-24)CLP_STORAGE_ENGINES(25-25)SETTINGS_STORAGE_ENGINE(27-27)components/webui/common/index.ts (1)
CLP_QUERY_ENGINES(148-148)
🔇 Additional comments (6)
components/webui/client/src/pages/SearchPage/SearchControls/QueryStatus/index.tsx (1)
3-4: Import path updates look correctcomponents/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
12-12: New import is correct and localises status with controlscomponents/webui/client/src/components/SqlEditor/index.tsx (2)
75-85: Disabled theme setup looks good.
56-89: Confirmed: SqlEditor is only used in the Presto flowA full-text search across
.tsxand.tsfiles shows only one external import and JSX usage ofSqlEditor, located in
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlQueryInput/index.tsxlines 10, 23, 51No other references to
SqlEditorwere found in the repository. Therefore, defining the new default and disabled themes in the sharedSqlEditorcomponent will not impact any non-Presto engines.components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/CancelButton/index.tsx (1)
37-37: LGTM: Button size set to “middle” fits the compact PRESTO layoutMatches the Run button sizing and the reduced container width; no functional impact.
components/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/RunButton/index.tsx (1)
39-39: LGTM: Sizing aligned with Cancel and container shrinkKeeps visual parity with Cancel and should fit the 100px container; behaviour unchanged.
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx
Outdated
Show resolved
Hide resolved
...nts/webui/client/src/pages/SearchPage/SearchControls/Presto/SqlSearchButton/index.module.css
Show resolved
Hide resolved
davemarco
changed the title
There was a problem hiding this comment.
looks good! The new ui looks neat professional and neat!
I remember we need to reduce top padding of the search page in the standup?
.searchPageContainer {
display: flex;
flex-direction: column;
gap: 16px;
padding: 16px 16px 16px;
}
How about this title:
feat(webui): Move run button under SQL query input; Modify SQL editor theming to look like AntD input.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Deferring to @hoophalab's review.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/webui/client/src/components/SqlEditor/index.tsx (2)
24-29: Ref forwarding is broken; wrap component with forwardRef.Function components don’t receive
refvia props. CurrentuseImperativeHandle(ref, …)is a no-op. Ref-based focus will never work.Apply:
-import { - useCallback, - useEffect, - useImperativeHandle, - useRef, -} from "react"; +import { + useCallback, + useEffect, + useImperativeHandle, + useRef, + forwardRef, +} from "react"; -type SqlEditorProps = Omit<EditorProps, "language"> & React.RefAttributes<SqlEditorRef> & { +type SqlEditorProps = Omit<EditorProps, "language"> & { disabled: boolean; /** Callback when the editor is mounted and ref is ready to use. */ onEditorReady?: () => void; }; -const SqlEditor = (props: SqlEditorProps) => { - const {ref, disabled, onEditorReady, ...editorProps} = props; +const SqlEditor = forwardRef<SqlEditorRef, SqlEditorProps>((props, forwardedRef) => { + const {disabled, onEditorReady, ...editorProps} = props; - useImperativeHandle(ref, () => ({ + useImperativeHandle(forwardedRef, () => ({ focus: () => { editorRef.current?.focus(); }, }), []); -}; +});Also applies to: 37-47
15-15: Avoid bundling Monaco runtime for types—use type-only import.Prevents unnecessary code in the client bundle.
-import * as monaco from "monaco-editor/esm/vs/editor/editor.api.js"; +import type * as monaco from "monaco-editor/esm/vs/editor/editor.api.js";components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
22-24: Avoid React namespace types; import FormEventReduces reliance on ambient namespace; keeps typing explicit.
-const handleSubmit = (ev: React.FormEvent<HTMLFormElement>) => { +const handleSubmit = (ev: FormEvent<HTMLFormElement>) => { ev.preventDefault(); };Add import (outside this range):
import type { FormEvent } from "react";
♻️ Duplicate comments (7)
components/webui/client/src/components/SqlEditor/index.tsx (5)
62-71: Don’t remove focus visuals (WCAG 2.4.7)—use a visible focusBorder.Transparent focus eliminates keyboard focus indication.
colors: { "editor.background": color(token.colorBgContainer).hexa(), "editor.foreground": color(token.colorText).hexa(), - "focusBorder": "#0000", + "focusBorder": color(token.colorPrimary).hexa(), },
73-82: Same for disabled theme—keep a subtle but visible focus.Use a toned-down primary instead of transparent.
colors: { "editor.background": color(token.colorBgContainerDisabled).hexa(), "editor.foreground": color(token.colorTextDisabled).hexa(), - "focusBorder": "#0000", + "focusBorder": color(token.colorPrimary).alpha(0.45).hexa(), },
90-96: Expose disabled state to assistive tech; improve cursor affordance.Add
aria-disabledand adjust cursor.- <div + <div + aria-disabled={disabled} style={{ border: `1px solid ${token.colorBorder}`, borderRadius: token.borderRadius, pointerEvents: disabled ? "none" : "auto", + cursor: disabled ? "not-allowed" : "text", }} >
108-122: Disabled editor still accepts keyboard input—set readOnly and ARIA label.
pointer-events: nonedoesn’t block keyboard input; Monaco needsreadOnly. Also label the control.options={{ automaticLayout: true, folding: false, lineNumbers: "off", minimap: {enabled: false}, overviewRulerBorder: false, + readOnly: disabled, padding: { top: token.paddingXS, bottom: token.paddingXS, }, placeholder: "Enter your SQL query", renderLineHighlight: "none", scrollBeyondLastLine: false, + ariaLabel: disabled ? "SQL editor (disabled)" : "SQL editor", wordWrap: "on", }}
123-126: Guard theme until Monaco is ready to avoid flicker/fallback logs.Pass a built-in theme before custom themes are registered.
- theme={disabled ? - "disabled-theme" : - "default-theme"} + theme={monacoEditor ? + (disabled ? "disabled-theme" : "default-theme") : + "vs"}components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
34-47: Non‑PRESTO: align layout with PRESTO or update PR scopeEither group button+status in one row (parity) or update PR description to note non‑PRESTO UI changes.
- <div className={styles["searchControlsContainer"]}> - <div className={styles["inputsAndButtonRow"]}> + <div className={styles["searchControlsContainer"]}> + <div className={styles["inputsAndButtonRow"]}> {CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} <QueryInput/> <TimeRangeInput/> - <SearchButton/> - </div> - <div className={styles["status"]}> - <QueryStatus/> - </div> + </div> + <div className={styles["buttonAndStatusRow"]}> + <div className={styles["status"]}> + <QueryStatus/> + </div> + <SearchButton/> + </div> </div>components/webui/client/src/pages/SearchPage/SearchControls/index.module.css (1)
19-21: Drop micro left margin; rely on gap; use rem for paddingSimplifies spacing and scales with font size.
.status { - margin-left: 2px; - padding-top: 4px; + padding-top: 0.25rem; /* 4px */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/webui/client/src/components/SqlEditor/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.module.css(1 hunks)components/webui/client/src/pages/SearchPage/SearchControls/index.tsx(2 hunks)components/webui/client/src/pages/SearchPage/index.module.css(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/client/src/pages/SearchPage/SearchControls/index.tsxcomponents/webui/client/src/components/SqlEditor/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-26T13:45:46.445Z
Learnt from: davemarco
PR: y-scope/clp#1244
File: components/webui/client/package.json:29-29
Timestamp: 2025-08-26T13:45:46.445Z
Learning: The "color" package in components/webui/client/package.json is used in the SqlEditor component to convert Ant Design token colors to hex format for Monaco editor themes, specifically for the disabled state styling.
Applied to files:
components/webui/client/src/components/SqlEditor/index.tsx
🧬 Code graph analysis (1)
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
components/webui/client/src/config/index.ts (4)
SETTINGS_QUERY_ENGINE(26-26)CLP_QUERY_ENGINES(24-24)CLP_STORAGE_ENGINES(25-25)SETTINGS_STORAGE_ENGINE(27-27)
🔇 Additional comments (3)
components/webui/client/src/components/SqlEditor/index.tsx (2)
56-61: Theme setup effect looks good.Guarding on
monacoEditor === nullis correct; tokens in deps are fine.
103-103: Loading skeleton theming—LGTM.Matches AntD container background.
components/webui/client/src/pages/SearchPage/SearchControls/index.tsx (1)
49-56: PRESTO structure LGTMStatus and run button co‑located; matches PR intent.
| gap: 16px; | ||
| padding: 16px 16px 16px; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use padding shorthand for clarity
Equivalent and shorter.
- gap: 16px;
- padding: 16px 16px 16px;
+ gap: 16px;
+ padding: 16px;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gap: 16px; | |
| padding: 16px 16px 16px; | |
| gap: 16px; | |
| padding: 16px; |
🤖 Prompt for AI Agents
In components/webui/client/src/pages/SearchPage/index.module.css around lines
4-5, the padding is written as "padding: 16px 16px 16px;" which is redundant;
replace it with the concise equivalent "padding: 16px;" to use the shorthand and
keep the behavior identical.
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
components/webui/client/src/pages/SearchPage/SearchControls/index.module.css
Show resolved
Hide resolved
|
not sure if this is the breaking PR, but the progress bar overflows the window width once it shows up. can we double check? @coderabbitai create an issue to track this |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create an issue to track the progress bar overflow problem you've reported. @junhaoliao I've created an issue to track the progress bar overflow problem you reported. The issue will help investigate whether this PR is causing the overflow or if it's an existing problem that needs to be addressed separately. 🏁 Script executed: Length of output: 42 Perfect! I've successfully created issue #1305 to track the progress bar overflow problem. You can view it at: #1305 The issue has been assigned to you and includes all the relevant context from your report, including references to this PR. This will help investigate whether the overflow is caused by the changes in this PR or if it's a separate issue that needs to be addressed. |
4 participants
Description
Moves run button beneath the editor for presto. Modify input to look more like antd.
Checklist
breaking change.
Validation performed
Components are moved. did not break native ui
Summary by CodeRabbit
New Features
Style
Refactor