fix(editor): Restore MCP connection popup retries (#37956)

This commit is contained in:
Predrag Ristic
2026-09-15 10:05:51 +00:00
committed by GitHub
parent dba75ffb25
commit 4d9cd43168
10 changed files with 636 additions and 123 deletions
@@ -14,6 +14,7 @@ const {
ensureBrowserConnected,
computerUseTelemetry,
featureFlags,
ignorePendingConnectResult,
mcpStore,
mcpTelemetry,
settingsStore,
@@ -23,6 +24,7 @@ const {
ensureBrowserConnected: vi.fn(),
computerUseTelemetry: { trackModalOpened: vi.fn() },
featureFlags: { browserUse: true, computerUse: true, mcp: true },
ignorePendingConnectResult: vi.fn(),
mcpStore: {
connections: [] as Array<Record<string, unknown>>,
fetchConnectionsLazy: vi.fn(),
@@ -102,6 +104,10 @@ vi.mock('../instanceAiMcp.store', () => ({
useInstanceAiMcpStore: () => mcpStore,
}));
vi.mock('../composables/useMcpServerConnect', () => ({
useMcpServerConnect: () => ({ ignorePendingConnectResult }),
}));
vi.mock('../instanceAiMcp.telemetry', () => ({
useInstanceAiMcpTelemetry: () => mcpTelemetry,
}));
@@ -292,6 +298,10 @@ describe('useInstanceAiInputMenuItems', () => {
data: { connectionId: '1' },
});
expect(mcpStore.disconnect).toHaveBeenCalledWith('1');
expect(ignorePendingConnectResult).toHaveBeenCalledWith('server-1');
expect(ignorePendingConnectResult.mock.invocationCallOrder[0]).toBeLessThan(
mcpStore.disconnect.mock.invocationCallOrder[0] ?? 0,
);
expect(settingsStore.disconnectComputerUse).toHaveBeenCalledOnce();
expect(ensureBrowserConnected).toHaveBeenCalledWith('input_menu');
});
@@ -9,7 +9,6 @@ import { useCredentialsStore } from '@/features/credentials/credentials.store';
import ToolCredentialPicker from '@/features/shared/toolsConnection/ToolCredentialPicker.vue';
import {
TOOL_CONNECTION_CREDENTIAL_ADAPTER_KEY,
hasToolConnection,
type McpServerConnectionItem,
type ToolCredentialRef,
} from '@/features/shared/toolsConnection/types';
@@ -35,7 +34,13 @@ const uiStore = useUIStore();
const credentialsStore = useCredentialsStore();
const mcpStore = useInstanceAiMcpStore();
const mcpTelemetry = useInstanceAiMcpTelemetry();
const { connectServer, connectWithCredential, createCredentialAdapter } = useMcpServerConnect();
const {
connectServer,
connectWithCredential,
createCredentialAdapter,
ignorePendingConnectResult,
isConnectLocked,
} = useMcpServerConnect();
const isConnecting = ref(false);
@@ -80,7 +85,7 @@ const rows = computed<CardRow[]>(() =>
id: connection?.id ?? server.serverSlug,
kind: 'mcp-server',
title: entry?.title ?? server.title,
status: connection?.status ?? 'none',
status: isConnectLocked(server.serverSlug) ? 'connecting' : (connection?.status ?? 'none'),
credentials: credentialOptions.map(({ credentialType, name }) => ({
authType: credentialType,
displayName: name,
@@ -98,7 +103,7 @@ const isActionable = computed(() => !props.readOnly && !props.expired);
const anyConnected = computed(() => rows.value.some(hasConnection));
function hasConnection(row: CardRow): boolean {
return hasToolConnection(row.item.status);
return mcpStore.connections.some((connection) => connection.serverSlug === row.serverSlug);
}
function finish(approved: boolean) {
@@ -140,18 +145,12 @@ async function runConnect(attempt: () => Promise<unknown>) {
}
async function connect(row: CardRow, credentialType: string, credentialTypes?: readonly string[]) {
await runConnect(
async () =>
await connectServer({
slug: row.serverSlug,
credentialType,
credentialTypes,
}),
);
await connectServer({ slug: row.serverSlug, credentialType, credentialTypes });
}
async function handleSelectCredential(row: CardRow, credentialId: string) {
await runConnect(async () => {
ignorePendingConnectResult(row.serverSlug);
mcpTelemetry.trackExistingCredentialSelected(row.serverSlug);
return await connectWithCredential(row.serverSlug, credentialId);
});
@@ -23,6 +23,8 @@ const {
uiStoreMock,
connectServerMock,
connectWithCredentialMock,
ignorePendingConnectResultMock,
isConnectLockedMock,
credentialsMock,
} = vi.hoisted(() => ({
telemetryMock: {
@@ -42,6 +44,8 @@ const {
},
connectServerMock: vi.fn(),
connectWithCredentialMock: vi.fn(),
ignorePendingConnectResultMock: vi.fn(),
isConnectLockedMock: vi.fn(),
credentialsMock: {
credentials: [] as Array<{ id: string; name: string; type: string }>,
},
@@ -66,6 +70,8 @@ vi.mock('../../composables/useMcpServerConnect', () => ({
useMcpServerConnect: () => ({
connectServer: connectServerMock,
connectWithCredential: connectWithCredentialMock,
ignorePendingConnectResult: ignorePendingConnectResultMock,
isConnectLocked: isConnectLockedMock,
createCredentialAdapter: (
openNewCredential: ToolConnectionCredentialAdapter['openNewCredential'],
) => ({
@@ -124,6 +130,7 @@ describe('InstanceAiMcpConnectCard', () => {
beforeEach(() => {
vi.clearAllMocks();
credentialsMock.credentials = [];
isConnectLockedMock.mockReturnValue(false);
setActivePinia(createTestingPinia({ stubActions: false }));
mcpStoreMock.mockReturnValue(makeMcpStore());
});
@@ -155,6 +162,7 @@ describe('InstanceAiMcpConnectCard', () => {
await nextTick();
await fireEvent.click(getByTestId('tool-credential-picker-trigger-connect'));
await nextTick();
expect(telemetryMock.trackFirstCredentialConnectionStart).toHaveBeenCalledWith('brave');
expect(connectServerMock).toHaveBeenCalledWith({
@@ -241,6 +249,20 @@ describe('InstanceAiMcpConnectCard', () => {
expect(emitted().resolve).toBeUndefined();
});
it('shows an external connection lock as noninteractive', async () => {
isConnectLockedMock.mockReturnValue(true);
const { emitted, getByTestId, getByText, queryByTestId } = renderComponent({
props: { servers: [BRAVE_PAYLOAD] },
});
expect(getByTestId('tool-credential-picker-trigger-connecting')).toBeVisible();
expect(queryByTestId('tool-credential-picker-trigger-connect')).toBeNull();
await fireEvent.click(getByText('Brave Search'));
expect(uiStoreMock.openModalWithData).not.toHaveBeenCalled();
expect(emitted().resolve).toBeUndefined();
});
it('resolves as unapproved when skipped', async () => {
const { getByTestId, emitted } = renderComponent({ props: { servers: [BRAVE_PAYLOAD] } });
@@ -249,8 +271,8 @@ describe('InstanceAiMcpConnectCard', () => {
expect(emitted().resolve).toEqual([[{ approved: false, connectedSlugs: [] }]]);
});
it('continues as approved with what was connected when some rows are left', async () => {
mcpStoreMock.mockReturnValue(makeMcpStore({ connections: [BRAVE_CONNECTION] }));
it.each(['connected', 'connecting'])('continues with a %s connection', async (status) => {
mcpStoreMock.mockReturnValue(makeMcpStore({ connections: [{ ...BRAVE_CONNECTION, status }] }));
const { getByTestId, emitted } = renderComponent({
props: {
servers: [
@@ -403,7 +425,11 @@ describe('InstanceAiMcpConnectCard', () => {
await findAllByTestId('tool-credential-picker-row');
await fireEvent.click(getByText('Brave key'));
expect(ignorePendingConnectResultMock).toHaveBeenCalledWith('brave');
expect(connectWithCredentialMock).toHaveBeenCalledWith('brave', 'cred-1');
expect(ignorePendingConnectResultMock.mock.invocationCallOrder[0]).toBeLessThan(
connectWithCredentialMock.mock.invocationCallOrder[0] ?? 0,
);
expect(connectServerMock).not.toHaveBeenCalled();
});
@@ -435,7 +461,7 @@ describe('InstanceAiMcpConnectCard', () => {
});
});
it('keeps the footer disabled while a connect is still in flight', async () => {
it('allows another attempt after the server lock clears', async () => {
mcpStoreMock.mockReturnValue(
makeMcpStore({
catalog: [
@@ -455,9 +481,14 @@ describe('InstanceAiMcpConnectCard', () => {
credentialsMock.credentials = [
{ id: 'cred-1', name: 'Brave key', type: 'braveMcpOAuth2Api' },
];
connectServerMock.mockReturnValue(new Promise(() => {}));
const lockedSlugs = reactive(new Set<string>());
isConnectLockedMock.mockImplementation((slug: string) => lockedSlugs.has(slug));
connectServerMock.mockImplementation(({ slug }: { slug: string }) => {
lockedSlugs.add(slug);
return new Promise(() => {});
});
const { getAllByTestId, getByTestId, getByText, findAllByTestId } = renderComponent({
const { getAllByTestId } = renderComponent({
props: {
servers: [
BRAVE_PAYLOAD,
@@ -473,14 +504,15 @@ describe('InstanceAiMcpConnectCard', () => {
});
await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[1]);
expect(getByTestId('instance-ai-mcp-connect-resolve')).toBeDisabled();
expect(getAllByTestId('tool-credential-picker-trigger-connect')).toHaveLength(1);
expect(getAllByTestId('tool-credential-picker-trigger-connecting')).toHaveLength(1);
await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[0]);
await findAllByTestId('tool-credential-picker-row');
await fireEvent.click(getByText('Brave key'));
lockedSlugs.clear();
await nextTick();
await fireEvent.click(getAllByTestId('tool-credential-picker-trigger-connect')[1]);
expect(connectServerMock).toHaveBeenCalledTimes(2);
expect(connectWithCredentialMock).not.toHaveBeenCalled();
expect(getByTestId('instance-ai-mcp-connect-resolve')).toBeDisabled();
});
it('opens the connection settings on row click', async () => {
@@ -12,7 +12,6 @@ import McpToolSettingsContent from '@/features/shared/toolsConnection/McpToolSet
import ToolsConnectionModal from '@/features/shared/toolsConnection/ToolsConnectionModal.vue';
import McpRegistrySuggestionFooter from '@/app/components/McpRegistrySuggestionFooter.vue';
import {
hasToolConnection,
TOOL_CONNECTION_CREDENTIAL_ADAPTER_KEY,
type McpServerConnectionItem,
type McpServerTool,
@@ -111,14 +110,21 @@ const detailItem = computed<ToolConnectionItem | null>(() => {
});
const detailMode = computed<'detail' | 'settings'>(() =>
detailItem.value?.kind === 'mcp-server' && hasToolConnection(detailItem.value.status)
detailItem.value?.kind === 'mcp-server' &&
mcpStore.connections.some((connection) => connection.id === activeItemId.value)
? 'settings'
: 'detail',
);
type McpToolMetadata = McpRegistryServerToolResponse | InstanceAiMcpConnectionToolResponse;
const { connectServer, connectWithCredential, createCredentialAdapter } = useMcpServerConnect();
const {
connectServer,
connectWithCredential,
createCredentialAdapter,
ignorePendingConnectResult,
isConnectLocked,
} = useMcpServerConnect();
/** Reveals the settings view of the server the user just connected */
function showConnectedServer(connectionId: string | null): void {
@@ -206,7 +212,7 @@ function buildItem(
title: server.title,
description: server.tagline,
longDescription: server.description,
status: connection?.status ?? 'none',
status: isConnectLocked(server.slug) ? 'connecting' : (connection?.status ?? 'none'),
iconSource: iconForTool(server.icons, uiStore.appliedTheme),
credentials: server.credentials.map(({ credentialType, name }) => ({
authType: credentialType,
@@ -323,10 +329,14 @@ provide(
}),
);
function findServerForItem(item: McpServerConnectionItem): McpRegistryServerResponse | undefined {
function serverSlugForItem(item: McpServerConnectionItem): string {
const connection = mcpStore.connections.find((c) => c.id === item.id);
const slug = connection?.serverSlug ?? item.id;
return mcpStore.catalog?.find((s) => s.slug === slug);
return connection?.serverSlug ?? item.id;
}
function findServerForItem(item: McpServerConnectionItem): McpRegistryServerResponse | undefined {
const serverSlug = serverSlugForItem(item);
return mcpStore.catalog?.find((server) => server.slug === serverSlug);
}
function trackMcpCredentialInteraction(
@@ -365,6 +375,7 @@ async function handleSelectCredential(
if (item.kind !== 'mcp-server') return;
const server = findServerForItem(item);
if (!server) return;
ignorePendingConnectResult(server.slug);
mcpTelemetry.trackExistingCredentialSelected(server.slug);
showConnectedServer(await connectWithCredential(server.slug, credentialId));
}
@@ -386,6 +397,9 @@ async function handleSave(item: ToolConnectionItem, settings?: ToolConnectionSet
}
async function handleDisconnect(item: ToolConnectionItem) {
if (item.kind === 'mcp-server') {
ignorePendingConnectResult(serverSlugForItem(item));
}
const disconnected = await mcpStore.disconnect(item.id);
if (!disconnected) return;
activeItemId.value = null;
@@ -1,6 +1,7 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { flushPromises } from '@vue/test-utils';
import { defineComponent, nextTick, reactive } from 'vue';
import { fireEvent } from '@testing-library/vue';
import { defineComponent, nextTick, reactive, ref } from 'vue';
import { createComponentRenderer } from '@/__tests__/render';
import { CREDENTIAL_EDIT_MODAL_KEY } from '@/features/credentials/credentials.constants';
import InstanceAiToolsConnectionModalWrapper from '../InstanceAiToolsConnectionModalWrapper.vue';
@@ -42,12 +43,24 @@ vi.mock('@/experiments/instanceAiBrowserUse', () => ({
}),
}));
const { mockConnect, mockUpdateConnection, mcpStoreMock } = vi.hoisted(() => {
const {
mockConnect,
mockConnectServer,
mockConnectWithCredential,
mockIgnorePendingConnectResult,
mockUpdateConnection,
mockIsConnectLocked,
mcpStoreMock,
} = vi.hoisted(() => {
const mockConnect = vi.fn();
const mockUpdateConnection = vi.fn();
return {
mockConnect,
mockConnectServer: vi.fn(),
mockConnectWithCredential: vi.fn(),
mockIgnorePendingConnectResult: vi.fn(),
mockUpdateConnection,
mockIsConnectLocked: vi.fn(),
mcpStoreMock: {
connections: [] as Array<{
id: string;
@@ -89,8 +102,10 @@ vi.mock('../../../instanceAiMcp.store', () => ({
vi.mock('../../../composables/useMcpServerConnect', () => ({
useMcpServerConnect: () => ({
connectServer: vi.fn().mockResolvedValue(null),
connectWithCredential: vi.fn().mockResolvedValue(null),
connectServer: mockConnectServer,
connectWithCredential: mockConnectWithCredential,
ignorePendingConnectResult: mockIgnorePendingConnectResult,
isConnectLocked: mockIsConnectLocked,
createCredentialAdapter: (
openNewCredential: ToolConnectionCredentialAdapter['openNewCredential'],
) => ({
@@ -289,7 +304,10 @@ describe('InstanceAiToolsConnectionModalWrapper', () => {
uiStoreMock.modalsById.instanceAiToolsConnection.data = {};
delete uiStoreMock.modalsById[CREDENTIAL_EDIT_MODAL_KEY];
mockConnect.mockResolvedValue(null);
mockConnectServer.mockResolvedValue(null);
mockConnectWithCredential.mockResolvedValue(null);
mockUpdateConnection.mockResolvedValue({ serverSlug: 'linear' });
mockIsConnectLocked.mockReturnValue(false);
});
it('configures the suggestion footer copy', () => {
@@ -358,6 +376,57 @@ describe('InstanceAiToolsConnectionModalWrapper', () => {
expect(mcpStoreMock.fetchConnectionToolsLazy).toHaveBeenCalledWith('conn-1');
});
it('keeps unsaved settings while the connection is locked', async () => {
const isLocked = ref(false);
mockIsConnectLocked.mockImplementation(() => isLocked.value);
const connection = {
id: 'conn-1',
serverSlug: 'linear',
credentialId: 'cred-1',
status: 'connected' as const,
toolFilter: null,
};
mcpStoreMock.connections = [connection];
mcpStoreMock.connectionsByServerSlug = new Map([['linear', [connection]]]);
uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'conn-1' };
const { getByTestId, getByText } = renderComponent({
global: {
stubs: {
ToolsConnectionModal: false,
N8nDialog: { template: '<div><slot /></div>' },
},
},
});
await flushPromises();
const inclusionInput = () =>
getByTestId('tools-connection-settings-inclusion').querySelector('input')!;
await fireEvent.click(inclusionInput());
await fireEvent.click(getByText('All Except'));
isLocked.value = true;
await nextTick();
expect(getByTestId('tool-credential-picker-trigger-connecting')).toBeVisible();
expect(inclusionInput()).toHaveValue('All Except');
isLocked.value = false;
await nextTick();
expect(getByTestId('tool-credential-picker-trigger-connected')).toBeVisible();
expect(inclusionInput()).toHaveValue('All Except');
});
it('keeps a new connection in the detail view while the connection is locked', () => {
mockIsConnectLocked.mockReturnValue(true);
uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'linear' };
renderComponent();
expect(modalProps.detailItem).toMatchObject({ id: 'linear', status: 'connecting' });
expect(modalProps.detailMode).toBe('detail');
});
it('hides and restores the selected connection while editing a credential', async () => {
uiStoreMock.modalsById.instanceAiToolsConnection.data = { connectionId: 'linear' };
renderComponent();
@@ -425,6 +494,11 @@ describe('InstanceAiToolsConnectionModalWrapper', () => {
await flushPromises();
expect(telemetryMock.trackExistingCredentialSelected).toHaveBeenCalledWith('linear');
expect(mockIgnorePendingConnectResult).toHaveBeenCalledWith('linear');
expect(mockConnectWithCredential).toHaveBeenCalledWith('linear', 'cred-1');
expect(mockIgnorePendingConnectResult.mock.invocationCallOrder[0]).toBeLessThan(
mockConnectWithCredential.mock.invocationCallOrder[0] ?? 0,
);
});
it('tracks new credential connection start', () => {
@@ -15,6 +15,7 @@ import { useInstanceAiMcpTelemetry } from '../instanceAiMcp.telemetry';
import { useInstanceAiComputerUseTelemetry } from '../instanceAiComputerUse.telemetry';
import { useInstanceAiSettingsStore } from '../instanceAiSettings.store';
import { useBrowserUseConnection } from './useBrowserUseConnection';
import { useMcpServerConnect } from './useMcpServerConnect';
import { iconForTool } from '../toolIcons';
type InputMenuItemData = {
@@ -30,6 +31,7 @@ export function useInstanceAiInputMenuItems(attachFiles: () => void) {
const uiStore = useUIStore();
const settingsStore = useInstanceAiSettingsStore();
const mcpStore = useInstanceAiMcpStore();
const { ignorePendingConnectResult } = useMcpServerConnect();
const mcpTelemetry = useInstanceAiMcpTelemetry();
const { ensureConnected: ensureBrowserConnected } = useBrowserUseConnection();
const computerUseTelemetry = useInstanceAiComputerUseTelemetry();
@@ -195,6 +197,7 @@ export function useInstanceAiInputMenuItems(attachFiles: () => void) {
divided: true,
data: {
action: async () => {
ignorePendingConnectResult(connection.serverSlug);
await mcpStore.disconnect(connection.id);
},
},
@@ -91,6 +91,7 @@ describe('useMcpServerConnect', () => {
});
afterEach(async () => {
vi.useRealTimers();
await closeCredentialModal();
});
@@ -154,28 +155,157 @@ describe('useMcpServerConnect', () => {
await expect(useMcpServerConnect().connectServer(linear)).resolves.toBe('conn-new');
expect(mockCreateAndAuthorize).toHaveBeenCalledWith('linearMcpOAuth2Api');
expect(mockCreateAndAuthorize).toHaveBeenCalledWith('linearMcpOAuth2Api', undefined, {
onAuthorizationStarted: expect.any(Function),
});
expect(uiStore.modalsById[CREDENTIAL_EDIT_MODAL_KEY].open).toBe(false);
});
it('shares one attempt across concurrent quick-connect callers', async () => {
it('reopens one OAuth attempt after the two-second lock', async () => {
vi.useFakeTimers();
let finishAttempt: ((credential: { id: string }) => void) | undefined;
const reopen = vi.fn();
mockCanQuickConnect.mockReturnValue(true);
let authorize!: (credential: { id: string }) => void;
mockCreateAndAuthorize
.mockReturnValueOnce(
new Promise<{ id: string }>((resolve) => {
finishAttempt = resolve;
}),
)
.mockResolvedValue(null);
const { connectServer, isConnectLocked } = useMcpServerConnect();
const connecting = connectServer(linear);
mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen);
const concurrent = connectServer(linear);
expect(reopen).not.toHaveBeenCalled();
await vi.advanceTimersByTimeAsync(1999);
void connectServer(linear);
expect(reopen).not.toHaveBeenCalled();
await vi.advanceTimersByTimeAsync(1);
void connectServer(linear);
expect(reopen).toHaveBeenCalledOnce();
expect(isConnectLocked(linear.slug)).toBe(true);
void connectServer(linear);
expect(reopen).toHaveBeenCalledOnce();
await vi.advanceTimersByTimeAsync(2000);
void connectServer(linear);
expect(reopen).toHaveBeenCalledTimes(2);
expect(mockCreateAndAuthorize).toHaveBeenCalledOnce();
finishAttempt?.({ id: 'cred-new' });
await expect(connecting).resolves.toBe('conn-new');
await expect(concurrent).resolves.toBe('conn-new');
expect(mcpStore.connect).toHaveBeenCalledOnce();
expect(isConnectLocked(linear.slug)).toBe(false);
await connectServer(linear);
expect(mockCreateAndAuthorize).toHaveBeenCalledTimes(2);
});
it('ignores the OAuth result after another credential wins', async () => {
mockCanQuickConnect.mockReturnValue(true);
let finishAttempt: ((credential: { id: string }) => void) | undefined;
mockCreateAndAuthorize.mockReturnValue(
new Promise<{ id: string }>((resolve) => {
authorize = resolve;
finishAttempt = resolve;
}),
);
const { connectServer } = useMcpServerConnect();
const { connectServer, ignorePendingConnectResult } = useMcpServerConnect();
const connecting = connectServer(linear);
const first = connectServer(linear);
const second = connectServer(linear);
authorize({ id: 'cred-new' });
ignorePendingConnectResult(linear.slug);
finishAttempt?.({ id: 'cred-new' });
await expect(first).resolves.toBe('conn-new');
await expect(second).resolves.toBe('conn-new');
expect(mockCreateAndAuthorize).toHaveBeenCalledTimes(1);
expect(mcpStore.connect).toHaveBeenCalledTimes(1);
await expect(connecting).resolves.toBeNull();
expect(mcpStore.connect).not.toHaveBeenCalled();
});
it('keeps the credential request locked when an ignored OAuth attempt settles', async () => {
let finishAuthorization: ((credential: { id: string }) => void) | undefined;
let finishConnection: ((connection: InstanceAiMcpConnection) => void) | undefined;
const reopen = vi.fn();
mockCanQuickConnect.mockReturnValue(true);
mockCreateAndAuthorize.mockReturnValue(
new Promise<{ id: string }>((resolve) => {
finishAuthorization = resolve;
}),
);
mcpStore.connect.mockReturnValue(
new Promise<InstanceAiMcpConnection>((resolve) => {
finishConnection = resolve;
}),
);
const { connectServer, connectWithCredential, ignorePendingConnectResult, isConnectLocked } =
useMcpServerConnect();
const oauthAttempt = connectServer(linear);
mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen);
ignorePendingConnectResult(linear.slug);
const credentialConnect = connectWithCredential(linear.slug, 'cred-existing');
expect(isConnectLocked(linear.slug)).toBe(true);
const concurrent = connectServer(linear);
expect(reopen).not.toHaveBeenCalled();
finishAuthorization?.({ id: 'cred-oauth' });
await expect(oauthAttempt).resolves.toBeNull();
await expect(concurrent).resolves.toBeNull();
expect(isConnectLocked(linear.slug)).toBe(true);
expect(mcpStore.connect).toHaveBeenCalledOnce();
finishConnection?.(makeConnection({ id: 'conn-new', credentialId: 'cred-existing' }));
await expect(credentialConnect).resolves.toBe('conn-new');
expect(isConnectLocked(linear.slug)).toBe(false);
});
it('resumes the ignored OAuth attempt instead of starting another one', async () => {
mockCanQuickConnect.mockReturnValue(true);
let finishAttempt: ((credential: { id: string }) => void) | undefined;
const reopen = vi.fn();
mockCreateAndAuthorize.mockReturnValue(
new Promise<{ id: string }>((resolve) => {
finishAttempt = resolve;
}),
);
const { connectServer, ignorePendingConnectResult } = useMcpServerConnect();
const connecting = connectServer(linear);
mockCreateAndAuthorize.mock.calls[0]?.[2]?.onAuthorizationStarted(reopen);
ignorePendingConnectResult(linear.slug);
const resumed = connectServer(linear);
expect(reopen).toHaveBeenCalledOnce();
expect(mockCreateAndAuthorize).toHaveBeenCalledOnce();
finishAttempt?.({ id: 'cred-new' });
await expect(connecting).resolves.toBe('conn-new');
await expect(resumed).resolves.toBe('conn-new');
expect(mcpStore.connect).toHaveBeenCalledOnce();
});
it('keeps the server locked while the MCP connection request is pending', async () => {
vi.useFakeTimers();
let finishConnection: ((connection: InstanceAiMcpConnection) => void) | undefined;
mockCanQuickConnect.mockReturnValue(true);
mockCreateAndAuthorize.mockResolvedValue({ id: 'cred-new' });
mcpStore.connect.mockReturnValue(
new Promise<InstanceAiMcpConnection>((resolve) => {
finishConnection = resolve;
}),
);
const { connectServer, isConnectLocked } = useMcpServerConnect();
const connecting = connectServer(linear);
await flushPromises();
await vi.advanceTimersByTimeAsync(5000);
expect(isConnectLocked(linear.slug)).toBe(true);
const concurrent = connectServer(linear);
expect(mockCreateAndAuthorize).toHaveBeenCalledOnce();
finishConnection?.(makeConnection({ id: 'conn-new', credentialId: 'cred-new' }));
await expect(connecting).resolves.toBe('conn-new');
await expect(concurrent).resolves.toBe('conn-new');
expect(isConnectLocked(linear.slug)).toBe(false);
});
it('stops when the user aborts the OAuth flow', async () => {
@@ -367,6 +497,21 @@ describe('useMcpServerConnect', () => {
await expect(second).resolves.toBe('conn-new');
});
it('keeps sharing a manual credential attempt after two seconds', async () => {
vi.useFakeTimers();
const { connectServer } = useMcpServerConnect();
const first = connectServer(linear);
await vi.advanceTimersByTimeAsync(2000);
const second = connectServer(linear);
expect(credentialCreatedListeners.size).toBe(1);
emitCredentialCreated('cred-new');
await closeCredentialModal();
await expect(first).resolves.toBe('conn-new');
await expect(second).resolves.toBe('conn-new');
});
it('starts a fresh attempt after the previous one settled', async () => {
const { connectServer } = useMcpServerConnect();
const cancelled = connectServer(linear);
@@ -1,8 +1,9 @@
import { effectScope } from 'vue';
import { effectScope, shallowReactive } from 'vue';
import { camelCase } from 'change-case';
import type { INode } from 'n8n-workflow';
import { i18n } from '@n8n/i18n';
import { useToast } from '@n8n/composables/useToast';
import { TIME } from '@/app/constants/durations';
import { listenForModalChanges, useUIStore } from '@/app/stores/ui.store';
import {
listenForCredentialChanges,
@@ -19,7 +20,21 @@ export interface McpConnectTarget {
credentialTypes?: readonly string[];
}
const inFlightConnectsByServerSlug = new Map<string, Promise<string | null>>();
interface McpConnectAttemptState {
acceptCredential: boolean;
reopen: (() => void) | undefined;
unlockTimer: ReturnType<typeof setTimeout> | undefined;
}
interface McpConnectAttempt {
promise: Promise<string | null>;
state: McpConnectAttemptState;
}
const QUICK_CONNECT_LOCK = 2 * TIME.SECOND;
const connectAttemptsByServerSlug = new Map<string, McpConnectAttempt>();
const oauthLockedServerSlugs = shallowReactive(new Set<string>());
const credentialRequestLockedServerSlugs = shallowReactive(new Set<string>());
/**
* The credential half of connecting an MCP server, shared by the tools
@@ -41,27 +56,26 @@ export function useMcpServerConnect() {
serverSlug: string,
credentialId: string,
): Promise<string | null> {
const existing = mcpStore.connections.find((c) => c.serverSlug === serverSlug);
credentialRequestLockedServerSlugs.add(serverSlug);
try {
const existing = mcpStore.connections.find((c) => c.serverSlug === serverSlug);
if (existing?.credentialId === credentialId) return null;
const connection = existing
? await mcpStore.updateConnection(existing.id, { credentialId })
: await mcpStore.connect({ serverSlug, credentialId });
if (!connection) return null;
if (!existing) {
const created = await mcpStore.connect({ serverSlug, credentialId });
if (!created) return null;
toast.showMessage({
type: 'success',
title: i18n.baseText('instanceAi.mcp.success.connect'),
title: i18n.baseText(
existing ? 'instanceAi.mcp.success.changeCredential' : 'instanceAi.mcp.success.connect',
),
});
return created.id;
return connection.id;
} finally {
credentialRequestLockedServerSlugs.delete(serverSlug);
}
if (existing.credentialId === credentialId) return null;
const updated = await mcpStore.updateConnection(existing.id, { credentialId });
if (!updated) return null;
toast.showMessage({
type: 'success',
title: i18n.baseText('instanceAi.mcp.success.changeCredential'),
});
return updated.id;
}
/**
@@ -70,25 +84,82 @@ export function useMcpServerConnect() {
* modal. Resolves once the user is done, with null if they backed out.
*/
async function connectServer(server: McpConnectTarget): Promise<string | null> {
const inFlight = inFlightConnectsByServerSlug.get(server.slug);
if (inFlight) return await inFlight;
const attempt = startConnect(server);
inFlightConnectsByServerSlug.set(server.slug, attempt);
try {
return await attempt;
} finally {
inFlightConnectsByServerSlug.delete(server.slug);
const activeAttempt = connectAttemptsByServerSlug.get(server.slug);
if (activeAttempt) {
if (!isConnectLocked(server.slug) && activeAttempt.state.reopen) {
activeAttempt.state.acceptCredential = true;
activeAttempt.state.reopen();
lockConnect(server.slug, activeAttempt);
}
return await activeAttempt.promise;
}
const hasOneOption = (server.credentialTypes?.length ?? 0) <= 1;
const isQuickConnect = hasOneOption && canOAuthCredentialQuickConnect(server.credentialType);
const state: McpConnectAttemptState = {
acceptCredential: true,
reopen: undefined,
unlockTimer: undefined,
};
const connecting = isQuickConnect
? connectViaOAuth(server, state)
: connectViaCredentialModal(server);
const promise = connecting.finally(() => {
if (state.unlockTimer) clearTimeout(state.unlockTimer);
connectAttemptsByServerSlug.delete(server.slug);
oauthLockedServerSlugs.delete(server.slug);
});
const attempt: McpConnectAttempt = {
promise,
state,
};
connectAttemptsByServerSlug.set(server.slug, attempt);
if (isQuickConnect) lockConnect(server.slug, attempt);
return await promise;
}
async function startConnect(server: McpConnectTarget): Promise<string | null> {
const hasOneOption = (server.credentialTypes?.length ?? 0) <= 1;
if (hasOneOption && canOAuthCredentialQuickConnect(server.credentialType)) {
const credential = await createAndAuthorize(server.credentialType);
return credential ? await connectWithCredential(server.slug, credential.id) : null;
function lockConnect(serverSlug: string, attempt: McpConnectAttempt): void {
const { state } = attempt;
if (state.unlockTimer) clearTimeout(state.unlockTimer);
oauthLockedServerSlugs.add(serverSlug);
state.unlockTimer = setTimeout(() => {
oauthLockedServerSlugs.delete(serverSlug);
state.unlockTimer = undefined;
}, QUICK_CONNECT_LOCK);
}
async function connectViaOAuth(
server: McpConnectTarget,
state: McpConnectAttemptState,
): Promise<string | null> {
const credential = await createAndAuthorize(server.credentialType, undefined, {
onAuthorizationStarted: (reopen) => {
state.reopen = reopen;
},
}).finally(() => {
state.reopen = undefined;
});
if (!credential || !state.acceptCredential) return null;
return await connectWithCredential(server.slug, credential.id);
}
function isConnectLocked(serverSlug: string): boolean {
return (
oauthLockedServerSlugs.has(serverSlug) || credentialRequestLockedServerSlugs.has(serverSlug)
);
}
function ignorePendingConnectResult(serverSlug: string): void {
const attempt = connectAttemptsByServerSlug.get(serverSlug);
if (attempt) {
attempt.state.acceptCredential = false;
if (attempt.state.unlockTimer) clearTimeout(attempt.state.unlockTimer);
attempt.state.unlockTimer = undefined;
}
return await connectViaCredentialModal(server);
oauthLockedServerSlugs.delete(serverSlug);
}
function registryContextNode(server: McpConnectTarget): INode {
@@ -215,5 +286,11 @@ export function useMcpServerConnect() {
};
}
return { connectServer, connectWithCredential, createCredentialAdapter };
return {
connectServer,
connectWithCredential,
createCredentialAdapter,
ignorePendingConnectResult,
isConnectLocked,
};
}
@@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { createTestingPinia } from '@pinia/testing';
import { setActivePinia } from 'pinia';
import { useCredentialOAuth } from '../useCredentialOAuth';
import { OAUTH_FLOW_TIMEOUT } from '../oauthCallback';
import { useCredentialsStore } from '../../credentials.store';
import { useRootStore } from '@n8n/stores/useRootStore';
import { mockedStore } from '@/__tests__/utils';
@@ -813,6 +814,7 @@ describe('useCredentialOAuth', () => {
let mockPopup: {
closed: boolean;
close: ReturnType<typeof vi.fn>;
focus: ReturnType<typeof vi.fn>;
location: { href: string };
};
@@ -847,7 +849,8 @@ describe('useCredentialOAuth', () => {
beforeEach(() => {
mockTrack.mockClear();
mockShowError.mockClear();
mockPopup = { closed: false, close: vi.fn(), location: { href: '' } };
mockShowMessage.mockClear();
mockPopup = { closed: false, close: vi.fn(), focus: vi.fn(), location: { href: '' } };
MockBroadcastChannel.silent = false;
mockedStore(useCredentialsStore).deleteCredential.mockResolvedValue();
vi.stubGlobal('BroadcastChannel', MockBroadcastChannel);
@@ -1032,6 +1035,113 @@ describe('useCredentialOAuth', () => {
expect(mockPopup.close).toHaveBeenCalled();
});
it('should expose a handle that focuses the active popup', async () => {
setupSuccessfulOAuthFlow();
let reopen: (() => void) | undefined;
const { createAndAuthorize } = useCredentialOAuth();
const promise = createAndAuthorize('slackOAuth2Api', undefined, {
onAuthorizationStarted: (resume) => {
reopen = resume;
},
});
expect(reopen).toBeTypeOf('function');
reopen?.();
expect(window.open).toHaveBeenCalledOnce();
expect(mockPopup.focus).toHaveBeenCalledOnce();
await promise;
});
it('should navigate a reopened popup when the authorization URL arrives', async () => {
const credentialsStore = setupSuccessfulOAuthFlow();
let resolveAuthorizationUrl: ((url: string) => void) | undefined;
credentialsStore.oAuth2Authorize.mockReturnValueOnce(
new Promise<string>((resolve) => {
resolveAuthorizationUrl = resolve;
}),
);
const reopenedPopup = {
closed: false,
close: vi.fn(),
focus: vi.fn(),
location: { href: '' },
};
vi.stubGlobal(
'open',
vi.fn().mockReturnValueOnce(mockPopup).mockReturnValueOnce(reopenedPopup),
);
let reopen: (() => void) | undefined;
const { createAndAuthorize } = useCredentialOAuth();
const promise = createAndAuthorize('slackOAuth2Api', undefined, {
onAuthorizationStarted: (resume) => {
reopen = resume;
},
});
await vi.waitFor(() => expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledOnce());
mockPopup.closed = true;
reopen?.();
expect(window.open).toHaveBeenLastCalledWith(
'about:blank',
'OAuth Authorization',
expect.any(String),
);
resolveAuthorizationUrl?.('https://oauth.example.com/auth');
await promise;
expect(reopenedPopup.location.href).toBe('https://oauth.example.com/auth');
expect(reopenedPopup.close).toHaveBeenCalledOnce();
expect(credentialsStore.createNewCredential).toHaveBeenCalledOnce();
expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledOnce();
});
it('should refresh authorization and the timeout when a popup is reopened', async () => {
const credentialsStore = setupSuccessfulOAuthFlow();
MockBroadcastChannel.silent = true;
credentialsStore.oAuth2Authorize
.mockResolvedValueOnce('https://oauth.example.com/auth?state=first')
.mockResolvedValueOnce('https://oauth.example.com/auth?state=second');
const reopenedPopup = {
closed: false,
close: vi.fn(),
focus: vi.fn(),
location: { href: '' },
};
vi.stubGlobal('open', vi.fn().mockReturnValueOnce(mockPopup).mockReturnValue(reopenedPopup));
let reopen: (() => void) | undefined;
vi.useFakeTimers();
try {
const { createAndAuthorize } = useCredentialOAuth();
const promise = createAndAuthorize('slackOAuth2Api', undefined, {
onAuthorizationStarted: (resume) => {
reopen = resume;
},
});
await vi.advanceTimersByTimeAsync(OAUTH_FLOW_TIMEOUT - 1000);
mockPopup.closed = true;
reopen?.();
await vi.advanceTimersByTimeAsync(2000);
expect(credentialsStore.oAuth2Authorize).toHaveBeenCalledTimes(2);
expect(reopenedPopup.location.href).toBe('https://oauth.example.com/auth?state=second');
expect(reopenedPopup.close).not.toHaveBeenCalled();
expect(credentialsStore.deleteCredential).not.toHaveBeenCalled();
window.dispatchEvent(
new MessageEvent('message', { origin: window.location.origin, data: 'success' }),
);
await expect(promise).resolves.toEqual(createdCredential);
expect(credentialsStore.createNewCredential).toHaveBeenCalledOnce();
expect(reopenedPopup.close).toHaveBeenCalledOnce();
expect(credentialsStore.deleteCredential).not.toHaveBeenCalled();
} finally {
vi.useRealTimers();
}
});
it('should keep the credential when OAuth succeeds via backend verification (COOP)', async () => {
const credentialsStore = setupSuccessfulOAuthFlow();
// No callback message reaches the editor; the popup reads as closed
@@ -17,11 +17,25 @@ import type { ICredentialsResponse } from '../credentials.types';
import { useTelemetry } from '@n8n/composables/useTelemetry';
import { useWorkflowsStore } from '@/app/stores/workflows.store';
import { useRootStore } from '@n8n/stores/useRootStore';
import { getTrustedOAuthOrigins, hasOAuthTokenData, waitForOAuthCallback } from './oauthCallback';
import {
getTrustedOAuthOrigins,
hasOAuthTokenData,
waitForOAuthCallback,
type OAuthFlowOutcome,
} from './oauthCallback';
interface OAuthPopupState {
window: Window;
onReopen?: () => void;
}
interface OAuthAuthorizationOptions {
abortOnPopupClose?: boolean;
preopenedPopup?: Window;
popup?: OAuthPopupState;
}
interface CreateAndAuthorizeOptions {
onAuthorizationStarted?: (reopen: () => void) => void;
}
/**
@@ -231,7 +245,7 @@ export function useCredentialOAuth() {
* Returns true if OAuth was successful, false if cancelled or failed.
*
* Must be called synchronously from the click handler (or be given a popup
* that was opened synchronously from it, see `preopenedPopup`).
* that was opened synchronously from it, see `options.popup`).
*/
async function authorize(
credential: ICredentialsResponse,
@@ -243,11 +257,12 @@ export function useCredentialOAuth() {
// slow connections (Chrome expires activation after ~5s) and in stricter
// browsers (Safari) regardless of timing. Open a blank window now and
// navigate it once the authorization URL is known.
const popup = options.preopenedPopup ?? openOAuthPopup('about:blank', signal);
if (!popup) {
const popupWindow = options.popup?.window ?? openOAuthPopup('about:blank', signal);
if (!popupWindow) {
showPopupBlockedError();
return false;
}
const popup: OAuthPopupState = options.popup ?? { window: popupWindow };
// Token presence in credential data can only confirm the flow for fixed
// credentials that had no token before the popup opened: a reconnect's old
@@ -256,48 +271,56 @@ export function useCredentialOAuth() {
// data, so presence never changes there.
const canVerifyConnected = !credential.isResolvable && !(await isConnected(credential.id));
const urlResult = await getOAuthAuthorizationUrl(credential);
if (!urlResult.ok) {
popup.close();
if (urlResult.error === 'no-url') showOAuthUrlError();
return false;
}
let outcome: OAuthFlowOutcome;
while (true) {
const urlResult = await getOAuthAuthorizationUrl(credential);
if (!urlResult.ok) {
popup.window.close();
if (urlResult.error === 'no-url') showOAuthUrlError();
return false;
}
if (!isValidHttpUrl(urlResult.result)) {
popup.close();
showOAuthUrlError();
return false;
}
if (!isValidHttpUrl(urlResult.result)) {
popup.window.close();
showOAuthUrlError();
return false;
}
popup.location.href = urlResult.result;
popup.window.location.href = urlResult.result;
const retryController = new AbortController();
popup.onReopen = () => retryController.abort();
try {
outcome = await waitForOAuthCallback({
popup: popup.window,
trustedOrigins: getTrustedOAuthOrigins(rootStore.urlBaseEditor),
signal: signal
? AbortSignal.any([signal, retryController.signal])
: retryController.signal,
verifyConnected: canVerifyConnected
? async () => await isConnected(credential.id)
: undefined,
abortOnPopupClose: options.abortOnPopupClose,
});
} finally {
popup.onReopen = undefined;
}
let outcome = await waitForOAuthCallback({
popup,
trustedOrigins: getTrustedOAuthOrigins(rootStore.urlBaseEditor),
signal,
verifyConnected: canVerifyConnected
? async () => await isConnected(credential.id)
: undefined,
abortOnPopupClose: options.abortOnPopupClose,
});
// The callback can finish while a timeout, retry, or cancellation starts.
if (
(outcome === 'timeout' || outcome === 'aborted') &&
canVerifyConnected &&
(await isConnected(credential.id))
) {
outcome = 'success';
}
// Timeout and abort can race the backend committing the token: authorization
// can legitimately take longer than the timeout, and cancellation is not
// always explicit user intent (NodeCredentials also cancels on unmount).
// Re-check before treating the flow as failed — a wrong failure deletes the
// credential in createAndAuthorize and would resurface "Credential not
// found" on the callback page.
if (
(outcome === 'timeout' || outcome === 'aborted') &&
canVerifyConnected &&
(await isConnected(credential.id))
) {
outcome = 'success';
// Reopening needs fresh server state and a fresh callback timeout.
if (!retryController.signal.aborted || signal?.aborted || outcome !== 'aborted') break;
}
// No-op when the opener relationship was severed by the provider's COOP
// policy; the callback page closes itself in that case.
popup.close();
popup.window.close();
if (outcome === 'success') {
toast.showMessage({
@@ -358,6 +381,7 @@ export function useCredentialOAuth() {
async function createAndAuthorize(
credentialTypeName: string,
nodeType?: string,
options: CreateAndAuthorizeOptions = {},
): Promise<ICredentialsResponse | null> {
const credentialType = credentialsStore.getCredentialTypeByName(credentialTypeName);
if (!credentialType) {
@@ -369,12 +393,35 @@ export function useCredentialOAuth() {
// Opened before the credential-creation round trips so it stays within
// the click's transient user activation (see authorize).
const popup = openOAuthPopup('about:blank', controller.signal);
if (!popup) {
const initialPopup = openOAuthPopup('about:blank', controller.signal);
if (!initialPopup) {
showPopupBlockedError();
oauthAbortController.value = null;
return null;
}
const popup: OAuthPopupState = { window: initialPopup };
let authorizationFinished = false;
options.onAuthorizationStarted?.(() => {
if (authorizationFinished || controller.signal.aborted) return;
try {
if (!popup.window.closed) {
popup.window.focus();
return;
}
} catch {
// A provider can isolate the popup from its opener.
}
const reopenedPopup = openOAuthPopup('about:blank', controller.signal);
if (!reopenedPopup) {
showPopupBlockedError();
return;
}
popup.window = reopenedPopup;
popup.onReopen?.();
reopenedPopup.focus();
});
const data: ICredentialDataDecryptedObject = {};
const allowedHttpRequestDomainsProperty = credentialType.properties.find(
@@ -408,7 +455,7 @@ export function useCredentialOAuth() {
workflow_id: workflowsStore.workflowId,
});
} catch (error) {
popup.close();
popup.window.close();
oauthAbortController.value = null;
toast.showError(error, i18n.baseText('nodeCredentials.showMessage.title'));
return null;
@@ -416,7 +463,9 @@ export function useCredentialOAuth() {
pendingCredentialId.value = credential.id;
const success = await authorize(credential, controller.signal, { preopenedPopup: popup });
const success = await authorize(credential, controller.signal, { popup }).finally(() => {
authorizationFinished = true;
});
oauthAbortController.value = null;
pendingCredentialId.value = null;