Drill-XXXX: Refactor User Interface#3037
Draft
cgivre wants to merge 215 commits into
Draft
Conversation
cgivre
changed the title
cgivre
added
enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
cgivre
force-pushed
the
feature/sqllab-react-ui
branch
from
March 9, 2026 13:45
080ba0b to
36af30f
Compare
cgivre
force-pushed
the
feature/sqllab-react-ui
branch
2 times, most recently
from
March 25, 2026 13:50
33fb68e to
34bfc23
Compare
cgivre
force-pushed
the
feature/sqllab-react-ui
branch
from
June 16, 2026 14:20
1b68007 to
bc4e985
Compare
cgivre
force-pushed
the
feature/sqllab-react-ui
branch
from
June 24, 2026 03:47
facc35c to
ff3d719
Compare
The two existing wiring tests in ProjectContextBlockTest only assert the absence of "PROJECT CONTEXT" and would pass unchanged even if the injection block in buildMessages were deleted. Make loadProject and loadSavedQueries package-private so a test can override them with fixtures via an anonymous ProspectorResources subclass, and add testLoadedProjectReachesSystemPrompt to prove a loaded project's description, tags, wiki title, and saved-query description actually reach the system prompt, that wiki bodies are excluded, and that loadSavedQueries receives the project's own savedQueryIds.
Wiki page bodies can be tens of KB, and the full system prompt is re-sent on every tool round, so only page titles are included in the project-context block. This adds a tool the model can call to fetch a specific page's full content (capped at 8000 chars) or list titles when called with no arguments.
Assert the exact error message instead of matching /project/i, which also matched the unrelated 'Unknown tool: get_project_docs' fallback string, letting the test pass even if the tool were deleted.
buildMessages accepted the six notebookMode/notebookDfName/... fields the notebook tab sends but never emitted them into the system prompt, so the model never actually saw notebook context. Added a notebookMode section (dataframe name, shape, columns, current cell code, last cell error) following the existing log/dashboard section style. Separately, the sendDataToAi privacy flag was honoured by the optimize path but ignored by the execute_sql tool (always returned 5 sample rows) and by appendDashboardData (always serialized sample rows). Added ChatContext.sendDataToAi (Boolean, so absent is distinguishable from false) and ProspectorResources.isSendDataToAi, and gated sample-row output on it in both places while keeping columns/types/rowCount either way. Threaded the same flag through the TS ChatContext type, useProspector's execute_sql case, and SqlLabPage's aiContext memo (reusing the sendDataToAi state the optimize path already reads).
RightInspector (hosting GlobalProspectorTab) renders above the router outlet, so ProjectContextProvider is not an ancestor and useProjectContext() would throw. Derive projectId from useMatch on '/projects/:id/*' instead, which matches both the bare project route and nested project pages.
The chat context's projectId is client-supplied, but loadProject read it straight
out of the store with no checks. Any authenticated user could name another user's
private project id and have its description, tags, wiki page titles and the full
SQL of its saved queries read back out of the system prompt.
Enforce the same authorization the REST read paths enforce, reusing their
predicates rather than copying them — a divergent copy is how this comes back:
- ProjectResources.canRead becomes package-private static; loadProject applies it
plus the deletedAt guard, matching GET /api/v1/projects/{id}.
- SavedQueryResources grows the canRead it previously inlined at two call sites;
loadSavedQueries applies it per query, since a readable project can reference
another user's private saved query.
An unauthorized or soft-deleted project behaves exactly like a missing one: no
block, no error, chat proceeds. Unauthorized is indistinguishable from missing so
chat cannot be used to probe which project ids exist.
Also gate dashboard sample rows on the server-side LlmConfig.sendDataToAi rather
than a client-supplied ChatContext flag. The flag was only ever set by SqlLabPage,
so the gate was dead code for the dashboard panels that actually reach it. Reading
the authoritative config makes every server path correct by construction and stops
trusting a client boolean; ChatContext.sendDataToAi and isSendDataToAi are deleted.
Mirror sendDataToAi onto the /status response: it is readable by every
authenticated user, whereas /api/v1/ai/config is admin-only, so a non-admin's
browser otherwise has no way to learn a setting it is expected to honour.
Finally, guard the project name, saved-query name and wiki title against null so a
nameless project no longer renders "Project: null" into the prompt, and drop the
redundant projectId null/blank check that the sole caller already performs.
…orget it The execute_sql row gate read sendDataToAi off the ChatContext, which meant every caller had to remember to set it. Two did not: the global Prospector tab and the dashboard panels passed no flag, so execute_sql returned five sample rows with the setting off. It was a client boolean standing in for server config. Read it inside the hook instead, from getAiStatus() on mount. That gates all three callers by construction and matches the server, which now reads the same value from LlmConfig. Read from /status rather than /config because config is admin-only: a non-admin's fetch 403s and falls back to permissive, which is the same bug again. This is why the flag is not a hook parameter — a parameter is another thing a caller can omit, and omission is what shipped the leak. Unknown (still loading, or the fetch failed) withholds rows: a privacy setting has to fail closed. SqlLabPage's own sendDataToAi state likewise moves to /status and now starts false; it previously defaulted true and silently stayed true whenever the admin-only config fetch failed, so the optimize path leaked sample data for every non-admin user. Columns and rowCount are metadata and are still always sent. Correct prospector.md, which claimed the flag suppressed dashboard sampleRows — true only for a path no caller exercised. Note the remaining gap: AiQnAPanel and ExecutiveSummaryPanel inline sample rows into the user message, which no server gate can suppress.
Extract the sendDataToAi privacy-flag read (default withhold, fail closed on error, sourced from the non-admin-readable /status endpoint) out of useProspector into a new useSendDataToAi hook, and point SqlLabPage at the same hook instead of its own duplicated useState + getAiStatus effect. This removes the duplication and adds direct test coverage for the exact fail-open regression that was previously fixed with no test pinning it. Also correct a doc claim in prospector.md that overstated dashboard gating: appendDashboardData only gates the server-side system-prompt path, not the user-message sample-row inlining in AiQnAPanel and ExecutiveSummaryPanel (already documented separately as a known gap).
Design for two new destinations in the SQL Lab save dialog: Drill views and materialized views. Drill already has the DDL (DRILL-8543 for materialized views) and /query.json executes arbitrary SQL, so the work is a schema-eligibility flag, a description store, and UI. Records the non-obvious constraints found while designing: - View creation needs a file-based plugin AND a writable workspace. IS_MUTABLE alone is insufficient: Splunk, Kudu, JDBC and GoogleSheets report mutable but reject views. - The SELECT guard checks only the leading keyword. The grammar makes that sufficient, and scanning the body would reject valid views such as SELECT 'INSERT' AS action FROM t. - View names are paths, not identifiers, so slashes are allowed. Views in subdirectories are queryable but invisible to INFORMATION_SCHEMA, since TABLES, VIEWS and MATERIALIZED_VIEWS all read the same non-recursive glob.
The approved supportsViews flag on SchemaInfo does not work: getSchemas returns root plugin names only and returns early, so workspaces such as dfs.tmp never appear there. They come from getPluginSchemas, one plugin per call, which would force the save dialog into N+1 requests to fill a dropdown. Replace it with GET /api/v1/metadata/view-targets, running one SCHEMATA query and filtering to file-based plugins via the registry. Same authoritative backend rule, one round trip, and no flag threaded through endpoints that have no use for it.
Covers phases 1-2 of the design spec: the view-targets endpoint, the save dialog's view modes, the DDL builder and SELECT guard, tree icons, and refreshing the tree after a create. Descriptions, materialized view refresh and nested-view rendering follow in their own plans, written against real code rather than anticipated code.
Pre-flight review found the plan settling for manual verification where the codebase already supports automated tests: TreeNodeBuilder.test.tsx exists and buildTableNodes is pure, and AddToProjectModal.test.tsx shows modal testing is established. Add failing icon assertions to Task 6, and extract Task 5's orchestration into utils/createView.ts so the partial-failure rule -- a view survives a project-link failure -- is covered by a real test rather than by driving antd's Select through RTL.
handleSave no longer falls through to createSavedQuery when the "Save as" mode is view/materialized_view, which previously saved a plain, mislabeled query and discarded the schema/replace inputs. Also reset mode back to 'query' in the save mutation's onSuccess handler so the permanently-mounted dialog doesn't reopen in a stale view mode after a successful save.
…ew-mode guard The test selected the View radio but never filled the required schema and name fields, so form.validateFields() rejected before handleSave ever reached the isViewMode guard. That let createSavedQuery go uncalled regardless of whether the guard existed, so the test passed against broken code too. Now the test fills the view Name input and picks a mocked schema from the Select, ensuring the assertion depends on the guard actually short-circuiting the save.
Add createViewFromQuery to run the CREATE VIEW/MATERIALIZED VIEW DDL and link the result to the active project, reporting a partial failure rather than throwing if only the project-link step fails. Wire it into SaveQueryDialog's handleSave, replacing the placeholder guard that no-opped view mode. Give ProjectDetailPage real labels for view and materialized_view datasets instead of falling through to "Table".
…ring and labels The project dataset allow-list only allow-listed type 'table', so a view or materialized view created inside a project was silently stripped from the schema tree instead of appearing alongside regular tables. The data sources page also mislabeled both new types as plain 'Table'. Widen both ternary/ condition chains to treat 'view' and 'materialized_view' the same as their sibling handling elsewhere (e.g. ProjectDetailPage).
Extract the datasetAllowList useMemo body in SchemaExplorer into a pure buildDatasetAllowList() function so the fix that treats 'view' and 'materialized_view' datasets like 'table' datasets (otherwise a newly created view silently drops out of the project-scoped schema tree) can be unit tested without rendering the component.
The Bug 1 test drove an antd Select through userEvent, whose click fires a
full pointer sequence (pointerover/down/focus/up/click) each wrapped in
act() with microtask flushes. On a Modal+Select that re-renders per event
this compounded to ~4s in isolation and crossed the 5s per-test timeout
under the full suite's parallel CPU load — it passed only when run alone,
so a single local run reported green while CI failed.
Drive both tests with synchronous fireEvent instead, which exercises the
same handlers without the pointer-sequence overhead: Bug 1 drops to
~2.5-3.5s. Both tests stay non-vacuous, mutation-verified — breaking the
isViewMode branch fails Bug 1, removing setMode('query') from onSuccess
fails Bug 2. A 10s per-test timeout adds headroom for slower CI hardware,
not to mask a hang.
Also fix the getComputedStyle test shim, which passed the pseudo-element
argument straight through and so suppressed nothing; jsdom logged a "Not
implemented" error on every antd popup alignment, flooding CI output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DRILL-XXXX: Refactor User Interface
Description
This PR refactors Drill's UI and refactors the Query view, adds visualizations and dashboards and in general makes Drill much more user friendly.
Documentation
(Please describe user-visible changes similar to what should appear in the Drill documentation.)
Testing
(Please describe how this PR has been tested.)