UX-1330 - feat(sql): add SQL query workspace with Iceberg bridge#2495
Conversation
a2d19ea to
3e1e3eb
Compare
b9b144c to
c37c3e9
Compare
|
@claude review |
|
Claude finished @c-julin's task in 2m 51s —— View job Code Review — SQL query workspace with Iceberg bridgeReviewed the diff against 🐞 Bugs / correctness
⚡ Performance
🔒 Minor / data-handling
♿ Accessibility
✅ Things I liked / verified
Notes
None of the above is blocking on its own, but #1 (CTE rejection) and #4 (N+1 requests) are worth addressing before this ships widely. |
|
@claude review |
|
Claude finished @c-julin's task in 2m 33s —— View job Code Review — SQL query workspace with Iceberg bridge (round 2)Re-reviewed against
Nice turnaround. A few fresh observations from this pass, none blocking: 🔒 Read-only gate is client-side only
⚡ Every single-table SELECT fires 2 metric queriesIn 📤 CSV export: header row isn't escaped/neutralized
🐛 Minor: side effects inside a state updater (
|
malinskibeniamin
left a comment
There was a problem hiding this comment.
Review — SQL query workspace + Iceberg bridge (#2495)
Multi-hat review (ponytail, security, resilience, regular, adversarial, visual, test-perf, thermo-nuclear) against merge-base 44fc8fe2. This is a large, well-structured feature (~4k LOC) with good test coverage on the leaf components. Inline comments cover the actionable findings; this summary holds the value gate, the open question for the backend reviewer, what was cleared, and P3/follow-ups.
Open question (sets final severity of two inline findings)
The PR notes backend SQL auth/config ships separately. Does SQLService.ExecuteQuery independently enforce (a) read-only and (b) the SQL license / enableSqlInConsole gate server-side? If yes, the /sql route guard and the SELECT-only gate are defense-in-depth/UX; if no, the client gate is the only barrier and both rise in severity.
Cleared with evidence (not issues)
- No XSS in results rendering — all cells render as React text children; no
dangerouslySetInnerHTML/setHTML; CSV/JSON export goes throughBlob. - CREATE TABLE interpolation is safe —
tableNameis zod-validated^[a-z_][a-z0-9_]*$;topiccomes from the server topic list and Kafka topic names can't contain a quote. zod@^4.3.6is pre-existing on master, not introduced here.routeTree.gen.tsis genuine codegen output.- Editor
outline: noneis paired with a caret + active-line highlight (conventional focus indication); icon buttons'titleprovides a fallback accessible name — minor, not posted.
P3 / follow-ups (not posted inline)
sql-workspace.tsx(657 LOC — the core logic:doRungate, run-token guard,useStudioModepersistence, helpers) has no test file, while every leaf component does. Add coverage for the gate branches andcellValueForColumn/resultRowFromProto.- The
bridgelaguseMemocan briefly pair the new topic name with the previous topic's lag during a topic switch (transient display inaccuracy) — gate onisFetching. completionCatalogs→ editorextensionsrebuild on everyredpandaTablesDatarefetch (new object ref) → editor reconfiguration churn; stabilize the schema memo.bun.lockdropped the top-levelconfigVersion: 0— confirm the bun version matches CI to avoid frozen-install drift.catalog-tree.tsxusesbiome-ignore noExcessiveCognitiveComplexityonTableRow/NamespaceNoderather than extracting sub-components.globals.csscarries a UX-1259 dark-mode border fix inside this UX-1330 PR — note it in the description for traceability.- A11y polish: disabled "Run selection" button has no tooltip explaining why;
DataGridand the table-specific icon buttons would benefit from explicitaria-labels. sqlRoledefaults toviewerstandalone (admin injected by cloud-ui via federation) — a one-line comment would stop it reading as missing wiring.
Value gate
Major improvement: a full SQL query workspace (catalog tree, CodeMirror editor, results grid, Iceberg bridge wizard) — net-new product capability. Value: HIGH. Steelman not needed.
Counts
Posted inline: P1 ×3, P2 ×7, P3 ×2. Plus ~8 P3/follow-ups in this summary. No P0.
🤖 Multi-hat review via Claude Code
malinskibeniamin
left a comment
There was a problem hiding this comment.
Left some comments
SQL editor/workspace refactor (catalog-tree, sql-editor, sql-results, sql-wizard, sql-workspace, sql, sql API), plus vendored redpanda-ui registry component updates and supporting route/config changes. [UX-1259]
…tokens [UX-1259] Map palette literals (blue/green/orange/indigo/purple) in the SQL workspace to semantic registry tokens (info/success/warning/accent/primary) and snap arbitrary typography/radius utilities (text-[Npx], rounded-[3px], leading/ tracking-[...]) to scale tokens. Drop now-redundant dark: variants since the semantic tokens are theme-adaptive. Lookout audit: 0 off-token, 0 ad-hoc.
Remove three style={{ borderColor: 'var(--color-border)' }} props that just
re-asserted the base-layer default (* { border-color: var(--color-border) }),
and move the two static bridge-timeline widths to w-full/w-[74%]/w-[26%]
utility classes.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…es [UX-1330] Keywords for autocomplete now come from Monaco's built-in SQL Monarch definition, formatting runs through sql-formatter behind Monaco's native formatDocument action (preserving cursor and undo), and the wizard preview uses the registry DynamicCodeBlock (shiki) instead of a custom tokenizer rendered via dangerouslySetInnerHTML. firstKeyword, which gates the editor to SELECT-only, is a small comment-skipping regex. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
The overlay controls were design-phase scaffolding for the SQL workspace; nothing reads the overlay query param anymore. Co-Authored-By: Claude <noreply@anthropic.com>
…s [UX-1330] JSON/JSONB columns now show a braces icon instead of the generic text glyph, and array types (TEXT[], _INT4, ARRAY<...>/LIST<...>) unwrap to their element kind with a brackets prefix. BOOL[] cells keep their raw string form instead of coercing to a scalar boolean. Also replace the results grid's manual useMemo with pure helpers (React Compiler memoizes).
…tree semantics [UX-1330] Replace hand-rolled spinner, badges, buttons, search input and text styles with registry components. Add role=tree/treeitem/group with aria-level/expanded/selected and arrow-key navigation per the WAI-ARIA tree pattern. Move tree-wide state into a context to cut per-node prop drilling, normalize arbitrary Tailwind values to the spacing scale, and cover the tree with component tests.
Mirrors ui-registry PR #205. When set, the table wrapper fills its flex-column parent and owns the scrolling, so sticky header/column cells pin to the container instead of the page. Defaults to false.
Replace hand-rolled markup in sql-results with registry primitives: Table/TableHead/TableCell (fill variant) for the grid, Empty + Kbd + Spinner for idle/running states, Alert for query errors, Badge for the bridge/truncated/watermark chips, and StatusDot for success/lag markers. Adds aria-sort-free static headers and normalizes one-off arbitrary values to scale tokens where exact. Behavior changes: - Drop client-side sorting; rows render in server order and ordering belongs to the query (ORDER BY). Exports follow server order too. - Replace the pagination-reset useEffect by keying SuccessGrid on run.token so a new run remounts with fresh paging state.
Tabs/Popover/Kbd/Button replace hand-rolled tab strip, history dropdown and shortcut chips; Run uses the secondary variant instead of !important overrides. Kbd hint is platform-aware (Ctrl on Windows/Linux). Monaco language providers move to module scope (global per language, registered once) and the Cmd/Ctrl+Enter command reads through a render-synced ref, dropping all useEffects and manual memoization.
…1330] Remove the unused ProtoCatalog/ProtoTable/ProtoColumn re-exports, unexport the QueryRun variants only consumed via the union, inline the trivial shortPgType helper at its two call sites, and trim comments that restated their declarations.
A global stylesheet forces the indicator's Circle svg to inline-block, so it sat baseline-aligned in its wrapper's 21px line box and rendered ~1px below the ring's center. Make the wrapper a flex container so the svg is blockified and centered exactly.
Replace the hand-rolled topic radio list, progress bar, badges, alerts and table-name field with Choicebox, Progress, Badge, Alert and Field registry components, and validate the table name with react-hook-form + zod. Prefill a sanitized table name from the chosen topic, render the SQL preview as an always-dark SyncCodeBlock, and outline the catalog and source-topic summary rows so they stay visible in dark mode.
Cover topic search and selection gating, sanitized table-name prefill and SQL preview, validation rejection, create submission, iceberg badge and bridged-query notice, parent error rendering, and close.
Per design discussion: the results grid is a niche, perf-sensitive one-off, so it uses react-data-grid page-locally instead of the registry table. Virtualization renders only visible rows/columns, so the full result set is handed over and the Load-100-more pagination, its state, and the footer bar are gone. The frozen row-number column replaces the hand-rolled sticky CSS, themed via rdg CSS variables mapped to design tokens (unlayered stylesheet, since Tailwind's layered utilities cannot override rdg's own custom properties). Pinned to 7.0.0-beta.47, the last release supporting React 18. The test file shims the grid root's measured size because happy-dom reports zero dimensions, which would cull every column.
…1330] Per design discussion the registry table stays a plain shadcn component without app-specific variants (ui-registry PR #205 closed for the same reason). The SQL results grid now uses react-data-grid page-locally, so nothing consumes the variant.
max-content columns can come up short of the panel width, leaving a dead gap on the right with no header surface, zebra stripe or hover. A trailing non-resizable 1fr filler column absorbs the remainder; rdg re-measures string widths on grid resize so it stays flexed.
Size data columns with minmax(max-content, 1fr) instead of max-content plus a 1fr spacer column: each column is at least content-sized and spare panel width is shared between the real columns, so the grid fills horizontally without a synthetic filler.
Replace Monaco with @uiw/react-codemirror + @codemirror/lang-sql for the SQL workspace editor. Schema-aware, alias-aware completion driven by the catalog tree; catalog arrow notation (catalog=>table) suggestions; Tab to accept, Mod-Enter to run, Shift-Alt-f / Format via sql-formatter; light/dark themes from design tokens. Replace the layout useEffect with a callback ref. Dedupe @codemirror/* via package.json overrides to fix duplicate @codemirror/state instances. Drop the Monaco SQL language registration and the monaco-editor sql module shim; Monaco stays for the YAML/TS surfaces.
Self-hosted gates the SQL sidebar item on the REDPANDA_SQL app feature (injected by the enterprise backend when SQL is licensed); embedded keeps the enableSqlInConsole flag that cloud-ui sets per-cluster. Default the flag to false so SQL only appears where explicitly enabled.
Catalog tree, results grid, workspace layout, and shared SQL types for the SQL studio page.
…ed /sql [UX-1330] Fullscreen routes (the SQL studio) render minimal chrome. Detection now reads the typed staticData.fullscreen flag — StaticDataRouteOption is augmented in app.tsx so the per-call-site casts can go. The path fallback stays (on soft navigation useMatches() lags useLocation(), so staticData is briefly stale and the studio would flash full chrome on the way in) but no longer hardcodes /sql: fullscreen-routes.ts derives the paths from the route tree, so future fullscreen routes are covered automatically.
- JSON/composite result cells now open a rich popover: formatted, syntax-highlighted JSON with a header (name + type) and copy button - Drop per-type icons from catalog tree column rows and results column headers, keeping the text only - Remove the redundant Iceberg-lag chip from the bridge summary bar and reword the caption to "Bridge query covers …" - Remove the "Redpanda only" hint from the catalog tree header - Fix a temporal-dead-zone crash by ordering completionCatalogs ahead of the doRun callback that depends on it in sql-workspace
- Allow WITH/CTE and paren-wrapped read queries (not just SELECT); skip a leading paren in firstKeyword and gate on a READ_KEYWORDS set. - Memoize DataGrid columns/rowKeys so bridge resolution doesn't reset user column widths and scroll. - Collapse the duplicate Iceberg badge into a single computed flag. - Fetch per-table GetTopicConfigurations only when the row is expanded, avoiding N+1 requests across the catalog tree. - Neutralize CSV formula injection (cells starting with = + - @). - Drop the unused RunMode/mode param from the editor's onRun. - Rove tabIndex across treeitems so the catalog tree is a single tab stop.
Gate the SQL nav and /sql route on the SQLService being reported in endpoint compatibility (cfg.API.SQL.Enabled), instead of the enableSqlInConsole feature flag, so the backend is the single source of truth for both embedded and self-hosted. Adds a beforeLoad redirect on /sql so direct navigation can't bypass the gate (review P1).
- Gate the editor with a write/DDL/DCL denylist instead of a SELECT-only allowlist, so WITH/EXPLAIN/SHOW read-shaped queries run and the server rejects what it can't (P1). The first keyword stays a UX guard rail. - Strip SQL comments and single-quoted strings before bridge-ref detection, so `=>` inside them isn't treated as a catalog reference (P2). - Replace the module-global RUN_TOKEN with a per-instance useRef so concurrently-mounted workspaces don't drop each other's results (P2). - Lowercase boolean cell values so 'TRUE'/'T' don't render as false (P2). - Classify INTERVAL/POINT correctly: test temporal before numeric and word-boundary-anchor the numeric regex so they don't match INT (P2). - Build the CREATE TABLE statement from one shared createTableSql helper so the wizard preview can't drift from what runs (P2). - Match fullscreen paths only at the start or trailing segment, so an interior segment named `sql` (/clusters/sql/overview) doesn't match (P3). - Drop uppercase styling on the catalog Iceberg badge to match the wizard (P3).
…X-1330] - Remove the duplicate `.claude/worktrees/` line. - Drop the `@codemirror/*` overrides: each already resolves to a single version without them (bun install reports no change), so they were redundant. dompurify/prismjs/baseline-browser-mapping pins kept.
- Always render the Iceberg pill on catalog-tree table rows instead of only after the row is expanded (drop the isOpen gate on the topic config query; react-query caches each result). - Keep JSON/value cell popovers confined to the results grid: pass the rdg scroll viewport as the popover collisionBoundary and enable sticky so they no longer float off-screen (or to the page top) on scroll. - Fix the JSON viewer's nested double-scroll by forwarding viewportProps through SyncCodeBlock and using a single capped scroll container. UX-1330
Replace the sticky/boundary approach (which pinned the popup to the page top) with a clip overlay: cell popovers now portal into a transparent, overflow-hidden layer covering the grid's data area below the sticky header. With collisionAvoidance align disabled they track the anchor cell instead of repositioning, so on scroll they slide behind the header and stay confined to the table section. UX-1330
Numeric cell values keep right alignment (digits line up), but the column-name header now reads left-aligned for every kind instead of right-aligning for numeric columns. UX-1330
…1330] isSupported(Feature.SQLService) returns false both when SQLService is unsupported and when endpoint compatibility hasn't loaded yet. On a cold load of /sql the route's beforeLoad redirected to / before the capability list arrived; in the shared MF dev runtime that index match renders the host's (Cloud UI) login Landing inside Console's outlet, so the SQL page flashed a 'Log in to Redpanda Cloud' screen until the user navigated away and back. Gate the redirect on endpointCompatibility being loaded, and add a reactive guard in SqlWorkspace so a cluster that genuinely lacks SQLService still bounces once the answer is known.
…X-1330] - Center the "Redpanda SQL · Studio" header band evenly between the breadcrumb and the editor card in boxed mode - Start with a blank editor and show a "No tables yet" empty state in the results pane, with an admin CTA to add a topic to SQL - Auto-open the catalog (catalog=>) completion helper right after typing FROM/JOIN, without a manual Ctrl+Space - Derive the caller's role from the SQLService GetSqlIdentity endpoint instead of defaulting to viewer
6fae3d8 to
ae3466a
Compare
Summary
Adds a SQL query workspace to Console for querying Redpanda SQL: a catalog tree, SQL editor, results view, and an Iceberg bridge wizard.
Refs: UX-1330
Changes
frontend/src/components/pages/sql/module:sql-workspace,sql-editor,sql-results,catalog-tree,sql-wizard, sharedsql-types/sqlroute andreact-query/api/sqldata layerNotes
🤖 Generated with Claude Code