Fix query tool auto execution after session restore#10055
Fix query tool auto execution after session restore#10055dev-hari-prasad wants to merge 4 commits into
Conversation
- Handle both boolean and string values for is_query_tool - Fix Query Tool tabs being incorrectly detected during restore - Prevent queries from running automatically when restoring a session The issue was caused by is_query_tool only checking for the string 'true'. During session restore the value can come back as a boolean, which caused Query Tool tabs to be treated as View/Edit Data tabs and trigger the auto execution path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughQueryToolComponent normalizes ChangesQuery Tool Restore and Auto-Execution Prevention
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (3)
345-352: ⚡ Quick winConsider normalizing the
restoreparameter lifecycle.The
restoreflag has an inconsistent type lifecycle:
- Starts as boolean
truein saved metaData (context snippet 2:restore: true)- Submitted as string
'true'via form textarea (context snippet 1)- Set to string
'false'here after restoration completes- Checked with loose inequality
!= 'true'on line 390While the loose inequality
!=on line 390 handles both booleantrueand string'true'correctly, explicitly setting it to string'false'rather than booleanfalseadds cognitive overhead. Consider either:
- Normalizing to boolean throughout (set to
falsehere, check with!== trueon line 390), or- Documenting why string
'false'is required for persistence/serialization.The functional update pattern
prev => ({...prev, ...})is correct for React 19.💡 Suggested normalization (if no serialization constraint)
setQtStatePartial(prev => ({ ...prev, editor_disabled: false, params: { ...prev.params, - restore: 'false' + restore: false } }));And on line 390:
- if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore != 'true') { + if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore !== true) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 345 - 352, The restore flag in setQtStatePartial is being set to the string 'false' which mismatches its boolean usage elsewhere (saved metaData uses true and it's checked later with a loose comparison); change the code in the state updater (setQtStatePartial) to set params.restore to the boolean false instead of the string 'false', and update the check that reads the flag (the condition that currently compares against 'true') to use a strict boolean check (e.g., !== true) so the restore lifecycle is normalized to boolean across setQtStatePartial, params.restore, and the restore-checking logic.
917-920: ⚖️ Poor tradeoffReplace brittle string substitution with URL parameter manipulation.
The current implementation directly mutates
panel.metaData.toolUrlusing simple string replacement. This approach is fragile:
- Silent failure: If
'is_query_tool=false'isn't present (unexpected URL format), the replace silently does nothing.- First-match only:
String.replace()without a regex only replaces the first occurrence.- No URL encoding handling: Doesn't handle potential encoding variations.
- Brittle pattern matching: Could theoretically match the substring in other contexts (though unlikely given the URL structure from context snippets).
Additionally, directly mutating
panel.metaDatamay have side effects if the object is shared or tracked elsewhere.🔧 Robust URL parameter manipulation
toggleQueryTool: () => setQtStatePartial((prev)=>{ let panel = qtPanelDocker?.find(qtPanelId); if (panel?.metaData?.toolUrl) { - panel.metaData.toolUrl = panel.metaData.toolUrl.replace('is_query_tool=false', 'is_query_tool=true'); + try { + const url = new URL(panel.metaData.toolUrl, window.location.origin); + url.searchParams.set('is_query_tool', 'true'); + panel.metaData.toolUrl = url.pathname + url.search + url.hash; + } catch (e) { + console.warn('Failed to update is_query_tool parameter:', e); + } } return { ...prev,This approach:
- Properly parses the URL
- Updates the specific parameter
- Handles any URL structure
- Catches malformed URLs
- Preserves all other parameters
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 917 - 920, The current direct string replacement on panel.metaData.toolUrl is brittle; instead parse the toolUrl using the URL/URLSearchParams APIs (inside a try/catch to handle malformed URLs), set the search parameter is_query_tool to "true" via url.searchParams.set('is_query_tool','true'), reconstruct the full URL with url.toString(), and then assign the new string back (preferably by replacing the toolUrl property immutably, e.g. panel.metaData = Object.assign({}, panel.metaData, {toolUrl: newUrl})) so you avoid silent failures, encoding issues, and unintended shared-object side effects; locate this change around the qtPanelDocker?.find(qtPanelId) / panel.metaData.toolUrl code.
141-141: ⚡ Quick winUse consistent equality operators when checking
is_query_tool.The type-flexibility check mixes strict equality (
=== true) and loose equality (== 'true'). While functionally correct here, mixing operators is inconsistent and can obscure intent.Additionally, this pattern only explicitly handles
trueand'true', but URL parameters from context snippets showis_query_tool=falseis also possible (from view-data mode). The current logic treats any non-true/'true'value (including the string'false') as falsy, which works but could be made more explicit.♻️ Proposed fix for consistency
- is_query_tool: params.is_query_tool === true || params.is_query_tool == 'true', + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true',Apply the same fix on line 157:
- _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool == 'true'), + _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool === 'true'),Alternatively, for more explicit boolean coercion:
- is_query_tool: params.is_query_tool === true || params.is_query_tool == 'true', + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true',Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` at line 141, The boolean check for is_query_tool mixes === and == and doesn't explicitly handle string values like 'false'; replace the expression used to derive is_query_tool (currently: params.is_query_tool === true || params.is_query_tool == 'true') with a consistent coercion such as String(params.is_query_tool) === 'true' (or params.is_query_tool === true || params.is_query_tool === 'true') to use strict equality in both branches; update the same expression wherever it appears (the other occurrence that initializes is_query_tool in this component) so both places use the identical, explicit check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 345-352: The restore flag in setQtStatePartial is being set to the
string 'false' which mismatches its boolean usage elsewhere (saved metaData uses
true and it's checked later with a loose comparison); change the code in the
state updater (setQtStatePartial) to set params.restore to the boolean false
instead of the string 'false', and update the check that reads the flag (the
condition that currently compares against 'true') to use a strict boolean check
(e.g., !== true) so the restore lifecycle is normalized to boolean across
setQtStatePartial, params.restore, and the restore-checking logic.
- Around line 917-920: The current direct string replacement on
panel.metaData.toolUrl is brittle; instead parse the toolUrl using the
URL/URLSearchParams APIs (inside a try/catch to handle malformed URLs), set the
search parameter is_query_tool to "true" via
url.searchParams.set('is_query_tool','true'), reconstruct the full URL with
url.toString(), and then assign the new string back (preferably by replacing the
toolUrl property immutably, e.g. panel.metaData = Object.assign({},
panel.metaData, {toolUrl: newUrl})) so you avoid silent failures, encoding
issues, and unintended shared-object side effects; locate this change around the
qtPanelDocker?.find(qtPanelId) / panel.metaData.toolUrl code.
- Line 141: The boolean check for is_query_tool mixes === and == and doesn't
explicitly handle string values like 'false'; replace the expression used to
derive is_query_tool (currently: params.is_query_tool === true ||
params.is_query_tool == 'true') with a consistent coercion such as
String(params.is_query_tool) === 'true' (or params.is_query_tool === true ||
params.is_query_tool === 'true') to use strict equality in both branches; update
the same expression wherever it appears (the other occurrence that initializes
is_query_tool in this component) so both places use the identical, explicit
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13354e15-d117-46d6-9736-8833f3cd5f37
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
- Normalize is_query_tool check with strict equality - Normalize restore flag to use boolean types instead of strings - Refactor fragile toolUrl replacement to use URL API with searchParams
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
293-304:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Restore guard type inconsistency allows execution during restore.
Lines 293 and 300 check
restoreagainst the string'true'using loose equality, but line 390 uses strict inequality against the booleantrue. Whenparams.restorearrives as the string'true'from the URL (the expected format), the guard at line 390 evaluates'true' !== truetotrue, allowing execution to proceed and defeating the restore guard's purpose.Line 350 sets
restore: falseafter restore completes, but this doesn't protect the initial execution check at line 390 when the component mounts withrestore='true'.🛡️ Recommended fix: normalize restore during initialization
Apply this fix at line 141 to normalize
restoreduring state initialization (matching theis_query_toolpattern):is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', + restore: params.restore === true || params.restore === 'true', node_name: retrieveNodeName(selectedNodeInfo),And update line 390 to use strict equality with the normalized boolean:
- if((!qtState.params.is_query_tool || reexecute) && qtState.params.restore !== true) { + if((!qtState.params.is_query_tool || reexecute) && !qtState.params.restore) { eventBus.current.fireEvent(QUERY_TOOL_EVENTS.TRIGGER_EXECUTION, explainObject, macroSQL, executeCursor, executeServerCursor);This ensures consistent boolean handling throughout the restore lifecycle.
Also applies to: 350-350, 390-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 293 - 304, qtState.params.restore is treated inconsistently (string 'true' vs boolean true) which lets restores bypass guards; normalize params.restore to a boolean during state initialization (same approach used for is_query_tool) so qtState.params.restore is true/false, then change the runtime guard that currently checks restore against the string to a strict boolean check (e.g., use qtState.params.restore === true) and ensure restoreToolContent and the subsequent setQtStatePartial({ restore: false }) work with the normalized boolean value.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (2)
919-926: 💤 Low valueConsider validating toolUrl before URL construction.
The URL API approach is solid, but if
panel.metaData.toolUrlhas an unexpected format (e.g., already a full URL with a different origin, or malformed), thetry/catchwill silently log a warning without indicating the root cause. Consider adding a pre-check or enriching the error message to aid debugging.💡 Optional: enrich error logging
try { const url = new URL(panel.metaData.toolUrl, window.location.origin); url.searchParams.set('is_query_tool', 'true'); panel.metaData = Object.assign({}, panel.metaData, {toolUrl: url.pathname + url.search + url.hash}); } catch (e) { - console.warn('Failed to update is_query_tool parameter:', e); + console.warn('Failed to update is_query_tool parameter for toolUrl:', panel.metaData.toolUrl, e); }This helps trace which URL caused the parse failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 919 - 926, The try/catch around new URL(panel.metaData.toolUrl, window.location.origin) should validate and handle edge cases before blindly constructing the URL: check panel.metaData.toolUrl for emptiness and whether it looks like an absolute URL (contains "://" or startsWith("http")), and if absolute use new URL(panel.metaData.toolUrl) while if relative use new URL(panel.metaData.toolUrl, window.location.origin); also enrich the catch to include the offending panel.metaData.toolUrl and window.location.origin and the caught error (e) in the processLogger/console message so you can see the root cause when URL construction fails.
1010-1028: 💤 Low valueMissing PropTypes entry for restore parameter.
The
restoreparameter is used throughout the component (lines 293, 300, 350, 390), but it's not declared in thePropTypesdefinition. Adding it would improve type safety and documentation.📝 Suggested addition
is_query_tool: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]).isRequired, + restore: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]), server_cursor: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 1010 - 1028, Add a missing PropTypes entry for the params.restore prop on QueryToolComponent: update the params PropTypes shape to include a non-required restore property (e.g., params.restore: PropTypes.oneOfType([PropTypes.bool, PropTypes.string])) so the restore prop used in the component (references to params.restore) is declared and type-checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 293-304: qtState.params.restore is treated inconsistently (string
'true' vs boolean true) which lets restores bypass guards; normalize
params.restore to a boolean during state initialization (same approach used for
is_query_tool) so qtState.params.restore is true/false, then change the runtime
guard that currently checks restore against the string to a strict boolean check
(e.g., use qtState.params.restore === true) and ensure restoreToolContent and
the subsequent setQtStatePartial({ restore: false }) work with the normalized
boolean value.
---
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 919-926: The try/catch around new URL(panel.metaData.toolUrl,
window.location.origin) should validate and handle edge cases before blindly
constructing the URL: check panel.metaData.toolUrl for emptiness and whether it
looks like an absolute URL (contains "://" or startsWith("http")), and if
absolute use new URL(panel.metaData.toolUrl) while if relative use new
URL(panel.metaData.toolUrl, window.location.origin); also enrich the catch to
include the offending panel.metaData.toolUrl and window.location.origin and the
caught error (e) in the processLogger/console message so you can see the root
cause when URL construction fails.
- Around line 1010-1028: Add a missing PropTypes entry for the params.restore
prop on QueryToolComponent: update the params PropTypes shape to include a
non-required restore property (e.g., params.restore:
PropTypes.oneOfType([PropTypes.bool, PropTypes.string])) so the restore prop
used in the component (references to params.restore) is declared and
type-checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7576b2cd-c69b-41c5-ae5f-d9230246945d
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (2)
921-923: ⚡ Quick winSimplify the absolute URL detection.
The condition
toolUrl.includes('://') || toolUrl.startsWith('http')could incorrectly classify edge cases. For instance, a URL string"http"(without://) would match the second condition and be treated as absolute, causingnew URL("http")to throw an error.♻️ Proposed fix to use a more robust check
-const isAbsolute = toolUrl.includes('://') || toolUrl.startsWith('http'); +const isAbsolute = toolUrl.includes('://'); const url = isAbsolute ? new URL(toolUrl) : new URL(toolUrl, window.location.origin);The
includes('://')check reliably identifies absolute URLs (http://, https://, file://, etc.) without false positives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` around lines 921 - 923, The absolute-URL detection using isAbsolute (currently toolUrl.includes('://') || toolUrl.startsWith('http')) is too permissive; change the check to rely only on the presence of '://'. Locate the variables toolUrl, isAbsolute, and url in QueryToolComponent.jsx and update isAbsolute to use toolUrl.includes('://') only, then construct url using new URL(toolUrl) when isAbsolute is true and new URL(toolUrl, window.location.origin) otherwise to avoid treating short strings like "http" as absolute.
158-158: ⚡ Quick winConsider reusing the normalized
is_query_toolvalue.The normalization check
params.is_query_tool === true || params.is_query_tool === 'true'is duplicated from line 141. Since theparamsobject (containing the normalizedis_query_tool) is constructed beforeconnection_list, you could reference the already-normalized value to reduce duplication.♻️ Proposed refactor to eliminate duplication
Compute the normalized boolean once before the state object:
+const normalizedParams = { + ...params, + title: _.unescape(params.title), + is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', + restore: params.restore === true || params.restore === 'true', + node_name: retrieveNodeName(selectedNodeInfo), + dbname: _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), + server_cursor: preferencesStore.getPreferencesForModule('sqleditor').server_cursor === true, +}; + const [qtState, setQtState] = useState({ preferences: { ... }, is_new_tab: window.location == window.parent?.location, ... - params: { - ...params, - title: _.unescape(params.title), - is_query_tool: params.is_query_tool === true || params.is_query_tool === 'true', - restore: params.restore === true || params.restore === 'true', - node_name: retrieveNodeName(selectedNodeInfo), - dbname: _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), - server_cursor: preferencesStore.getPreferencesForModule('sqleditor').server_cursor === true, - }, + params: normalizedParams, connection_list: [{ ... conn_title: getTitle( pgAdmin, null, selectedNodeInfo, true, _.unescape(params.server_name), _.unescape(params.database_name) || getDatabaseLabel(selectedNodeInfo), - _.unescape(params.role) || _.unescape(params.user), params.is_query_tool === true || params.is_query_tool === 'true'), + _.unescape(params.role) || _.unescape(params.user), normalizedParams.is_query_tool), ... }],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx` at line 158, The duplicated normalization of params.is_query_tool should be removed: use the already-normalized boolean stored on params (or a local variable like isQueryTool set when building params) when constructing connection_list instead of re-evaluating params.is_query_tool === true || params.is_query_tool === 'true'; update the connection_list call in QueryToolComponent.jsx to reference params.is_query_tool (or the local isQueryTool) so the normalized value is computed once and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx`:
- Around line 921-923: The absolute-URL detection using isAbsolute (currently
toolUrl.includes('://') || toolUrl.startsWith('http')) is too permissive; change
the check to rely only on the presence of '://'. Locate the variables toolUrl,
isAbsolute, and url in QueryToolComponent.jsx and update isAbsolute to use
toolUrl.includes('://') only, then construct url using new URL(toolUrl) when
isAbsolute is true and new URL(toolUrl, window.location.origin) otherwise to
avoid treating short strings like "http" as absolute.
- Line 158: The duplicated normalization of params.is_query_tool should be
removed: use the already-normalized boolean stored on params (or a local
variable like isQueryTool set when building params) when constructing
connection_list instead of re-evaluating params.is_query_tool === true ||
params.is_query_tool === 'true'; update the connection_list call in
QueryToolComponent.jsx to reference params.is_query_tool (or the local
isQueryTool) so the normalized value is computed once and reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8ae2cda-8f88-4f6d-8427-a09d8dbf75b5
📒 Files selected for processing (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
- Extracted isQueryTool to eliminate duplicate checks - Simplified absolute-URL detection to use '://' only
|
The PR is ready to merge. I have addressed all concerns of CodeRabbit. Pls consider merging this after updating the branch. I have used AI in the last two commit eg. Refactoring: Clean up state initialization and URL checks and Update QueryToolComponent.jsx, but rest assured, I have thoroughly read and understood the changes, made the initial changes myself, and verified everything manually. Pls review the changes @asheshv if any changes are required, let me know. |
1 participant
What this fixes
Fixes an issue where Query Tool tabs are restored from a previous session can/start automatically executing their queries when pgAdmin starts.
What was happening
While looking into this, I found that
is_query_toolwas only being checked against the string value 'true'`.During the session restoration process the value can be restored as a boolean instead. When that happens, the tab gets treated as a view/edit data tab instead of a Query toool tab and can end up going through the auto execution logic during initialization. Which can lead to descurtive and unwanted queries to run against the database and this is not what a user would expect.
Fix
This change addresses the restore flow in a few places:
is_query_toolis_query_toolstate in the saved metadataTesting
PS: You will need to build the app with the changes made in this branch inorder to see this in effect otherwise the offical build will emit the same bevhiour as descirbied in the issue #10031.
Fixes #10031
Summary by CodeRabbit