From 80e5d5789f906d2c279866713dba47baf8d1d158 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 17:33:26 +0100 Subject: [PATCH 1/2] Keep Query Tool execution disabled until a restored tab's execute options reach the server. #10618 --- .../js/components/sections/MainToolBar.jsx | 42 +++++--- .../sqleditor/MainToolBarRestore.spec.js | 102 +++++++++++++++++- 2 files changed, 131 insertions(+), 13 deletions(-) diff --git a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx index 5cac5d3dd43..c9e34c1923c 100644 --- a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx +++ b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx @@ -57,6 +57,10 @@ function changeQueryExecutionSettings(type, api, transId, value) { // they must be sent to the server when changed. const EXECUTION_OPTIONS = ['auto_commit', 'auto_rollback', 'server_cursor']; +// Buttons kept disabled whilst restored execute options are being sent to the +// server, so that nothing runs with the preference values instead. +const BUTTONS_GATED_BY_EXEC_OPTIONS = ['execute', 'execute-options', 'explain', 'explain_analyse']; + // Check menu options that are saved and restored with the tab's state. export const RESTORABLE_TOOLBAR_OPTIONS = [ ...EXECUTION_OPTIONS, @@ -101,6 +105,8 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const checkedMenuItemsRef = React.useRef(checkedMenuItems); // Restored execute options waiting for the connection to be established. const [pendingExecOptions, setPendingExecOptions] = useState(null); + // The number of restored execute options still being sent to the server. + const [applyingExecOptions, setApplyingExecOptions] = useState(0); const {isSaveToolDataEnabled} = useApplicationState() ?? {}; /* Menu button refs */ const saveAsMenuRef = React.useRef(null); @@ -116,6 +122,11 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT const setDisableButton = useCallback((name, disable=true)=>{ setButtonsDisabled((prev)=>({...prev, [name]: disable})); }, []); + /* Kept separate from buttonsDisabled, which the execution events set + directly, so that nothing else can enable these buttons early and the + gate always lifts once the requests have settled. */ + const isButtonDisabled = (name)=>(buttonsDisabled[name] + || (applyingExecOptions > 0 && BUTTONS_GATED_BY_EXEC_OPTIONS.includes(name))); const executeCursor = useCallback(()=>{ if(!queryToolCtx.preferences.sqleditor.underline_query_cursor && queryToolCtx.preferences.sqleditor.underlined_query_execute_warning){ @@ -224,8 +235,12 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT return; } setPendingExecOptions(null); - Object.entries(pendingExecOptions).forEach(([option, value])=>{ - changeQueryExecutionSettings(option, queryToolCtx.api, queryToolCtx.params.trans_id, value) + const options = Object.entries(pendingExecOptions); + setApplyingExecOptions((prev)=>prev + options.length); + options.forEach(([option, value])=>{ + // Start from a promise so that a synchronous failure is handled too. + Promise.resolve() + .then(()=>changeQueryExecutionSettings(option, queryToolCtx.api, queryToolCtx.params.trans_id, value)) .catch((error)=>{ setCheckedMenuItems((prev)=>({...prev, [option]: !value})); if(option === 'server_cursor') { @@ -238,6 +253,9 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT eventBus.fireEvent(QUERY_TOOL_EVENTS.HANDLE_API_ERROR, error, { checkTransaction: true, }); + }) + .finally(()=>{ + setApplyingExecOptions((prev)=>prev - 1); }); }); }, [pendingExecOptions, queryToolConnCtx.connected]); @@ -500,7 +518,7 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT { shortcut: queryToolPref.btn_execute_options, options: { - callback: ()=>{!buttonsDisabled['execute-options']&&toggleMenu({ + callback: ()=>{!isButtonDisabled('execute-options')&&toggleMenu({ currentTarget: {name: 'menu-autocommit'} });} } @@ -508,25 +526,25 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT { shortcut: queryToolPref.execute_script, options: { - callback: ()=>{!buttonsDisabled['execute']&&executeScript();} + callback: ()=>{!isButtonDisabled('execute')&&executeScript();} } }, { shortcut: queryToolPref.execute_cursor, options: { - callback: ()=>{!buttonsDisabled['execute']&&executeCursor();} + callback: ()=>{!isButtonDisabled('execute')&&executeCursor();} } }, { shortcut: queryToolPref.explain_query, options: { - callback: (e)=>{e.preventDefault(); !buttonsDisabled['explain']&&explain();} + callback: (e)=>{e.preventDefault(); !isButtonDisabled('explain')&&explain();} } }, { shortcut: queryToolPref.explain_analyze_query, options: { - callback: ()=>{!buttonsDisabled['explain_analyse']&&explainAnalyse();} + callback: ()=>{!isButtonDisabled('explain_analyse')&&explainAnalyse();} } }, { @@ -643,18 +661,18 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT } onClick={cancelQuery} disabled={buttonsDisabled['cancel']} shortcut={queryToolPref.btn_cancel_query} /> } - onClick={executeScript} disabled={buttonsDisabled['execute']} shortcut={queryToolPref.execute_script}/> + onClick={executeScript} disabled={isButtonDisabled('execute')} shortcut={queryToolPref.execute_script}/> } - onClick={executeCursor} disabled={buttonsDisabled['execute'] || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.execute_cursor}/> + onClick={executeCursor} disabled={isButtonDisabled('execute') || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.execute_cursor}/> } splitButton name="menu-autocommit" ref={autoCommitMenuRef} shortcut={queryToolPref.btn_execute_options} - onClick={toggleMenu} disabled={buttonsDisabled['execute-options']}/> + onClick={toggleMenu} disabled={isButtonDisabled('execute-options')}/> } - onClick={()=>{explain();}} disabled={buttonsDisabled['explain'] || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.explain_query}/> + onClick={()=>{explain();}} disabled={isButtonDisabled('explain') || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.explain_query}/> } - onClick={()=>{explainAnalyse();}} disabled={buttonsDisabled['explain_analyse'] || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.explain_analyze_query}/> + onClick={()=>{explainAnalyse();}} disabled={isButtonDisabled('explain_analyse') || !queryToolCtx.params.is_query_tool} shortcut={queryToolPref.explain_analyze_query}/> } splitButton disabled={!queryToolCtx.params.is_query_tool} name="menu-explain" ref={explainMenuRef} onClick={toggleMenu} /> diff --git a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js index 8d7d88f8d23..b287c34bb6b 100644 --- a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js +++ b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js @@ -28,7 +28,7 @@ jest.mock('../../../pgadmin/settings/static/ApplicationStateProvider', () => ({ })); import React from 'react'; -import { render, act, waitFor } from '@testing-library/react'; +import { render, act, waitFor, fireEvent as fireDomEvent } from '@testing-library/react'; import '@testing-library/jest-dom'; import { withTheme } from '../fake_theme'; import EventBus from '../../../pgadmin/static/js/helpers/EventBus'; @@ -177,6 +177,106 @@ describe('MainToolBar toolbar options restore', () => { fireEvent.mockRestore(); }); + // A disabled button loses its accessible name to a wrapping span. + const gatedButtons = () => ['Execute script', 'Execute query', 'Execute options', 'Explain', 'Explain Analyze'] + .map((label) => document.querySelector(`button[data-label="${label}"]`)); + const expectGated = (disabled) => { + gatedButtons().forEach((btn) => { + if(disabled) { + expect(btn).toBeDisabled(); + } else { + expect(btn).toBeEnabled(); + } + }); + }; + + it('keeps execution disabled until restored execute options are applied', async () => { + const resolvers = []; + queryToolCtx.api.post = jest.fn(() => new Promise((resolve) => { resolvers.push(resolve); })); + renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + expectGated(false); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false, auto_rollback: true}); + }); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(2)); + await waitFor(() => expectGated(true)); + + // Still disabled until every option has reached the server. + await act(async () => { resolvers[0]({data: {}}); }); + expectGated(true); + + await act(async () => { resolvers[1]({data: {}}); }); + await waitFor(() => expectGated(false)); + }); + + it('ignores the execute shortcut until restored execute options are applied', async () => { + let resolvePost; + queryToolCtx.api.post = jest.fn(() => new Promise((resolve) => { resolvePost = resolve; })); + // F5 + queryToolCtx.preferences.sqleditor.execute_script = { + alt: false, shift: false, control: false, key: {key_code: 116}, + }; + const triggered = jest.fn(); + eventBus.registerListener(QUERY_TOOL_EVENTS.TRIGGER_EXECUTION, triggered); + const {container} = renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false}); + }); + await waitFor(() => expectGated(true)); + await act(async () => { fireDomEvent.keyDown(container.firstChild, {keyCode: 116}); }); + expect(triggered).not.toHaveBeenCalled(); + + await act(async () => { resolvePost({data: {}}); }); + await waitFor(() => expectGated(false)); + await act(async () => { fireDomEvent.keyDown(container.firstChild, {keyCode: 116}); }); + await waitFor(() => expect(triggered).toHaveBeenCalledTimes(1)); + }); + + it('enables execution again when a restored execute option fails', async () => { + let rejectPost; + queryToolCtx.api.post = jest.fn(() => new Promise((_resolve, reject) => { rejectPost = reject; })); + renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false}); + }); + await waitFor(() => expectGated(true)); + + await act(async () => { rejectPost(new Error('failed')); }); + await waitFor(() => expectGated(false)); + expect(queryToolCtx.toolbarOptionsRef.current.auto_commit).toBe(true); + }); + + it('enables execution again if sending a restored execute option throws', async () => { + queryToolCtx.api.post = jest.fn(() => { throw new Error('failed'); }); + renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false}); + }); + await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalled()); + await waitFor(() => expectGated(false)); + expect(queryToolCtx.toolbarOptionsRef.current.auto_commit).toBe(true); + }); + + it('does not disable execution when no execute options need restoring', async () => { + renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: true, explain_costs: true}); + }); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current.explain_costs).toBe(true)); + expectGated(false); + expect(queryToolCtx.api.post).not.toHaveBeenCalled(); + }); + it('does not restore execute options for View/Edit Data', async () => { renderToolBar(true, false); await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); From 31992d7887d82679b97ebc4ceb12de2af1f926fc Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 17:40:25 +0100 Subject: [PATCH 2/2] Also keep macros, and the render before the requests start, gated until restored execute options reach the server. #10618 Macros run SQL through the same transaction, so their menu and shortcuts now wait too. Restored options pending on a connected tab also count, covering the render before the effect starts the requests; they do not count before the connection exists, as Execute is what re-makes a failed connection. --- .../js/components/sections/MainToolBar.jsx | 13 ++++++-- .../sqleditor/MainToolBarRestore.spec.js | 32 +++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx index c9e34c1923c..04908e2e2ff 100644 --- a/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx +++ b/web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx @@ -124,9 +124,13 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT }, []); /* Kept separate from buttonsDisabled, which the execution events set directly, so that nothing else can enable these buttons early and the - gate always lifts once the requests have settled. */ + gate always lifts once the requests have settled. Pending options count + only once connected, as they are sent in the effect after that render; + before then Execute is what re-makes a failed connection. */ + const execOptionsGated = applyingExecOptions > 0 + || (!_.isEmpty(pendingExecOptions) && Boolean(queryToolConnCtx.connected)); const isButtonDisabled = (name)=>(buttonsDisabled[name] - || (applyingExecOptions > 0 && BUTTONS_GATED_BY_EXEC_OPTIONS.includes(name))); + || (execOptionsGated && BUTTONS_GATED_BY_EXEC_OPTIONS.includes(name))); const executeCursor = useCallback(()=>{ if(!queryToolCtx.preferences.sqleditor.underline_query_cursor && queryToolCtx.preferences.sqleditor.underlined_query_execute_warning){ @@ -412,6 +416,9 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT eventBus.fireEvent(QUERY_TOOL_EVENTS.EXECUTION_START, 'ROLLBACK;', {external: true}); }; const executeMacro = (m)=>{ + /* Macros run through the same transaction, so they must wait for the + restored execute options too. */ + if(execOptionsGated) return; eventBus.fireEvent(QUERY_TOOL_EVENTS.TRIGGER_EXECUTION, null, m.sql); }; const onLimitChange=(e)=>{ @@ -686,7 +693,7 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT } - disabled={!queryToolCtx.params.is_query_tool} name="menu-macros" ref={macrosMenuRef} onClick={toggleMenu} /> + disabled={!queryToolCtx.params.is_query_tool || execOptionsGated} name="menu-macros" ref={macrosMenuRef} onClick={toggleMenu} /> } onClick={onHelpClick} /> diff --git a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js index b287c34bb6b..ff56e2dfad3 100644 --- a/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js +++ b/web/regression/javascript/sqleditor/MainToolBarRestore.spec.js @@ -151,6 +151,8 @@ describe('MainToolBar toolbar options restore', () => { expect(queryToolCtx.updateServerCursor).toHaveBeenCalledWith({server_cursor: true}); // Nothing can be sent to the server until the transaction exists. expect(queryToolCtx.api.post).not.toHaveBeenCalled(); + // Execution must stay possible, as it is what re-makes a failed connection. + expectGated(false); setConnected(true); await waitFor(() => expect(queryToolCtx.api.post).toHaveBeenCalledTimes(2)); @@ -236,6 +238,36 @@ describe('MainToolBar toolbar options restore', () => { await waitFor(() => expect(triggered).toHaveBeenCalledTimes(1)); }); + it('ignores macros until restored execute options are applied', async () => { + let resolvePost; + queryToolCtx.api.post = jest.fn(() => new Promise((resolve) => { resolvePost = resolve; })); + // F6 + queryToolCtx.params.macros = [{ + id: 1, name: 'Test macro', sql: 'SELECT 1;', + alt: false, shift: false, control: false, key: 'F6', key_code: 117, + }]; + const triggered = jest.fn(); + eventBus.registerListener(QUERY_TOOL_EVENTS.TRIGGER_EXECUTION, triggered); + const {container} = renderToolBar(true); + await waitFor(() => expect(queryToolCtx.toolbarOptionsRef.current).not.toBeNull()); + const macrosButton = () => document.querySelector('button[data-label="Macros"]'); + expect(macrosButton()).toBeEnabled(); + + act(() => { + eventBus.fireEvent(QUERY_TOOL_EVENTS.RESTORE_TOOLBAR_OPTIONS, {auto_commit: false}); + }); + await waitFor(() => expectGated(true)); + expect(macrosButton()).toBeDisabled(); + await act(async () => { fireDomEvent.keyDown(container.firstChild, {keyCode: 117}); }); + expect(triggered).not.toHaveBeenCalled(); + + await act(async () => { resolvePost({data: {}}); }); + await waitFor(() => expectGated(false)); + expect(macrosButton()).toBeEnabled(); + await act(async () => { fireDomEvent.keyDown(container.firstChild, {keyCode: 117}); }); + await waitFor(() => expect(triggered).toHaveBeenCalledWith(null, 'SELECT 1;')); + }); + it('enables execution again when a restored execute option fails', async () => { let rejectPost; queryToolCtx.api.post = jest.fn(() => new Promise((_resolve, reject) => { rejectPost = reject; }));