fix: follow-ups from reviewing #531–#533, with the tests they lacked

apps/web/src/frontend/features/access/components/AccessPermissionPanel.test.tsx

@@ -198,7 +198,7 @@ describe('AccessPermissionPanel — one session', () => {
   });
 
   it('keeps the confirmation open when apply fails', async () => {
-    runAccessSql.mockResolvedValue({ ok: false, error: 'role "r" does not exist' });
+    runAccessSql.mockResolvedValue({ ok: false, error: 'role "r" does not exist', applied: 0 });
     await grantsStageFor('alice');
     const loads = fetchDbAccess.mock.calls.length;
     fireEvent.click(await screen.findByTestId('access-grants-preset-read-write'));
@@ -209,6 +209,24 @@ describe('AccessPermissionPanel — one session', () => {
     expect(fetchDbAccess.mock.calls.length).toBe(loads);
   });
 
+  it('re-reads the catalog when part of the script was applied before the failure', async () => {
+    // Statements run in order and stop at the failure; the ones before it stay
+    // applied, so the grid must not keep showing the grants as they were.
+    runAccessSql.mockResolvedValue({
+      ok: false,
+      error: 'Statement 2 of 2 failed: permission denied. The 1 before it was applied.',
+      applied: 1,
+    });
+    await grantsStageFor('alice');
+    const loads = fetchDbAccess.mock.calls.length;
+    fireEvent.click(await screen.findByTestId('access-grants-preset-read-write'));
+    fireEvent.click(await screen.findByTestId('access-grants-open-sql'));
+    fireEvent.click(await screen.findByTestId('access-permission-apply'));
+    await waitFor(() => expect(fetchDbAccess.mock.calls.length).toBeGreaterThan(loads));
+    expect(screen.getByTestId('access-permission-error').textContent).toMatch(/The 1 before it was applied/);
+    expect(screen.getByTestId('access-permission-confirm')).toBeTruthy();
+  });
+
   it('hides Apply without the Grant privileges permission', async () => {
     useAuthStore.setState({
       user: {

apps/web/src/frontend/features/access/components/AccessPermissionPanel.tsx

@@ -180,6 +180,10 @@ export const AccessPermissionPanel: React.FC<{
         confirm.sql
       );
       if (!outcome.ok) {
+        // The statements before the failure stay applied, so re-read the
+        // catalog to show them — then say what failed, since a reload starts
+        // by clearing the error. The confirmation stays open either way.
+        if (outcome.applied > 0) await load({ force: true });
         setError(outcome.error);
         return;
       }

apps/web/src/frontend/features/sql-editor/state/useSqlEditorStore.transaction.test.ts

@@ -40,7 +40,7 @@ const ok = (n: number) => ({
   durationMs: 1,
 });
 
-async function arm(sql: string, checked: number[]) {
+async function arm(sql: string, checked: number[], dialect = 'sqlite') {
   await useSqlEditorStore.persist.rehydrate();
   const tab = createTab({
     id: 't1',
@@ -50,7 +50,8 @@ async function arm(sql: string, checked: number[]) {
     selectedConnectionIds: ['c1'],
   });
   useSyncStore.setState({
-    connections: [{ id: 'c1', name: 'Lite', dialect: 'sqlite', hasPassword: false } as never],
+    // A stored password, so a server engine runs without asking for one.
+    connections: [{ id: 'c1', name: 'Lite', dialect, hasPassword: true } as never],
   });
   useSqlEditorStore.setState({
     tabs: [tab],
@@ -120,4 +121,34 @@ describe('SQL editor one transaction', () => {
     expect(results[1]).toMatchObject({ ok: false });
     expect(results[1] && !results[1].ok ? results[1].error : '').toMatch(/rolled back/);
   });
+
+  it('runs a batch that steers its own transaction statement by statement, and says why', async () => {
+    // Wrapped, its COMMIT would make what ran before it permanent and a later
+    // failure would roll back nothing — while the panel reported a rollback.
+    executeSql.mockResolvedValue({ results: [ok(1)] });
+    const tabId = await arm('INSERT INTO t VALUES (1);\nCOMMIT;\nINSERT INTO t VALUES (2);', [0, 1, 2]);
+    await useSqlEditorStore.getState().execute();
+    expect(executeSql.mock.calls.length).toBe(3);
+    expect(executeSql.mock.calls.every((call) => !(call[5] as { transaction?: boolean } | undefined)?.transaction)).toBe(true);
+    expect(useSqlEditorStore.getState().resultsByTab[tabId]?.warnings?.join('\n')).toMatch(/BEGIN, COMMIT or ROLLBACK/);
+  });
+
+  it('runs statement by statement on an engine with no transactions, and says why', async () => {
+    // ClickHouse commits each statement as it runs; wrapped, a failure would
+    // leave the earlier ones in place while the panel reported a rollback.
+    executeSql.mockResolvedValue({ results: [ok(1)] });
+    const tabId = await arm('INSERT INTO t VALUES (1);\nINSERT INTO t VALUES (2);', [0, 1], 'clickhouse');
+    await useSqlEditorStore.getState().execute();
+    expect(executeSql.mock.calls.length).toBe(2);
+    expect(executeSql.mock.calls.every((call) => !(call[5] as { transaction?: boolean } | undefined)?.transaction)).toBe(true);
+    expect(useSqlEditorStore.getState().resultsByTab[tabId]?.warnings?.join('\n')).toMatch(/ClickHouse has no transactions/);
+  });
+
+  it('still batches savepoints, which stay inside the transaction', async () => {
+    executeSql.mockResolvedValue({ results: [ok(1), ok(2), ok(3)] });
+    await arm('SAVEPOINT s1;\nINSERT INTO t VALUES (1);\nROLLBACK TO SAVEPOINT s1;', [0, 1, 2]);
+    await useSqlEditorStore.getState().execute();
+    expect(executeSql).toHaveBeenCalledTimes(1);
+    expect(executeSql.mock.calls[0]![5]).toEqual({ transaction: true, stopOnError: true });
+  });
 });

apps/web/src/frontend/features/sql-editor/state/useSqlEditorStore.ts

@@ -34,6 +34,7 @@ import {
   isInsertWriteStatement,
   isMutatingDmlStatement,
   isPageableStatement,
+  isTransactionControl,
   isWriteStatement,
   splitSqlStatements,
 } from '@/shared/lib/sql-splitter';
@@ -82,7 +83,7 @@ import {
   type SqlVariableKind,
   type VariableOverride,
 } from '@/shared/lib/sql-variables';
-import { connectionNeedsSecret } from '@/shared/lib/provider-settings';
+import { connectionNeedsSecret, dialectHasTransactions, getProviderSettings } from '@/shared/lib/provider-settings';
 import { useSyncStore } from '@/features/compare';
 import { SQL_EDITOR_PERSIST_KEY, SQL_EDITOR_PERSIST_VERSION, useRecentQueries } from './recentQueries';
 import { workspaceEditorStorage } from '@/shared/lib/editorWorkspaceStorage';
@@ -1285,15 +1286,31 @@ export const useSqlEditorStore = create<SqlEditorState>()(
         };
 
         const wantsTransaction = get().oneTransaction && rawStatements.length > 1;
-        const transactionBlocked = rawStatements.some(
+        const hasCellOrSet = rawStatements.some(
           (statement) => detectCodeCell(statement) || parseSetDirectives(statement).directives.length > 0
         );
-        if (wantsTransaction && transactionBlocked) {
+        // A BEGIN or COMMIT of the batch's own would end the wrapping
+        // transaction halfway: what ran before it stays, and a later failure
+        // rolls back nothing. The server refuses such a batch; run it as typed.
+        const steersOwnTransaction = rawStatements.some((statement) => isTransactionControl(statement));
+        // ClickHouse, Redis and MongoDB open no transaction: each statement
+        // commits as it runs, so a failure cannot take the earlier ones back.
+        // The server refuses the batch; run it as typed and say so.
+        const noTransaction = connections.find((c) => !dialectHasTransactions(c.dialect));
+        if (wantsTransaction && hasCellOrSet) {
           appendWarning(
             'One transaction applies to plain SQL. This run has a code cell or an @set, so each statement runs on its own.'
           );
+        } else if (wantsTransaction && steersOwnTransaction) {
+          appendWarning(
+            'This run has its own BEGIN, COMMIT or ROLLBACK, so it runs statement by statement as written rather than as one transaction.'
+          );
+        } else if (wantsTransaction && noTransaction) {
+          appendWarning(
+            `${getProviderSettings(noTransaction.dialect).label} has no transactions, so this run goes statement by statement and nothing rolls back on a failure.`
+          );
         }
-        const asOneTransaction = wantsTransaction && !transactionBlocked;
+        const asOneTransaction = wantsTransaction && !hasCellOrSet && !steersOwnTransaction && !noTransaction;
         if (asOneTransaction) {
           type Prep = { ok: true; sql: string } | { ok: false; error: string };
           const prepared: Array<Map<string, Prep>> = [];

apps/web/src/frontend/features/utilities/components/DatabaseAccessModal.tsx

@@ -466,6 +466,13 @@ export const DatabaseAccessModal: React.FC<Props> = ({
         sql
       );
       if (!outcome.ok) {
+        // Statements before the failure stay applied; re-read so the list
+        // shows them rather than the grants as they were before — then set
+        // the error, which a reload starts by clearing.
+        if (outcome.applied > 0) {
+          invalidateDbAccessCache(connectionId);
+          await load();
+        }
         setError(outcome.error);
       } else {
         setStatus(kind === 'grant' ? 'Granted.' : 'Revoked.');

apps/web/src/frontend/features/workspaces/components/WorkspaceRailButton.test.tsx

@@ -72,3 +72,30 @@ describe('WorkspaceRailButton', () => {
     expect(screen.queryByTestId('workspace-rail-btn')).toBeNull();
   });
 });
+
+describe('WorkspaceRailButton follows the server’s workspace', () => {
+  beforeEach(() => {
+    localStorage.clear();
+  });
+
+  it('reloads once when the editor was on another workspace than the server opened', async () => {
+    // Switched elsewhere, or another account signed in on this browser: the
+    // editor hydrated from the old workspace and would save its scripts into
+    // the one that is open now.
+    localStorage.setItem('foxschema-active-workspace', 'team-a');
+    const reload = vi.fn();
+    render(<WorkspaceRailButton reload={reload} />);
+    await vi.waitFor(() => expect(reload).toHaveBeenCalledTimes(1));
+    expect(localStorage.getItem('foxschema-active-workspace')).toBe('own');
+  });
+
+  it('does not reload when they already agree, or on the first run', async () => {
+    const reload = vi.fn();
+    render(<WorkspaceRailButton reload={reload} />);
+    expect(await screen.findByTestId('workspace-rail-btn')).toBeTruthy();
+    expect(localStorage.getItem('foxschema-active-workspace')).toBe('own');
+    render(<WorkspaceRailButton reload={reload} />);
+    await screen.findAllByTestId('workspace-rail-btn');
+    expect(reload).not.toHaveBeenCalled();
+  });
+});

apps/web/src/frontend/features/workspaces/components/WorkspaceRailButton.tsx

@@ -16,7 +16,7 @@ import { Layers } from 'lucide-react';
 import { useAuthStore } from '@/platform/authStore';
 import { MountWhenOpened } from '@/shared/components/MountWhenOpened';
 import { useAnchoredPopover } from '@/shared/lib/useAnchoredPopover';
-import { claimEditorWorkspace } from '@/shared/lib/editorWorkspaceStorage';
+import { followServerWorkspace } from '@/shared/lib/editorWorkspaceStorage';
 import { apiListWorkspaces, apiMyInvites, type WorkspaceList } from '../api/workspacesApi';
 
 const WorkspaceMenu = lazy(() => import('./WorkspaceMenu').then((m) => ({ default: m.WorkspaceMenu })));
@@ -29,7 +29,15 @@ function mark(name: string): string {
   return (word.charAt(0) || 'W').toUpperCase();
 }
 
-export function WorkspaceRailButton(): React.ReactElement | null {
+/** Module-level, so the effect that may call it does not re-run on every render. */
+const reloadPage = () => window.location.reload();
+
+export function WorkspaceRailButton({
+  reload = reloadPage,
+}: {
+  /** Replaced in tests; the app reloads when the editor follows the server. */
+  reload?: () => void;
+} = {}): React.ReactElement | null {
   const launch = useAuthStore((s) => s.launch);
   const [open, setOpen] = useState(false);
   const [list, setList] = useState<WorkspaceList | null>(null);
@@ -43,7 +51,11 @@ export function WorkspaceRailButton(): React.ReactElement | null {
     apiListWorkspaces()
       .then((next) => {
         if (!alive) return;
-        claimEditorWorkspace(next.currentId);
+        // The server decides which workspace is open; the editor follows it.
+        if (followServerWorkspace(next.currentId) === 'moved') {
+          reload();
+          return;
+        }
         setList(next);
       })
       .catch(() => undefined);
@@ -53,7 +65,7 @@ export function WorkspaceRailButton(): React.ReactElement | null {
     return () => {
       alive = false;
     };
-  }, [launch]);
+  }, [launch, reload]);
 
   useEffect(() => {
     if (!open) return;

apps/web/src/frontend/shared/api/accessSql.test.ts

@@ -68,19 +68,21 @@ describe('runAccessSql', () => {
     ).resolves.toEqual({
       ok: false,
       error: 'Statement 2 of 3 failed: role "r" does not exist. The 1 before it was applied. The 1 after it did not run.',
+      applied: 1,
     });
   });
 
   it('reports a lone statement’s error as it is', async () => {
     executeSql.mockResolvedValue({ results: [{ ok: false, error: 'permission denied' }] });
-    await expect(runAccessSql(ref, 'GRANT SELECT ON t TO r;')).resolves.toEqual({ ok: false, error: 'permission denied' });
+    await expect(runAccessSql(ref, 'GRANT SELECT ON t TO r;')).resolves.toEqual({ ok: false, error: 'permission denied', applied: 0 });
   });
 
   it('does not report ok when fewer results came back than statements were sent', async () => {
     executeSql.mockResolvedValue({ results: [{ ok: true }] });
     await expect(runAccessSql(ref, 'GRANT SELECT ON t TO r;\nGRANT INSERT ON t TO r;')).resolves.toEqual({
       ok: false,
       error: 'Only 1 of 2 statements ran.',
+      applied: 1,
     });
   });
 

apps/web/src/frontend/shared/api/accessSql.ts

@@ -20,8 +20,13 @@ import { splitSqlStatements } from '@foxschema/ui-shared';
 import type { ConnectionRef } from './schemaApi';
 import { executeSql } from './sqlApi';
 
-/** Every statement succeeded, or which one failed and what ran around it. */
-export type AccessSqlOutcome = { ok: true } | { ok: false; error: string };
+/**
+ * Every statement succeeded, or which one failed and what ran around it.
+ * `applied` counts the statements that ran before the failure and stay
+ * applied — a caller re-reads the catalog when it is above zero, or it shows
+ * grants as they were before part of the script landed.
+ */
+export type AccessSqlOutcome = { ok: true } | { ok: false; error: string; applied: number };
 
 /**
  * Execute a grant/revoke script statement by statement.
@@ -46,14 +51,18 @@ export async function runAccessSql(
   const failedAt = results.findIndex((r) => !r.ok);
   if (failedAt === -1 && results.length === statements.length) return { ok: true };
   if (failedAt === -1) {
-    return { ok: false, error: `Only ${results.length} of ${statements.length} statements ran.` };
+    return { ok: false, error: `Only ${results.length} of ${statements.length} statements ran.`, applied: results.length };
   }
   const failure = results[failedAt]!;
   const error = 'error' in failure ? failure.error : 'failed';
-  if (statements.length === 1) return { ok: false, error };
+  if (statements.length === 1) return { ok: false, error, applied: 0 };
   const notRun = statements.length - failedAt - 1;
   const before = failedAt === 0 ? '' : ` The ${failedAt} before it ${failedAt === 1 ? 'was' : 'were'} applied.`;
   const after = notRun === 0 ? '' : ` The ${notRun} after it did not run.`;
   const reason = error.trim().replace(/\.$/, '');
-  return { ok: false, error: `Statement ${failedAt + 1} of ${statements.length} failed: ${reason}.${before}${after}` };
+  return {
+    ok: false,
+    error: `Statement ${failedAt + 1} of ${statements.length} failed: ${reason}.${before}${after}`,
+    applied: failedAt,
+  };
 }

apps/web/src/frontend/shared/lib/editorWorkspaceStorage.test.ts

@@ -10,6 +10,7 @@ import {
   activeEditorWorkspaceId,
   claimEditorWorkspace,
   editorStorageKey,
+  followServerWorkspace,
   prepareWorkspaceSwitch,
 } from './editorWorkspaceStorage';
 
@@ -50,3 +51,31 @@ describe('editor workspace storage', () => {
     expect(activeEditorWorkspaceId()).toBe('ws-a');
   });
 });
+
+describe('following the workspace the server opened', () => {
+  it('claims on the first run, and is a no-op while they agree', () => {
+    localStorage.setItem(SQL_EDITOR_PERSIST_KEY, snapshot('select 1'));
+    expect(followServerWorkspace('ws-a')).toBe('claimed');
+    expect(editorStorageKey()).toBe(`${SQL_EDITOR_PERSIST_KEY}:ws-a`);
+    expect(followServerWorkspace('ws-a')).toBe('same');
+  });
+
+  it('moves to the server’s workspace when it is not the one this browser last used', () => {
+    // Switched in another browser, taken out of ws-a, or another account
+    // signed in here: the editor must not keep showing — and saving into — ws-a.
+    claimEditorWorkspace('ws-a');
+    localStorage.setItem(`${SQL_EDITOR_PERSIST_KEY}:ws-a`, snapshot('a secret query'));
+    expect(followServerWorkspace('ws-b')).toBe('moved');
+    expect(activeEditorWorkspaceId()).toBe('ws-b');
+    expect(editorStorageKey()).toBe(`${SQL_EDITOR_PERSIST_KEY}:ws-b`);
+    // ws-a keeps its scripts; nothing was copied across.
+    expect(localStorage.getItem(`${SQL_EDITOR_PERSIST_KEY}:ws-a`)).toBe(snapshot('a secret query'));
+    expect(localStorage.getItem(`${SQL_EDITOR_PERSIST_KEY}:ws-b`)).toBeNull();
+  });
+
+  it('ignores an empty id rather than unscoping the editor', () => {
+    claimEditorWorkspace('ws-a');
+    expect(followServerWorkspace('  ')).toBe('same');
+    expect(activeEditorWorkspaceId()).toBe('ws-a');
+  });
+});

apps/web/src/frontend/shared/lib/editorWorkspaceStorage.ts

@@ -87,6 +87,34 @@ export function claimEditorWorkspace(workspaceId: string): void {
   write(ACTIVE_WORKSPACE_KEY, id);
 }
 
+/**
+ * Keep the editor on the workspace the server opened, which is not always the
+ * one this browser last used: a switch in another browser or tab, being taken
+ * out of a workspace, or another account signing in here all move the server
+ * without moving the pointer. The editor hydrated from the pointer, so on a
+ * mismatch it was showing — and saving into — another workspace's scripts.
+ *
+ * `'claimed'`: first run, the original blob now belongs to `workspaceId` and
+ * is what the editor already holds. `'moved'`: the pointer now names
+ * `workspaceId`; the caller reloads, so the editor hydrates from it rather
+ * than writing what it holds into the wrong workspace. `'same'`: nothing to do.
+ */
+export function followServerWorkspace(workspaceId: string): 'same' | 'claimed' | 'moved' {
+  const id = workspaceId.trim();
+  if (!id) return 'same';
+  const active = activeEditorWorkspaceId();
+  if (!active) {
+    claimEditorWorkspace(id);
+    return 'claimed';
+  }
+  if (active === id) return 'same';
+  // Pending writes are keyed when made, so they still land in the workspace
+  // they were typed in.
+  flushDeferredStorage();
+  write(ACTIVE_WORKSPACE_KEY, id);
+  return 'moved';
+}
+
 /**
  * Point the next page load at `nextId`, after the current blob is saved.
  *

apps/web/src/frontend/shared/lib/provider-settings.ts

@@ -33,6 +33,7 @@ export {
   withConnectionString,
   isFileDialect,
   dialectFamily,
+  dialectHasTransactions,
   DEFAULT_PORTS,
   connectionNeedsSecret,
   authMethodsForDialect,

apps/web/src/frontend/shared/lib/sql-splitter.ts

@@ -11,6 +11,7 @@ export {
   isMutatingDmlStatement,
   isInsertWriteStatement,
   isPageableStatement,
+  isTransactionControl,
   dmlLacksWhere,
   parseCodeCell,
   findCodeFences,