Fix Query Tool state restoration for new connections and queries. #8987 #9470
Conversation
WalkthroughThe changes augment connection information tracking in the SQL editor's state management layer. The application state provider now includes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
311-313: Consider avoiding direct mutation of restored data.Line 312 directly mutates
connList[0].is_selected, which modifies the object from the API response. While this works in practice since the data is immediately used to set state, creating a new object would be more idiomatic:// ensure exactly one is_selected if (!connList.some(c => c.is_selected)) { - connList[0].is_selected = true; + connList[0] = { ...connList[0], is_selected: true }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/pgadmin/settings/static/ApplicationStateProvider.jsx(1 hunks)web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx(1 hunks)web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx (3)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
queryToolCtx(832-832)web/pgadmin/static/js/components/ReactCodeMirror/index.jsx (1)
editor(67-67)web/pgadmin/static/js/custom_hooks.js (1)
useDelayDebounce(80-89)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 18)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
🔇 Additional comments (5)
web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx (2)
71-71: Good approach capturing initial trans_id.Using a ref to capture the initial
trans_idensures the debounced save operation uses a stable identifier even ifqueryToolCtx.params.trans_idchanges during the component lifecycle. This is a reasonable fix for state restoration with new connections.
365-380: Verify intent of saving when editor is empty.The condition
isDirty || value.length === 0will trigger saves even when the editor is empty and clean. This could cause unnecessary API calls on initial component mount or when clearing the editor.Was this intentional to ensure the empty state is persisted for restoration? If so, consider adding a guard to prevent saves during initial render (e.g., a mounted ref).
web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx (1)
308-331: Connection restoration logic looks correct.The implementation properly:
- Checks for valid connection list array
- Ensures exactly one connection is marked as selected (defaulting to first)
- Extracts connection parameters to update
paramsstate- Uses spread operator correctly to preserve existing state
One minor observation: the
setQtStatePartialcallback receivesprevbut then spreads it at the top level. SincesetQtStatePartialalready merges with previous state (see line 166-168), the explicit spread ofprevandprev.paramsis correct here to ensure nested params are preserved.web/pgadmin/settings/static/ApplicationStateProvider.jsx (2)
74-83: Consistent connectionInfo in all return paths - LGTM.The changes ensure
connectionInfois included in every return path ofgetToolContent, providing a consistent API for consumers. This aligns well with the restoration logic inQueryToolComponent.jsxthat now depends onconnectionInfo.connection_list.
62-83: Null handling is properly delegated to consumers.The
connectionInfocan beundefined(from line 62's optional chaining), and this is appropriately handled by consumers using optional chaining (toolContent.connectionInfo?.connection_list). The design correctly allows for backward compatibility with saved states that may not haveconnection_info.
| setQtStatePartial(prev => ({ | ||
| ...prev, | ||
| connection_list: connList, | ||
| params: { | ||
| ...prev.params, | ||
| sid: sel.sid, | ||
| did: sel.did, | ||
| user: sel.user, | ||
| role: sel.role, | ||
| server_name: sel.server_name, | ||
| database_name: sel.database_name, | ||
| title: sel.title, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for sgid, fgcolor, bgcolor usage in params context
echo "=== Searching for sgid, fgcolor, bgcolor in params ==="
rg -n --type=js -i 'params\.(sgid|fgcolor|bgcolor)' web/pgadmin/tools/sqleditor/
echo -e "\n=== Searching for these fields in connection objects ==="
rg -n --type=js '(sgid|fgcolor|bgcolor)' web/pgadmin/tools/sqleditor/ | grep -v node_modules | head -20Repository: pgadmin-org/pgadmin4
Length of output: 3094
🏁 Script executed:
#!/bin/bash
# Get the actual code context around lines 317-330
echo "=== Code at lines 317-330 in QueryToolComponent.jsx ==="
sed -n '310,340p' web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
echo -e "\n=== Initial state context (lines 140-165) ==="
sed -n '140,165p' web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsxRepository: pgadmin-org/pgadmin4
Length of output: 2507
Restore missing connection parameters during user/role updates.
When updating params after a user/role change, the restoration at lines 322-328 omits sgid, fgcolor, and bgcolor that are present in the connection state. The sgid field is used for connection matching (line 922), and fgcolor/bgcolor are used for UI styling (ConnectionBar.jsx). Include these fields in the params update to maintain connection identification and visual consistency.
🤖 Prompt for AI Agents
In web/pgadmin/tools/sqleditor/static/js/components/QueryToolComponent.jsx
around lines 317 to 330, the params update after selecting a connection restores
several fields but omits sgid, fgcolor, and bgcolor which are needed for
connection matching and UI styling; update the params object to also copy
sel.sgid, sel.fgcolor, and sel.bgcolor (e.g., add sgid: sel.sgid, fgcolor:
sel.fgcolor, bgcolor: sel.bgcolor) so the connection identification and visual
appearance are preserved when user/role changes occur.
|
|
||
| useDelayDebounce(() => { | ||
| const currentTransId = initialTransIdRef.current || queryToolCtx.params.trans_id; | ||
| const currentConnList = queryToolCtx.connection_list; | ||
| const currentFile = queryToolCtx.current_file; | ||
|
|
||
| let connectionInfo = { | ||
| connection_list: currentConnList, | ||
| open_file_name: currentFile, | ||
| is_editor_dirty: editor.current.isDirty() | ||
| }; | ||
| saveToolData('sqleditor', connectionInfo, currentTransId, editor.current.getValue()); | ||
| setSaveQtData(false); | ||
| }, saveQtData, 500); |
There was a problem hiding this comment.
Add null guard for editor reference in debounced save.
The debounced callback accesses editor.current without null checks. If the component unmounts or the editor hasn't initialized when the debounce fires, this could throw.
useDelayDebounce(() => {
+ if (!editor.current) {
+ setSaveQtData(false);
+ return;
+ }
const currentTransId = initialTransIdRef.current || queryToolCtx.params.trans_id;
const currentConnList = queryToolCtx.connection_list;
const currentFile = queryToolCtx.current_file;
let connectionInfo = {
connection_list: currentConnList,
open_file_name: currentFile,
is_editor_dirty: editor.current.isDirty()
};
saveToolData('sqleditor', connectionInfo, currentTransId, editor.current.getValue());
setSaveQtData(false);
}, saveQtData, 500);🤖 Prompt for AI Agents
In web/pgadmin/tools/sqleditor/static/js/components/sections/Query.jsx around
lines 384 to 397, the debounced callback dereferences editor.current without
null checks which can throw if the component unmounts or the editor isn't
initialized; add a null guard at the start of the callback (e.g. if (!editor ||
!editor.current) return or skip editor-dependent fields) and only call
editor.current.isDirty() and editor.current.getValue() when editor.current is
present, ensuring saveToolData still receives safe values (or bail out early)
and setSaveQtData(false) is handled appropriately.
1 participant
Detail Info:- #8987
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.