Skip to content

Keep Query Tool execution disabled until a restored tab's execute options reach the server. #10618 #10619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dpage wants to merge 2 commits into from
Closed

Keep Query Tool execution disabled until a restored tab's execute options reach the server. #10618 #10619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80e5d57
Keep Query Tool execution disabled until a restored tab's execute opt…
dpage Oct 15, 2026
31992d7
Also keep macros, and the render before the requests start, gated unt…
dpage Oct 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions web/pgadmin/tools/sqleditor/static/js/components/sections/MainToolBar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Check menu options that are saved and restored with the tab's state.
export const RESTORABLE_TOOLBAR_OPTIONS = [
...EXECUTION_OPTIONS,
Expand Down Expand Up @@ -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);
Expand All @@ -116,6 +122,15 @@ 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. 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]
|| (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){
Expand Down Expand Up @@ -224,8 +239,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') {
Expand All @@ -238,6 +257,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]);
Expand Down Expand Up @@ -394,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)=>{
Expand Down Expand Up @@ -500,33 +525,33 @@ 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'}
});}
}
},
{
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();}
}
},
{
Expand Down Expand Up @@ -643,18 +668,18 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT
<PgIconButton title={gettext('Cancel query')} icon={<StopRoundedIcon style={{height: 'unset'}} />}
onClick={cancelQuery} disabled={buttonsDisabled['cancel']} shortcut={queryToolPref.btn_cancel_query} />
<PgIconButton title={gettext('Execute script')} icon={<PlayArrowRoundedIcon style={{height: 'unset'}} />}
onClick={executeScript} disabled={buttonsDisabled['execute']} shortcut={queryToolPref.execute_script}/>
onClick={executeScript} disabled={isButtonDisabled('execute')} shortcut={queryToolPref.execute_script}/>
<PgIconButton title={gettext('Execute query')} icon={<ExecuteQueryIcon style={{padding: '2px 5px'}} />}
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}/>
<PgIconButton title={gettext('Execute options')} icon={<KeyboardArrowDownIcon />} splitButton
name="menu-autocommit" ref={autoCommitMenuRef} shortcut={queryToolPref.btn_execute_options}
onClick={toggleMenu} disabled={buttonsDisabled['execute-options']}/>
onClick={toggleMenu} disabled={isButtonDisabled('execute-options')}/>
</PgButtonGroup>
<PgButtonGroup size="small">
<PgIconButton title={gettext('Explain')} icon={<ExplicitRoundedIcon />}
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}/>
<PgIconButton title={gettext('Explain Analyze')} icon={<AssessmentRoundedIcon />}
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}/>
<PgIconButton title={gettext('Explain Settings')} icon={<KeyboardArrowDownIcon />} splitButton
disabled={!queryToolCtx.params.is_query_tool}
name="menu-explain" ref={explainMenuRef} onClick={toggleMenu} />
Expand All @@ -668,7 +693,7 @@ export function MainToolBar({containerRef, onFilterClick, onManageMacros, onAddT
<PgButtonGroup size="small">
<PgIconButton title={gettext('Macros')} icon={
<><FormatListNumberedRoundedIcon /><KeyboardArrowDownIcon style={{marginLeft: '-10px'}} /></>}
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} />
</PgButtonGroup>
<PgButtonGroup size="small">
<PgIconButton title={gettext('Help')} icon={<HelpIcon />} onClick={onHelpClick} />
Expand Down
134 changes: 133 additions & 1 deletion web/regression/javascript/sqleditor/MainToolBarRestore.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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));
Expand All @@ -177,6 +179,136 @@ 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('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; }));
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());
Expand Down
Loading