diff --git a/snuba/admin/static/clickhouse_queries/index.tsx b/snuba/admin/static/clickhouse_queries/index.tsx index e41b9647c1..ec2b8b8d7f 100644 --- a/snuba/admin/static/clickhouse_queries/index.tsx +++ b/snuba/admin/static/clickhouse_queries/index.tsx @@ -1,7 +1,10 @@ import React, { useState, useEffect } from "react"; import Client from "SnubaAdmin/api_client"; import { Table } from "SnubaAdmin/table"; -import QueryDisplay from "SnubaAdmin/clickhouse_queries/query_display"; +import QueryDisplay, { + STICKY_HEADER_TOP, +} from "SnubaAdmin/clickhouse_queries/query_display"; +import { COLORS } from "SnubaAdmin/theme"; import { parseSystemQueryPrefill } from "SnubaAdmin/clickhouse_queries/prefill"; import { QueryResult, PredefinedQuery } from "./types"; @@ -34,12 +37,11 @@ function ClickhouseQueries(props: { api: Client }) { function tablePopulator(queryResult: QueryResult) { return ( -
- - +
); } @@ -68,9 +70,19 @@ const selectStyle = { height: 30, }; -const scroll = { - overflowX: "scroll" as const, - width: "100%", +// Keeps column names in view below the editor pane while scrolling a long +// result. This needs no scrolling ancestor between the table and the page, +// so wide results wrap rather than scroll sideways. +const stickyHeaderStyles = { + thStyle: { + position: "sticky" as const, + top: STICKY_HEADER_TOP, + zIndex: 1, + backgroundColor: COLORS.TABLE_HEADER_BG, + // Collapsed borders don't move with a stuck cell, so draw its bottom + // edge here. + boxShadow: `inset 0 -1px 0 ${COLORS.TABLE_BORDER}`, + }, }; export default ClickhouseQueries; diff --git a/snuba/admin/static/clickhouse_queries/query_display.tsx b/snuba/admin/static/clickhouse_queries/query_display.tsx index dab886da33..908d8861e1 100644 --- a/snuba/admin/static/clickhouse_queries/query_display.tsx +++ b/snuba/admin/static/clickhouse_queries/query_display.tsx @@ -1,8 +1,16 @@ -import React, { useEffect, useState } from "react"; +import React, { useEffect, useRef, useState } from "react"; import Client from "SnubaAdmin/api_client"; import QueryEditor from "SnubaAdmin/query_editor"; import ExecuteButton from "SnubaAdmin/utils/execute_button"; -import { Alert, Box, SegmentedControl, Text, Tooltip } from "@mantine/core"; +import { + Alert, + Button, + SegmentedControl, + Text, + Tooltip, + UnstyledButton, +} from "@mantine/core"; +import { IconChevronDown, IconX } from "@tabler/icons-react"; import { getRecentHistory, setRecentHistory } from "SnubaAdmin/query_history"; import { COLORS, SECTION_HEADING_STYLE } from "SnubaAdmin/theme"; import { CustomSelect, getParamFromStorage } from "SnubaAdmin/select"; @@ -24,6 +32,10 @@ import { type QueryState = Partial; const HISTORY_KEY = "clickhouse_queries"; +// Set on the page to the editor pane's height; see STICKY_HEADER_TOP. +const EDITOR_PANEL_HEIGHT_VAR = "--system-queries-editor-height"; +// Where content stuck to the top of the results sits: just below the pane. +const STICKY_HEADER_TOP = `var(${EDITOR_PANEL_HEIGHT_VAR}, 0px)`; function QueryDisplay(props: { api: Client; resultDataPopulator: (queryResult: QueryResult) => JSX.Element; @@ -49,6 +61,26 @@ function QueryDisplay(props: { // Only a result the user just ran opens automatically; saved history loads // collapsed. const [justRan, setJustRan] = useState(null); + // The editor sits in a sticky panel so it can be reopened from anywhere in + // the results. It closes after a successful run to make room for them. + const [editorOpen, setEditorOpen] = useState(true); + const resultsHeadingRef = useRef(null); + // Result tables pin their headers just below the pane, so they need its + // height, which changes as it opens, closes and the query grows. + const editorPanelRef = useRef(null); + const [editorPanelHeight, setEditorPanelHeight] = useState(0); + + useEffect(() => { + const panel = editorPanelRef.current; + if (!panel || typeof ResizeObserver === "undefined") { + return; + } + const observer = new ResizeObserver(() => + setEditorPanelHeight(panel.offsetHeight) + ); + observer.observe(panel); + return () => observer.disconnect(); + }, []); const [queryError, setQueryError] = useState(null); @@ -76,6 +108,14 @@ function QueryDisplay(props: { }); }, []); + // Bring a fresh result into view, since it may be above where the user + // scrolled to. (jsdom has no scrollIntoView.) + useEffect(() => { + if (justRan) { + resultsHeadingRef.current?.scrollIntoView?.({ block: "start" }); + } + }, [justRan]); + function selectStorage(storage: string) { setQuery((prevQuery) => { // clear old host port @@ -155,6 +195,7 @@ function QueryDisplay(props: { setRecentHistory(HISTORY_KEY, result); setQueryResultHistory((prevHistory) => [result, ...prevHistory]); setJustRan(result); + setEditorOpen(false); }); } @@ -189,129 +230,186 @@ function QueryDisplay(props: { }; return ( -
-
- {prefill && prefillWarningOpen && ( - setPrefillWarningOpen(false)} - style={prefillAlertStyle} - data-testid="prefill-warning" +
+
+ {!editorOpen && ( + // The whole bar opens the editor. The "button" inside is only a + // span, since a real button can't be nested in another. + setEditorOpen(true)} + aria-label="Open editor" + style={editorClosedBarStyle} + sx={{ "&:hover": { backgroundColor: COLORS.BG_GRAY_LIGHTER } }} > -

- Whoever clicks Execute is responsible for the query, no matter - who or what wrote it. Read the whole query and check where it - will run before you execute it. Don't run it just because an - agent or a link suggested it. -

-

- Storage: {prefill.storage ?? "(not set)"} + + + {query.sudo && ( + + SUDO{" "} + + )} + {query.storage ?? "(no storage)"} + {" · "} + {query.host ? `${query.host}:${query.port}` : "(no host)"} {" · "} - Host:{" "} - - {prefill.host ? `${prefill.host}:${prefill.port}` : "(not set)"} - -

- + {query.sql || "(no query)"} + + )} -

Construct a ClickHouse System Query

-
-
-
- - Host entry - - setClusterlessHost(value === "manual")} - data={[ - { label: "Pick a node", value: "pick" }, - { label: "Enter host manually", value: "manual" }, - ]} - /> -
-
- - Storage - -
- storage.storage_name)} + {/* Hidden rather than unmounted, so the editor keeps its state. */} + + {prefill && prefillWarningOpen && ( + setPrefillWarningOpen(false)} + style={prefillAlertStyle} + data-testid="prefill-warning" + > +

+ Whoever clicks Execute is responsible for the query, no matter + who or what wrote it. Read the whole query and check where it + will run before you execute it. Don't run it just because an + agent or a link suggested it. +

+

+ Storage: {prefill.storage ?? "(not set)"} + {" · "} + Host:{" "} + + {prefill.host ? `${prefill.host}:${prefill.port}` : "(not set)"} + +

+ + )} +
+

+ Construct a ClickHouse System Query +

+ +
+
+
+
+ + Host entry + + setClusterlessHost(value === "manual")} + data={[ + { label: "Pick a node", value: "pick" }, + { label: "Enter host manually", value: "manual" }, + ]} />
- {missing === "storage" && ( -

{missingHint.storage}

- )} -
-
- - Host - - {query.clusterless ? ( - <> - selectHostIp(evt.target.value)} - name="host" - placeholder="enter host or host:port..." - type="text" +
+ + Storage + +
+ storage.storage_name)} /> - {missing === "host" && ( -

{missingHint.host}

- )} -

- Must be a known cluster node or listed in - admin.copy_tables_allowed_target_hosts. Defaults to port - 8123. -

- - ) : ( - <> -
- + {missing === "storage" && ( +

{missingHint.storage}

+ )} +
+
+ + Host + + {query.clusterless ? ( + <> + selectHostIp(evt.target.value)} + name="host" + placeholder="enter host or host:port..." + type="text" /> -
- {missing === "host" && ( -

{missingHint.host}

- )} - - )} + {missing === "host" && ( +

{missingHint.host}

+ )} +

+ Must be a known cluster node or listed in + admin.copy_tables_allowed_target_hosts. Defaults to port + 8123. +

+ + ) : ( + <> +
+ +
+ {missing === "host" && ( +

{missingHint.host}

+ )} + + )} +
-
-
- { - updateQuerySql(sql); - }} - predefinedQueryOptions={props.predefinedQueryOptions} - initialQuery={prefill?.sql} - /> -
-
-
-
+
+ { + updateQuerySql(sql); + }} + predefinedQueryOptions={props.predefinedQueryOptions} + initialQuery={prefill?.sql} + /> +
+ {/* Sudo can change data, so it gets a red frame. */} +
+

{query.sudo ? "Sudo only runs SYSTEM, ALTER, OPTIMIZE and DROP statements. It needs the sudo role, and every sudo query is audit-logged with a notification." : "SELECT, SHOW and DESCRIBE. SELECTs may only read system tables."}

+ {/* A disabled button gets no hover events, so the tooltip + hangs off a wrapper. */} + +
+ +
+
- {/* A disabled button gets no hover events, so the tooltip - hangs off a wrapper. */} - -
- -
-
-
- + +
{getErrorDomElement(queryError, collapseOpened, setCollapseOpened)}
-

Query results

+

+ Query results +

{queryResultHistory.map((queryResult, idx) => ( - - {props.title} - - {props.children} - - ); -} - -const sectionStyle = { - border: "1px solid #ced4da", - borderRadius: 4, +// Spaces out the target, query and execute rows. +const blockStyle = { + marginBottom: 16, }; -const dangerSectionStyle = { - ...sectionStyle, +const sudoBlockStyle = { + ...blockStyle, + padding: 12, border: `2px solid ${COLORS.DANGER}`, + borderRadius: 4, backgroundColor: "rgba(168, 50, 45, 0.06)", }; -// A step smaller than the page headings, so sections read as nested. -// Bootstrap makes legends large, full-width and floated; undo that so the -// label sits in the border. -const sectionLegendStyle = { - ...SECTION_HEADING_STYLE, - fontSize: 12, - float: "none" as const, - width: "auto", - padding: "0 4px", - margin: 0, -}; - -const segmentedStyle = { - marginBottom: 8, -}; - const targetRowStyle = { display: "flex", flexWrap: "wrap" as const, @@ -452,7 +511,61 @@ const needsInputHintStyle = { margin: 0, }; +// Separates the results from the form above. +const resultsHeadingStyle = { + ...SECTION_HEADING_STYLE, + marginTop: 32, + // Keeps the heading clear of the sticky editor bar when scrolled to. + scrollMarginTop: 72, +}; + +// Sticks to the top of the page's scroll area, just below the header. +const editorPanelStyle = { + position: "sticky" as const, + top: 0, + zIndex: 10, + backgroundColor: COLORS.PANEL_BG, + border: `1px solid ${COLORS.BORDER_GRAY}`, + borderRadius: 4, + boxShadow: "0 2px 6px rgba(0, 0, 0, 0.08)", +}; + +// Scrolls on its own when the open editor is taller than the screen. +const editorFormStyle = { + maxHeight: "calc(100vh - 100px)", + overflowY: "auto" as const, + padding: "12px 16px 0", +}; + +const editorTitleRowStyle = { + display: "flex", + justifyContent: "space-between", + alignItems: "center", + marginBottom: 12, +}; + +const editorClosedBarStyle = { + display: "flex", + width: "100%", + borderRadius: 4, + alignItems: "center", + gap: 12, + padding: "8px 16px", +}; + +// One line, so the closed bar stays short whatever the query. +const editorSummaryStyle = { + minWidth: 0, + overflow: "hidden", + textOverflow: "ellipsis", + whiteSpace: "nowrap" as const, + color: COLORS.TEXT_LIGHTER, +}; + +// Takes the space between the mode toggle and Execute. const modeHelpTextStyle = { + flex: 1, + minWidth: 0, fontSize: "0.85em", margin: 0, }; @@ -479,9 +592,9 @@ const hostHelpTextStyle = { const executeActionsStyle = { display: "flex", - justifyContent: "space-between", - alignItems: "flex-start", + alignItems: "center", gap: 16, }; +export { STICKY_HEADER_TOP }; export default QueryDisplay; diff --git a/snuba/admin/static/clickhouse_queries/query_history_entry.tsx b/snuba/admin/static/clickhouse_queries/query_history_entry.tsx index 226573c309..88d4cf595d 100644 --- a/snuba/admin/static/clickhouse_queries/query_history_entry.tsx +++ b/snuba/admin/static/clickhouse_queries/query_history_entry.tsx @@ -90,7 +90,9 @@ const cardStyle = { border: `1px solid ${COLORS.BORDER_GRAY_LIGHT}`, borderRadius: 8, marginBottom: 12, - overflow: "hidden", + // Clips to the rounded corners. Unlike "hidden", "clip" doesn't stop the + // result table's header from sticking. + overflow: "clip", }; const headerStyle = { diff --git a/snuba/admin/static/query_editor.tsx b/snuba/admin/static/query_editor.tsx index 7129983858..aed09eb325 100644 --- a/snuba/admin/static/query_editor.tsx +++ b/snuba/admin/static/query_editor.tsx @@ -210,10 +210,10 @@ function QueryEditor(props: { function renderPredefinedQueriesSelectors() { return (
- + Predefined query -
+
{ @@ -475,11 +475,15 @@ function QueryEditor(props: { ); } +// Label and picker on one line. const predefinedQueryStyle = { - width: "20em", display: "flex", - flexDirection: "column" as const, - gap: 4, + alignItems: "center", + gap: 8, +}; + +const predefinedQuerySelectStyle = { + width: "20em", }; const timeRangeStyle = { diff --git a/snuba/admin/static/tests/clickhouse_queries/prefill.spec.tsx b/snuba/admin/static/tests/clickhouse_queries/prefill.spec.tsx index d3a6898988..c8491a912f 100644 --- a/snuba/admin/static/tests/clickhouse_queries/prefill.spec.tsx +++ b/snuba/admin/static/tests/clickhouse_queries/prefill.spec.tsx @@ -6,6 +6,7 @@ import { render, screen, waitFor, + within, } from "@testing-library/react"; import Client from "SnubaAdmin/api_client"; import ClickhouseQueries from "SnubaAdmin/clickhouse_queries"; @@ -192,6 +193,41 @@ describe("QueryDisplay opened from a link", () => { expect(screen.queryByText("Select a host to run the query.")).toBeNull(); }); + it("closes the editor after a run and reopens it with the query kept", async () => { + const api = renderWithHash( + "#system-queries?query=SELECT%201&storage=errors&host=clickhouse-01%3A9000" + ); + await waitFor(() => expect(api.getClickhouseNodes).toBeCalled()); + await screen.findAllByDisplayValue("clickhouse-01:9000"); + + fireEvent.click(screen.getByText("Execute Query")); + await screen.findByText("Open Editor"); + expect(api.executeSystemQuery).toBeCalled(); + expect(screen.queryByRole("textbox")).toBeNull(); + // The closed bar says what will run. + expect(screen.getByTestId("editor-panel").textContent).toContain( + "errors · clickhouse-01:9000 · SELECT 1" + ); + + // Anywhere on the bar opens it, not just the button. + fireEvent.click( + within(screen.getByTestId("editor-panel")).getByText("SELECT 1", { + selector: "code", + }) + ); + expect(screen.getByRole("textbox").textContent).toBe("SELECT 1"); + expect(screen.queryByText("Open Editor")).toBeNull(); + }); + + it("closes the editor with the X", async () => { + const api = renderWithHash("#system-queries"); + await waitFor(() => expect(api.getClickhouseNodes).toBeCalled()); + + fireEvent.click(screen.getByLabelText("Close editor")); + expect(screen.getByText("Open Editor")).toBeTruthy(); + expect(api.executeSystemQuery).not.toBeCalled(); + }); + it("shows no warning without params", async () => { const api = renderWithHash("#system-queries"); await waitFor(() => expect(api.getClickhouseNodes).toBeCalled());