Rework query workspace explorer and history privacy#16
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a metadata explorer to the Queries UI, centralizes primary navigation active-state helpers, enhances the SQL editor (height + programmatic insert + Mod‑Enter submit), and restricts QueryLog access/updates to the requesting username. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Explorer as QueryMetadataExplorer
participant API as Backend API
participant Editor as SqlCodeEditor
User->>Explorer: select instance / expand node
Explorer->>API: GET /metadata (instanceId, nodeId)
API-->>Explorer: return nodes / table metadata (DDL / rows)
Explorer-->>User: render tree and details
User->>Explorer: double-click table (insert)
Explorer->>Editor: insertText(qualified_table_name)
Editor->>Editor: insert at cursor / update model
Editor-->>User: updated editor content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
Contributor
|
Note Docstrings generation - SUCCESS |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba08733e93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not (user.is_superuser or user.has_perm("sql.audit_user")): | ||
| filter_dict["username"] = user.username | ||
| # The normal query-history screen is always scoped to the current user. | ||
| filter_dict["username"] = user.username |
There was a problem hiding this comment.
This helper now always applies filter_dict["username"] = user.username, but querylog_audit still delegates to the same _querylog function and the legacy audit page continues to call /query/querylog_audit/ (sql/templates/audit_sqlquery.html). That means users with sql.audit_user can no longer see other users’ query history from the audit flow, which breaks the core audit use case rather than just tightening normal history privacy.
Useful? React with 👍 / 👎.
Comment on lines
+66
to
+69
| key: 'Mod-Enter', | ||
| run: () => { | ||
| emit('submit') | ||
| return true |
There was a problem hiding this comment.
The new Mod-Enter binding emits submit unconditionally, and in QueriesView that event calls runQuery('query') directly while runQuery itself has no early queryRunning guard. A user can press the shortcut repeatedly before the first request completes, triggering concurrent duplicate executions even though the Run button is visually disabled, which can create duplicate logs and unnecessary database load.
Useful? React with 👍 / 👎.
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 11, 2026
Docstrings generation was requested by @jruszo. * #16 (comment) The following files were modified: * `sql/query.py` * `sql_api/api_query.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
frontend/src/views/QueriesView.vue (2)
1009-1033:⚠️ Potential issue | 🔴 CriticalMirror the button guards inside
runQuery().The new editor shortcut calls this function directly, so the disabled buttons are no longer the only gate. Right now
Mod-Entercan bypass!canRunQueries, and repeated presses whilequeryRunningis already true can send duplicate non-idempotent queries.💡 Proposed fix
async function runQuery(mode: 'query' | 'plan') { + if (!canRunQueries.value) { + pushToast('Your account does not have permission to execute queries.', 'error') + return + } + if (queryRunning.value) { + return + } + const selectedSql = selectedQueryText() const instance = selectedInstance.value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/QueriesView.vue` around lines 1009 - 1033, In runQuery add the same guard checks that the UI buttons enforce: check canRunQueries.value at the start and if false push a toast and return, and check queryRunning.value and if true either push a toast or return to prevent duplicate submissions; place these checks (using the existing symbols canRunQueries and queryRunning) before building/executing the SQL so the keyboard shortcut cannot bypass the disabled-button logic.
45-56:⚠️ Potential issue | 🔴 CriticalRestore the full connection context when activating a runtime tab.
activateWorkspaceTab()only reappliessqlCache, but execution still readsselectedInstanceandform.dbName/schemaName/tableName. After working in tab B, switching back to tab A can run A's SQL against B's connection, which is unsafe for mutating statements and also makes the tab metadata misleading.💡 Proposed direction
type QueryTab = { id: string title: string kind: 'result' | 'describe' payload: QueryResultPayload | QueryDescribePayload | null error: string + instanceId: number instanceName: string dbName: string + schemaName: string dbType: string sqlCache: string tableName: string } -function activateWorkspaceTab(tabId: string) { +async function activateWorkspaceTab(tabId: string) { activeTab.value = tabId const tab = queryTabs.value.find((item) => item.id === tabId) if (tab) { + if (tab.instanceId && tab.instanceId !== form.instanceId) { + await selectInstance(tab.instanceId) + } + form.dbName = tab.dbName + form.schemaName = tab.schemaName + form.tableName = tab.tableName applyEditorValue(tab.sqlCache) } }Also applies to: 312-319, 1036-1042
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/QueriesView.vue` around lines 45 - 56, The activateWorkspaceTab() handler currently only reapplies sqlCache and must also restore the tab's full connection context: when activating a tab (e.g., QueryTab objects with fields instanceName, dbName, dbType, tableName), set the global/parent selection state (selectedInstance) and the form fields (form.dbName, form.schemaName if used, form.tableName) and any dbType/instanceName state the UI relies on so subsequent execution runs against that tab's connection; update activateWorkspaceTab and the analogous handlers referenced around the other occurrences (lines ~312-319 and ~1036-1042) to copy these QueryTab fields back into the shared selection/form state whenever a runtime tab is activated.
🧹 Nitpick comments (1)
frontend/src/App.vue (1)
200-200: Consider consistent key strategy across navigation lists.Primary navigation now uses
item.labelas the key, while settings navigation (line 232) continues to useitem.to. Both work since values are unique, butitem.tois more conventional for route-based navigation since route paths are inherently unique identifiers.♻️ Suggested fix for consistency
- :key="item.label" + :key="item.to"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.vue` at line 200, Primary navigation is using :key="item.label" while settings navigation uses :key="item.to"; make keys consistent by switching the primary navigation v-for key to use the route path (item.to) so both nav lists use the same route-based identifier—update the :key on the primary nav iteration (where :key="item.label") to :key="item.to" and ensure every nav item has a unique item.to value before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/QueriesView.vue`:
- Around line 583-595: The loop over rows in QueriesView.vue currently pushes
one explorer entry per row which duplicates multi-column indexes; instead
aggregate rows by the index identifier (use keyName computed from
row.Key_name/row.key_name/etc.) into a temporary map keyed by keyName, collect
each column with its sequence (seq from
Seq_in_index/seq_in_index/COLUMN_POSITION/ordinal_position) and type (from
Non_unique/Index_type fallbacks), then after the loop convert that map into the
indexes array by sorting each index's columns by seq and joining them into a
single ordered columns string; update the code that currently references
keyName, seq, and indexes.push to build the map and then produce the final
indexes entries so composite indexes become one entry with ordered columns and a
single type/name.
- Around line 968-986: The showTableStructure flow can fire duplicate
describeQueryTable requests and stale responses can overwrite
tableStructureByNode after instance changes; to fix, add a per-node in-flight
request map (keyed by node.id) and skip launching a new describeQueryTable if an
identical request is pending, and attach a generation or instance check (compare
selectedInstance.value.id or a selectionGeneration token updated by
selectInstance()) before committing the payload to tableStructureByNode so late
responses are ignored; update the logic in showTableStructure to consult the
in-flight map, populate it when calling describeQueryTable, and clear it on
completion or error, and ensure selectInstance() increments/updates the
generation token that showTableStructure checks.
In `@sql_api/api_query.py`:
- Around line 398-407: Black formatting failed for the conditional block
handling queryset filtering (variables: queryset, star, query_log_id, and
attribute self.audit_only) — reformat the code using Black (run black
sql_api/api_query.py) or apply equivalent formatting so the if/elif/if chains
and spacing conform to Black style (ensure consistent indentation and blank
lines around the block that includes the final permission check referencing
user.is_superuser and user.has_perm("sql.audit_user")).
- Around line 406-407: The if-block guarding queryset by username is dead code
because _check_permission() already enforces audit_only and sql.audit_user, so
remove the conditional and its body (the lines referencing self.audit_only,
user.is_superuser, user.has_perm("sql.audit_user"), and
queryset.filter(username=user.username)) from the method; rely on the existing
_check_permission() behavior instead and delete the redundant logic to simplify
the method.
---
Outside diff comments:
In `@frontend/src/views/QueriesView.vue`:
- Around line 1009-1033: In runQuery add the same guard checks that the UI
buttons enforce: check canRunQueries.value at the start and if false push a
toast and return, and check queryRunning.value and if true either push a toast
or return to prevent duplicate submissions; place these checks (using the
existing symbols canRunQueries and queryRunning) before building/executing the
SQL so the keyboard shortcut cannot bypass the disabled-button logic.
- Around line 45-56: The activateWorkspaceTab() handler currently only reapplies
sqlCache and must also restore the tab's full connection context: when
activating a tab (e.g., QueryTab objects with fields instanceName, dbName,
dbType, tableName), set the global/parent selection state (selectedInstance) and
the form fields (form.dbName, form.schemaName if used, form.tableName) and any
dbType/instanceName state the UI relies on so subsequent execution runs against
that tab's connection; update activateWorkspaceTab and the analogous handlers
referenced around the other occurrences (lines ~312-319 and ~1036-1042) to copy
these QueryTab fields back into the shared selection/form state whenever a
runtime tab is activated.
---
Nitpick comments:
In `@frontend/src/App.vue`:
- Line 200: Primary navigation is using :key="item.label" while settings
navigation uses :key="item.to"; make keys consistent by switching the primary
navigation v-for key to use the route path (item.to) so both nav lists use the
same route-based identifier—update the :key on the primary nav iteration (where
:key="item.label") to :key="item.to" and ensure every nav item has a unique
item.to value before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d337f36-a2a7-4e43-afe4-caf4bd627aa0
📒 Files selected for processing (8)
frontend/src/App.vuefrontend/src/components/queries/QueryMetadataExplorer.vuefrontend/src/components/queries/SqlCodeEditor.vuefrontend/src/components/queries/query-metadata-explorer.tsfrontend/src/views/QueriesView.vuesql/query.pysql_api/api_query.pysql_api/tests.py
Comment on lines
+583
to
+595
| for (const row of rows) { | ||
| const keyName = String(row.Key_name ?? row.key_name ?? row.index_name ?? row.INDEX_NAME ?? '') | ||
| const seq = row.Seq_in_index ?? row.seq_in_index ?? row.COLUMN_POSITION ?? row.ordinal_position | ||
| if (!keyName && seq === undefined) { | ||
| continue | ||
| } | ||
|
|
||
| indexes.push({ | ||
| name: keyName || 'INDEX', | ||
| type: String(row.Non_unique === 0 || row.non_unique === 0 ? 'UNIQUE' : row.Index_type ?? row.index_type ?? 'INDEX'), | ||
| columns: String(row.Column_name ?? row.column_name ?? row.COLUMN_NAME ?? ''), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Aggregate composite indexes before building explorer metadata.
Many describe/index endpoints return one row per indexed column. This loop ignores Seq_in_index / ordinal_position and emits one explorer entry per row, so multi-column indexes will show up as duplicates instead of a single ordered column list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 583 - 595, The loop over
rows in QueriesView.vue currently pushes one explorer entry per row which
duplicates multi-column indexes; instead aggregate rows by the index identifier
(use keyName computed from row.Key_name/row.key_name/etc.) into a temporary map
keyed by keyName, collect each column with its sequence (seq from
Seq_in_index/seq_in_index/COLUMN_POSITION/ordinal_position) and type (from
Non_unique/Index_type fallbacks), then after the loop convert that map into the
indexes array by sorting each index's columns by seq and joining them into a
single ordered columns string; update the code that currently references
keyName, seq, and indexes.push to build the map and then produce the final
indexes entries so composite indexes become one entry with ordered columns and a
single type/name.
Comment on lines
+968
to
+986
| async function showTableStructure(node: QueryMetadataNode) { | ||
| if (!selectedInstance.value || node.kind !== 'table') { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| const payload = await describeQueryTable( | ||
| { | ||
| instance_id: selectedInstance.value.id, | ||
| db_name: form.dbName, | ||
| schema_name: needsSchemaSelection.value ? form.schemaName : undefined, | ||
| tb_name: form.tableName, | ||
| db_name: node.dbName, | ||
| schema_name: needsSchemaSelection.value ? node.schemaName : undefined, | ||
| tb_name: node.name, | ||
| }, | ||
| requireToken(), | ||
| ) | ||
| upsertDescribeTab(payload) | ||
| tableStructureByNode.value = { | ||
| ...tableStructureByNode.value, | ||
| [node.id]: payload, | ||
| } |
Contributor
There was a problem hiding this comment.
Deduplicate and generation-guard table structure requests.
This path is hit from expand, select, and insert flows, and it always commits the response. Double-clicking a table can queue several describeQueryTable() calls, and a slow response from the previous explorer generation can repopulate tableStructureByNode after selectInstance() already cleared it.
💡 Proposed fix
+const loadingTableStructure = new Set<string>()
+
async function showTableStructure(node: QueryMetadataNode) {
if (!selectedInstance.value || node.kind !== 'table') {
return
}
+ if (tableStructureByNode.value[node.id] || loadingTableStructure.has(node.id)) {
+ return
+ }
+
+ const generation = explorerGeneration.value
+ const instanceId = selectedInstance.value.id
+ loadingTableStructure.add(node.id)
try {
const payload = await describeQueryTable(
{
instance_id: selectedInstance.value.id,
db_name: node.dbName,
schema_name: needsSchemaSelection.value ? node.schemaName : undefined,
tb_name: node.name,
},
requireToken(),
)
+ if (generation !== explorerGeneration.value || selectedInstance.value?.id !== instanceId) {
+ return
+ }
tableStructureByNode.value = {
...tableStructureByNode.value,
[node.id]: payload,
}
} catch (error) {
pushToast(toUserFacingMessage(error, 'Failed to load the table structure.'), 'error')
+ } finally {
+ loadingTableStructure.delete(node.id)
}
}📝 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.
Suggested change
| async function showTableStructure(node: QueryMetadataNode) { | |
| if (!selectedInstance.value || node.kind !== 'table') { | |
| return | |
| } | |
| try { | |
| const payload = await describeQueryTable( | |
| { | |
| instance_id: selectedInstance.value.id, | |
| db_name: form.dbName, | |
| schema_name: needsSchemaSelection.value ? form.schemaName : undefined, | |
| tb_name: form.tableName, | |
| db_name: node.dbName, | |
| schema_name: needsSchemaSelection.value ? node.schemaName : undefined, | |
| tb_name: node.name, | |
| }, | |
| requireToken(), | |
| ) | |
| upsertDescribeTab(payload) | |
| tableStructureByNode.value = { | |
| ...tableStructureByNode.value, | |
| [node.id]: payload, | |
| } | |
| const loadingTableStructure = new Set<string>() | |
| async function showTableStructure(node: QueryMetadataNode) { | |
| if (!selectedInstance.value || node.kind !== 'table') { | |
| return | |
| } | |
| if (tableStructureByNode.value[node.id] || loadingTableStructure.has(node.id)) { | |
| return | |
| } | |
| const generation = explorerGeneration.value | |
| const instanceId = selectedInstance.value.id | |
| loadingTableStructure.add(node.id) | |
| try { | |
| const payload = await describeQueryTable( | |
| { | |
| instance_id: selectedInstance.value.id, | |
| db_name: node.dbName, | |
| schema_name: needsSchemaSelection.value ? node.schemaName : undefined, | |
| tb_name: node.name, | |
| }, | |
| requireToken(), | |
| ) | |
| if (generation !== explorerGeneration.value || selectedInstance.value?.id !== instanceId) { | |
| return | |
| } | |
| tableStructureByNode.value = { | |
| ...tableStructureByNode.value, | |
| [node.id]: payload, | |
| } | |
| } catch (error) { | |
| pushToast(toUserFacingMessage(error, 'Failed to load the table structure.'), 'error') | |
| } finally { | |
| loadingTableStructure.delete(node.id) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 968 - 986, The
showTableStructure flow can fire duplicate describeQueryTable requests and stale
responses can overwrite tableStructureByNode after instance changes; to fix, add
a per-node in-flight request map (keyed by node.id) and skip launching a new
describeQueryTable if an identical request is pending, and attach a generation
or instance check (compare selectedInstance.value.id or a selectionGeneration
token updated by selectInstance()) before committing the payload to
tableStructureByNode so late responses are ignored; update the logic in
showTableStructure to consult the in-flight map, populate it when calling
describeQueryTable, and clear it on completion or error, and ensure
selectInstance() increments/updates the generation token that showTableStructure
checks.
Comment on lines
406
to
407
| if self.audit_only and not (user.is_superuser or user.has_perm("sql.audit_user")): | ||
| queryset = queryset.filter(username=user.username) |
Contributor
There was a problem hiding this comment.
Dead code: condition can never be True.
The condition self.audit_only and not (user.is_superuser or user.has_perm("sql.audit_user")) can never evaluate to True at this point.
When audit_only=True, _check_permission() already requires sql.audit_user permission:
- Superusers pass → condition is False (because
user.is_superuseris True) - Users with
audit_userperm pass → condition is False (becauseuser.has_perm(...)is True) - Users lacking both → rejected by
_check_permission()before reaching this line
This code adds complexity without effect. Consider removing it.
Proposed fix
if not self.audit_only:
queryset = queryset.filter(username=user.username)
if star == "true":
queryset = queryset.filter(favorite=True)
elif star == "false":
queryset = queryset.filter(favorite=False)
if query_log_id:
queryset = queryset.filter(id=query_log_id)
- if self.audit_only and not (user.is_superuser or user.has_perm("sql.audit_user")):
- queryset = queryset.filter(username=user.username)
if start_date and end_date:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql_api/api_query.py` around lines 406 - 407, The if-block guarding queryset
by username is dead code because _check_permission() already enforces audit_only
and sql.audit_user, so remove the conditional and its body (the lines
referencing self.audit_only, user.is_superuser, user.has_perm("sql.audit_user"),
and queryset.filter(username=user.username)) from the method; rely on the
existing _check_permission() behavior instead and delete the redundant logic to
simplify the method.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/QueriesView.vue`:
- Around line 1522-1525: The template is showing schema from form.schemaName
instead of the tab's data; update the span to use currentTab.schemaName (and
change the v-if to v-if="currentTab.schemaName") so the displayed schema matches
the tab's query result. Locate the snippet in QueriesView.vue where
currentTab.instanceName and currentTab.dbName are rendered and replace
references to form.schemaName with currentTab.schemaName to maintain consistency
with currentTab.
- Around line 254-277: startEditorResize can overwrite removeResizeListeners and
orphan prior listeners if called again; call stopEditorResize() at the start of
startEditorResize to ensure any existing listeners are removed before adding new
ones. Modify the startEditorResize function to invoke stopEditorResize()
immediately (before setting cursor/userSelect and adding new handlers), so
existing handlers (handlePointerMove/handlePointerUp) and removeResizeListeners
are cleaned up; keep the existing removeResizeListeners assignment and the once:
true mouseup listener behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56fd241e-03d4-4203-968f-a1ddb86d98c4
📒 Files selected for processing (2)
frontend/src/App.vuefrontend/src/views/QueriesView.vue
Comment on lines
+254
to
+277
| function startEditorResize(event: MouseEvent) { | ||
| const startY = event.clientY | ||
| const startHeight = editorPaneHeight.value | ||
|
|
||
| document.body.style.cursor = 'row-resize' | ||
| document.body.style.userSelect = 'none' | ||
|
|
||
| const handlePointerMove = (moveEvent: MouseEvent) => { | ||
| const delta = moveEvent.clientY - startY | ||
| editorPaneHeight.value = clampEditorPaneHeight(startHeight + delta) | ||
| } | ||
|
|
||
| const handlePointerUp = () => { | ||
| stopEditorResize() | ||
| } | ||
|
|
||
| window.addEventListener('mousemove', handlePointerMove) | ||
| window.addEventListener('mouseup', handlePointerUp, { once: true }) | ||
|
|
||
| removeResizeListeners = () => { | ||
| window.removeEventListener('mousemove', handlePointerMove) | ||
| window.removeEventListener('mouseup', handlePointerUp) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Call stopEditorResize() at the start to prevent orphaned listeners.
If startEditorResize is invoked twice before a mouseup fires (e.g., rapid clicks or touch events), the previous handlePointerMove listener becomes orphaned because removeResizeListeners is overwritten. Adding a cleanup call at the start ensures any prior listeners are removed first.
🛡️ Proposed fix
function startEditorResize(event: MouseEvent) {
+ stopEditorResize()
+
const startY = event.clientY
const startHeight = editorPaneHeight.value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 254 - 277, startEditorResize
can overwrite removeResizeListeners and orphan prior listeners if called again;
call stopEditorResize() at the start of startEditorResize to ensure any existing
listeners are removed before adding new ones. Modify the startEditorResize
function to invoke stopEditorResize() immediately (before setting
cursor/userSelect and adding new handlers), so existing handlers
(handlePointerMove/handlePointerUp) and removeResizeListeners are cleaned up;
keep the existing removeResizeListeners assignment and the once: true mouseup
listener behavior.
Comment on lines
+1522
to
+1525
| <p class="mt-1 text-xs text-slate-500"> | ||
| {{ currentTab.instanceName || 'No instance' }} / {{ currentTab.dbName || 'No database' }} | ||
| <span v-if="form.schemaName"> / {{ form.schemaName }}</span> | ||
| </p> |
Contributor
There was a problem hiding this comment.
Use currentTab.schemaName for consistency.
The instance and database are read from currentTab, but the schema is read from form.schemaName. This displays the currently selected form value rather than the schema associated with the tab's query result.
🔧 Proposed fix
<p class="mt-1 text-xs text-slate-500">
{{ currentTab.instanceName || 'No instance' }} / {{ currentTab.dbName || 'No database' }}
- <span v-if="form.schemaName"> / {{ form.schemaName }}</span>
+ <span v-if="currentTab.schemaName"> / {{ currentTab.schemaName }}</span>
</p>📝 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.
Suggested change
| <p class="mt-1 text-xs text-slate-500"> | |
| {{ currentTab.instanceName || 'No instance' }} / {{ currentTab.dbName || 'No database' }} | |
| <span v-if="form.schemaName"> / {{ form.schemaName }}</span> | |
| </p> | |
| <p class="mt-1 text-xs text-slate-500"> | |
| {{ currentTab.instanceName || 'No instance' }} / {{ currentTab.dbName || 'No database' }} | |
| <span v-if="currentTab.schemaName"> / {{ currentTab.schemaName }}</span> | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 1522 - 1525, The template is
showing schema from form.schemaName instead of the tab's data; update the span
to use currentTab.schemaName (and change the v-if to
v-if="currentTab.schemaName") so the displayed schema matches the tab's query
result. Locate the snippet in QueriesView.vue where currentTab.instanceName and
currentTab.dbName are rendered and replace references to form.schemaName with
currentTab.schemaName to maintain consistency with currentTab.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/views/QueriesView.vue (1)
1057-1083:⚠️ Potential issue | 🟠 MajorKeep the tab's editor cache on the original SQL for execution plans.
When
mode === 'plan',sqlToRunis the generatedEXPLAIN ...statement, and that same value is persisted intotab.sqlCache. Reopening the tab then rewrites the editor with the wrapper, so a later "Run Query" reruns the EXPLAIN instead of the user's SQL.💡 Proposed fix
const sqlToRun = mode === 'plan' ? buildExplainSql(selectedDbType.value, selectedSql) : selectedSql @@ - upsertResultTab(payload, sqlToRun) + upsertResultTab(payload, selectedSql) @@ - setResultError(message, sqlToRun) + setResultError(message, selectedSql)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/views/QueriesView.vue` around lines 1057 - 1083, The code persists the wrapper EXPLAIN SQL (sqlToRun) into the tab cache when mode === 'plan', causing future runs to re-run the EXPLAIN instead of the user's original SQL; change the call to upsertResultTab so that when mode === 'plan' you pass the original selectedSql (not sqlToRun) as the editor cache (or update upsertResultTab to accept an explicit rawSql parameter) so tab.sqlCache is set to selectedSql while still sending sqlToRun to executeQuery; use the variables sqlToRun, selectedSql, mode and the upsertResultTab call to implement this conditional behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/views/QueriesView.vue`:
- Around line 1297-1303: The icon-only tab buttons lack accessible names; update
the two Button elements that call createResultTab() and removeActiveTab to
provide accessible labels (e.g., add aria-label attributes like aria-label="Add
result tab" and aria-label="Remove active tab" or include visually hidden text
inside the Button) so screen readers can announce their purpose; target the
Button components wrapping the Plus and Minus icons in the QueriesView.vue
template and ensure labels are concise and descriptive.
- Around line 1470-1480: The icon-only history action buttons lack accessible
labels and state; update the Button elements around the RefreshCcw and Star
icons to include descriptive aria-label attributes (e.g., aria-label="Rerun
query" on the button that calls rerunHistoryItem(item)) and make the favorite
toggle expose its state by adding aria-pressed bound to item.favorite (on the
Button that calls toggleFavorite(item)); ensure these attributes are added where
the Button components are rendered so rerunHistoryItem and toggleFavorite remain
the click handlers.
- Around line 314-330: activateWorkspaceTab can overwrite form state and the
editor when an awaited selectInstance completes after the user has switched
tabs; capture the intended tab id (e.g. const current = tabId or compare
activeTab.value) before awaiting selectInstance and after the await verify the
tab is still the same tab (and that tab still exists) before mutating
form.dbName, form.schemaName, form.tableName or calling applyEditorValue; also
guard the branch that sets form.instanceId so it only runs if the activation is
still valid. Use the existing symbols activateWorkspaceTab, activeTab,
selectInstance, form, applyEditorValue and the found tab/instance variables to
perform the pre/post-await check.
---
Outside diff comments:
In `@frontend/src/views/QueriesView.vue`:
- Around line 1057-1083: The code persists the wrapper EXPLAIN SQL (sqlToRun)
into the tab cache when mode === 'plan', causing future runs to re-run the
EXPLAIN instead of the user's original SQL; change the call to upsertResultTab
so that when mode === 'plan' you pass the original selectedSql (not sqlToRun) as
the editor cache (or update upsertResultTab to accept an explicit rawSql
parameter) so tab.sqlCache is set to selectedSql while still sending sqlToRun to
executeQuery; use the variables sqlToRun, selectedSql, mode and the
upsertResultTab call to implement this conditional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa023ceb-1fd2-42d1-ae88-ea2fbd46522e
📒 Files selected for processing (1)
frontend/src/views/QueriesView.vue
Comment on lines
+314
to
+330
| async function activateWorkspaceTab(tabId: string) { | ||
| activeTab.value = tabId | ||
|
|
||
| const tab = queryTabs.value.find((item) => item.id === tabId) | ||
| if (tab) { | ||
| const instance = instances.value.find((item) => item.instance_name === tab.instanceName) | ||
| if (instance && form.instanceId !== instance.id) { | ||
| await selectInstance(instance.id) | ||
| } else if (instance) { | ||
| form.instanceId = instance.id | ||
| } | ||
|
|
||
| form.dbName = tab.dbName | ||
| form.schemaName = tab.schemaName | ||
| form.tableName = tab.tableName | ||
| applyEditorValue(tab.sqlCache) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard tab activation against stale async completions.
Line 321 awaits selectInstance(), but Lines 326-329 still run even if the user has already switched to another tab. A slower earlier activation can therefore overwrite form.* and the editor for the newly active tab.
💡 Proposed fix
+let workspaceActivationToken = 0
+
async function activateWorkspaceTab(tabId: string) {
activeTab.value = tabId
+ const activationToken = ++workspaceActivationToken
const tab = queryTabs.value.find((item) => item.id === tabId)
if (tab) {
const instance = instances.value.find((item) => item.instance_name === tab.instanceName)
if (instance && form.instanceId !== instance.id) {
await selectInstance(instance.id)
+ if (workspaceActivationToken !== activationToken || activeTab.value !== tabId) {
+ return
+ }
} else if (instance) {
form.instanceId = instance.id
}
+ if (workspaceActivationToken !== activationToken || activeTab.value !== tabId) {
+ return
+ }
+
form.dbName = tab.dbName
form.schemaName = tab.schemaName
form.tableName = tab.tableName
applyEditorValue(tab.sqlCache)
}
}📝 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.
Suggested change
| async function activateWorkspaceTab(tabId: string) { | |
| activeTab.value = tabId | |
| const tab = queryTabs.value.find((item) => item.id === tabId) | |
| if (tab) { | |
| const instance = instances.value.find((item) => item.instance_name === tab.instanceName) | |
| if (instance && form.instanceId !== instance.id) { | |
| await selectInstance(instance.id) | |
| } else if (instance) { | |
| form.instanceId = instance.id | |
| } | |
| form.dbName = tab.dbName | |
| form.schemaName = tab.schemaName | |
| form.tableName = tab.tableName | |
| applyEditorValue(tab.sqlCache) | |
| } | |
| let workspaceActivationToken = 0 | |
| async function activateWorkspaceTab(tabId: string) { | |
| activeTab.value = tabId | |
| const activationToken = ++workspaceActivationToken | |
| const tab = queryTabs.value.find((item) => item.id === tabId) | |
| if (tab) { | |
| const instance = instances.value.find((item) => item.instance_name === tab.instanceName) | |
| if (instance && form.instanceId !== instance.id) { | |
| await selectInstance(instance.id) | |
| if (workspaceActivationToken !== activationToken || activeTab.value !== tabId) { | |
| return | |
| } | |
| } else if (instance) { | |
| form.instanceId = instance.id | |
| } | |
| if (workspaceActivationToken !== activationToken || activeTab.value !== tabId) { | |
| return | |
| } | |
| form.dbName = tab.dbName | |
| form.schemaName = tab.schemaName | |
| form.tableName = tab.tableName | |
| applyEditorValue(tab.sqlCache) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 314 - 330,
activateWorkspaceTab can overwrite form state and the editor when an awaited
selectInstance completes after the user has switched tabs; capture the intended
tab id (e.g. const current = tabId or compare activeTab.value) before awaiting
selectInstance and after the await verify the tab is still the same tab (and
that tab still exists) before mutating form.dbName, form.schemaName,
form.tableName or calling applyEditorValue; also guard the branch that sets
form.instanceId so it only runs if the activation is still valid. Use the
existing symbols activateWorkspaceTab, activeTab, selectInstance, form,
applyEditorValue and the found tab/instance variables to perform the
pre/post-await check.
Comment on lines
+1297
to
+1303
| <div class="flex items-center gap-2"> | ||
| <Button variant="outline" size="sm" @click="createResultTab()"> | ||
| <Plus class="h-4 w-4" /> | ||
| </Button> | ||
| <Button variant="outline" size="sm" @click="removeActiveTab"> | ||
| <Minus class="h-4 w-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add accessible names to the icon-only tab controls.
These buttons render only icons, so their purpose is not announced to screen-reader users. Please add an aria-label or sr-only text for both actions.
💡 Proposed fix
-<Button variant="outline" size="sm" `@click`="createResultTab()">
+<Button aria-label="Add runtime tab" variant="outline" size="sm" `@click`="createResultTab()">
<Plus class="h-4 w-4" />
</Button>
-<Button variant="outline" size="sm" `@click`="removeActiveTab">
+<Button aria-label="Close active runtime tab" variant="outline" size="sm" `@click`="removeActiveTab">
<Minus class="h-4 w-4" />
</Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 1297 - 1303, The icon-only
tab buttons lack accessible names; update the two Button elements that call
createResultTab() and removeActiveTab to provide accessible labels (e.g., add
aria-label attributes like aria-label="Add result tab" and aria-label="Remove
active tab" or include visually hidden text inside the Button) so screen readers
can announce their purpose; target the Button components wrapping the Plus and
Minus icons in the QueriesView.vue template and ensure labels are concise and
descriptive.
Comment on lines
+1470
to
+1480
| <div class="flex gap-2"> | ||
| <Button variant="outline" size="sm" @click.stop="void rerunHistoryItem(item)"> | ||
| <RefreshCcw class="h-4 w-4" /> | ||
| </Button> | ||
| <Button | ||
| :variant="item.favorite ? 'default' : 'outline'" | ||
| size="sm" | ||
| @click.stop="void toggleFavorite(item)" | ||
| > | ||
| <Star class="h-4 w-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Label the history action buttons.
The rerun and favorite controls are icon-only here, so their purpose is not announced in the history table. Adding aria-label is enough for both, and aria-pressed would make the favorite toggle state explicit.
💡 Proposed fix
-<Button variant="outline" size="sm" `@click.stop`="void rerunHistoryItem(item)">
+<Button aria-label="Open query log in a new runtime tab" variant="outline" size="sm" `@click.stop`="void rerunHistoryItem(item)">
<RefreshCcw class="h-4 w-4" />
</Button>
<Button
+ :aria-label="item.favorite ? 'Remove query from common queries' : 'Save query to common queries'"
+ :aria-pressed="item.favorite"
:variant="item.favorite ? 'default' : 'outline'"
size="sm"
`@click.stop`="void toggleFavorite(item)"
>📝 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.
Suggested change
| <div class="flex gap-2"> | |
| <Button variant="outline" size="sm" @click.stop="void rerunHistoryItem(item)"> | |
| <RefreshCcw class="h-4 w-4" /> | |
| </Button> | |
| <Button | |
| :variant="item.favorite ? 'default' : 'outline'" | |
| size="sm" | |
| @click.stop="void toggleFavorite(item)" | |
| > | |
| <Star class="h-4 w-4" /> | |
| </Button> | |
| <div class="flex gap-2"> | |
| <Button aria-label="Open query log in a new runtime tab" variant="outline" size="sm" `@click.stop`="void rerunHistoryItem(item)"> | |
| <RefreshCcw class="h-4 w-4" /> | |
| </Button> | |
| <Button | |
| :aria-label="item.favorite ? 'Remove query from common queries' : 'Save query to common queries'" | |
| :aria-pressed="item.favorite" | |
| :variant="item.favorite ? 'default' : 'outline'" | |
| size="sm" | |
| `@click.stop`="void toggleFavorite(item)" | |
| > | |
| <Star class="h-4 w-4" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/views/QueriesView.vue` around lines 1470 - 1480, The icon-only
history action buttons lack accessible labels and state; update the Button
elements around the RefreshCcw and Star icons to include descriptive aria-label
attributes (e.g., aria-label="Rerun query" on the button that calls
rerunHistoryItem(item)) and make the favorite toggle expose its state by adding
aria-pressed bound to item.favorite (on the Button that calls
toggleFavorite(item)); ensure these attributes are added where the Button
components are rendered so rerunHistoryItem and toggleFavorite remain the click
handlers.
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
1 participant
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.
Summary
Verification
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests