Fix Query Tool state restoration for new connections and queries. #8987 #9470
Conversation
WalkthroughgetToolContent now returns a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Query Component (Editor)
participant QTC as QueryToolComponent
participant ASP as ApplicationStateProvider
participant Store as Persistence / saveToolData
rect rgba(220,240,255,0.20)
note right of UI: Editor change -> debounced save uses\ncurrent editor value + composed connectionInfo
UI->>UI: compute value + dirty state\nbuild connectionInfo (connection_list, open_file_name, is_editor_dirty)
UI-->>Store: saveToolData(editor.getValue(), connectionInfo, trans_id)
Store-->>UI: save ack
end
rect rgba(220,255,220,0.20)
note right of QTC: Restoration reads stored toolContent\n(which now always contains connectionInfo)
QTC->>ASP: getToolContent()
ASP-->>QTC: toolContent{..., connectionInfo, ...}
QTC->>QTC: validate connection_list\nensure single is_selected or default to first
QTC->>QTC: set active connection & update qtState.params
QTC-->>UI: restore editor state + params
end
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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (7)
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.
| 'open_file_name':queryToolCtx.current_file, 'is_editor_dirty': editor.current.isDirty() }; | ||
| saveToolData('sqleditor', connectionInfo, queryToolCtx.params.trans_id, editor.current.getValue()); | ||
|
|
||
| useDelayDebounce(() => { |
There was a problem hiding this comment.
+ if (!editor.current) {
+ setSaveQtData(false);
+ return;
+ }
Add this as suggested by coderabbit. Rest changes looks good
RohitBhati8269
force-pushed
the
GH-8987
branch
from
a5ff997 to
561e53f
Compare
RohitBhati8269
force-pushed
the
GH-8987
branch
from
561e53f to
7278b01
Compare
2 participants
Detail Info:- #8987
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.