From 0cefbaec02339ac83f88cfcc7856649ba61722d8 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 17 Aug 2026 12:04:26 -0400 Subject: [PATCH 1/4] feat: make personal API access keys rotatable The personal access key is the bearer token for the external API v2 and the MCP server, but it was generated once at account creation with no way to change it, so a leaked key could only be dealt with by deleting the user. Adds PATCH /me/accessKey plus a Rotate Access Key control on the Personal API Access Key card, mirroring the existing team ingestion key flow. The route takes no user identifier (the id comes from the session), so it can only rotate the caller's own key, and it is deliberately not exposed on the bearer-authed external API v2. Rotation takes effect immediately and leaves the browser session signed in. Also standardizes APIKeyCopyButton on data-testid, which replaces data-test-id that neither Playwright nor Testing Library queries by default, and splits the duplicated "api-key" value into ingestion-api-key and personal-access-key. --- .changeset/rotatable-personal-access-key.md | 7 + packages/api/src/controllers/user.ts | 12 ++ .../src/routers/api/__tests__/me.int.test.ts | 105 ++++++++++ packages/api/src/routers/api/me.ts | 33 +++- packages/app/src/api.ts | 9 + .../TeamSettings/ApiKeysSection.tsx | 178 +++++++++++++---- .../__tests__/ApiKeysSection.test.tsx | 186 ++++++++++++++++++ packages/app/tests/e2e/features/team.spec.ts | 40 +++- .../app/tests/e2e/page-objects/TeamPage.ts | 38 ++++ packages/common-utils/src/types.ts | 14 ++ 10 files changed, 576 insertions(+), 46 deletions(-) create mode 100644 .changeset/rotatable-personal-access-key.md create mode 100644 packages/api/src/routers/api/__tests__/me.int.test.ts create mode 100644 packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx diff --git a/.changeset/rotatable-personal-access-key.md b/.changeset/rotatable-personal-access-key.md new file mode 100644 index 0000000000..b02d20c71a --- /dev/null +++ b/.changeset/rotatable-personal-access-key.md @@ -0,0 +1,7 @@ +--- +'@hyperdx/api': patch +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +Add a Rotate action for the personal API access key in Team Settings → API & Agents. Previously the personal access key — the bearer token for the external API v2 and the MCP server — was generated once at account creation and could never be changed, so a leaked key could only be remediated by deleting the user. Rotating immediately revokes the previous key, so MCP / AI agent configs, external API v2 clients, Terraform / IaC providers, and CI scripts using the old key must be updated with the new one. Browser sessions are unaffected. diff --git a/packages/api/src/controllers/user.ts b/packages/api/src/controllers/user.ts index 55258c2db6..cde81ea9d8 100644 --- a/packages/api/src/controllers/user.ts +++ b/packages/api/src/controllers/user.ts @@ -1,4 +1,5 @@ import mongoose from 'mongoose'; +import { v4 as uuidv4 } from 'uuid'; import type { ObjectId } from '@/models'; import Alert from '@/models/alert'; @@ -7,6 +8,17 @@ export function findUserByAccessKey(accessKey: string) { return User.findOne({ accessKey }); } +/** + * Rotates a user's personal access key, immediately revoking the previous one. + * + * There is exactly one key per user and no grace period: findUserByAccessKey + * above is hit uncached on every bearer request (see validateUserAccessKey), so + * requests presenting the old key start 401ing the instant this returns. + */ +export function rotateUserAccessKey(userId: string | ObjectId) { + return User.findByIdAndUpdate(userId, { accessKey: uuidv4() }, { new: true }); +} + export function findUserById(id: string) { return User.findById(id); } diff --git a/packages/api/src/routers/api/__tests__/me.int.test.ts b/packages/api/src/routers/api/__tests__/me.int.test.ts new file mode 100644 index 0000000000..b2ad3fa8a1 --- /dev/null +++ b/packages/api/src/routers/api/__tests__/me.int.test.ts @@ -0,0 +1,105 @@ +import { getAgent, getLoggedInAgent, getServer } from '@/fixtures'; +import User from '@/models/user'; + +describe('me router', () => { + const server = getServer(); + + beforeAll(async () => { + await server.start(); + }); + + afterEach(async () => { + await server.clearDBs(); + }); + + afterAll(async () => { + await server.stop(); + }); + + describe('GET /me', () => { + it('returns the calling user', async () => { + const { agent, team, user } = await getLoggedInAgent(server); + + const resp = await agent.get('/me').expect(200); + + expect(resp.body.id).toEqual(user._id.toString()); + expect(resp.body.email).toEqual('fake@deploysentinel.com'); + expect(resp.body.accessKey).toEqual(user.accessKey); + expect(resp.body.team.id).toEqual(team._id.toString()); + }); + + it('rejects an unauthenticated request', async () => { + await getAgent(server).get('/me').expect(401); + }); + }); + + describe('PATCH /me/accessKey', () => { + it('rejects an unauthenticated request', async () => { + // The new verb is covered by the mount-time isUserAuthenticated in + // api-app.ts, not by anything in the handler itself. + await getAgent(server).patch('/me/accessKey').expect(401); + }); + + it('returns a new key and persists it', async () => { + const { agent, user } = await getLoggedInAgent(server); + + const resp = await agent.patch('/me/accessKey').expect(200); + + expect(resp.body.newAccessKey).toEqual(expect.any(String)); + expect(resp.body.newAccessKey).not.toEqual(user.accessKey); + expect((await User.findById(user._id))?.accessKey).toEqual( + resp.body.newAccessKey, + ); + }); + + it('revokes the old key and accepts the new one', async () => { + const { agent, user } = await getLoggedInAgent(server); + const oldAccessKey = user.accessKey; + + // GET /api/v2 is the bearer-authed surface with no rate limiter attached, + // so three sequential calls here are safe. + await agent + .get('/api/v2') + .set('Authorization', `Bearer ${oldAccessKey}`) + .expect(200); + + const { body } = await agent.patch('/me/accessKey').expect(200); + + await agent + .get('/api/v2') + .set('Authorization', `Bearer ${oldAccessKey}`) + .expect(401); + await agent + .get('/api/v2') + .set('Authorization', `Bearer ${body.newAccessKey}`) + .expect(200); + }); + + it('does not sign the user out of their browser session', async () => { + // Session auth never reads accessKey (see isUserAuthenticated), so + // rotating must leave the cookie session intact. + const { agent } = await getLoggedInAgent(server); + + const { body } = await agent.patch('/me/accessKey').expect(200); + + const resp = await agent.get('/me').expect(200); + expect(resp.body.accessKey).toEqual(body.newAccessKey); + }); + + it("does not touch another user's key", async () => { + const { agent, user } = await getLoggedInAgent(server); + // Created directly rather than via /register/password, which is gated to + // the first user. We only read the schema-defaulted accessKey off it. + const other = await User.create({ + email: 'other@deploysentinel.com', + team: user.team, + }); + + await agent.patch('/me/accessKey').expect(200); + + expect((await User.findById(other._id))?.accessKey).toEqual( + other.accessKey, + ); + }); + }); +}); diff --git a/packages/api/src/routers/api/me.ts b/packages/api/src/routers/api/me.ts index b574928528..3586a6f6ac 100644 --- a/packages/api/src/routers/api/me.ts +++ b/packages/api/src/routers/api/me.ts @@ -1,8 +1,12 @@ -import type { MeApiResponse } from '@hyperdx/common-utils/dist/types'; +import type { + MeApiResponse, + RotateAccessKeyApiResponse, +} from '@hyperdx/common-utils/dist/types'; import express from 'express'; import { AI_API_KEY, ANTHROPIC_API_KEY, USAGE_STATS_ENABLED } from '@/config'; import { getTeam } from '@/controllers/team'; +import { rotateUserAccessKey } from '@/controllers/user'; import { Api404Error } from '@/utils/errors'; import { sendJson } from '@/utils/serialization'; @@ -43,4 +47,31 @@ router.get('/', async (req, res: express.Response, next) => { } }); +type RotateAccessKeyExpRes = express.Response; + +// Rotating your own personal access key. The user id comes from the session +// (isUserAuthenticated, applied at mount time in api-app.ts) and never from the +// request, so this route can only ever rotate the caller's own key. +// +// Deliberately NOT mirrored onto the bearer-authed external API v2: `GET /api/v2` +// echoes the caller's accessKey back, so a leaked key that could also rotate +// would let an attacker lock the legitimate owner out of their own tooling. +router.patch('/accessKey', async (req, res: RotateAccessKeyExpRes, next) => { + try { + const userId = req.user?._id; + if (userId == null) { + throw new Api404Error('Request without user found'); + } + + const user = await rotateUserAccessKey(userId); + if (user?.accessKey == null) { + throw new Error(`Failed to rotate access key for user ${userId}`); + } + + return sendJson(res, { newAccessKey: user.accessKey }); + } catch (e) { + next(e); + } +}); + export default router; diff --git a/packages/app/src/api.ts b/packages/app/src/api.ts index a8dea6a600..e6b90a2db8 100644 --- a/packages/app/src/api.ts +++ b/packages/app/src/api.ts @@ -11,6 +11,7 @@ import type { MeApiResponse, PresetDashboard, PresetDashboardFilter, + RotateAccessKeyApiResponse, RotateApiKeyApiResponse, TeamApiResponse, TeamClickHouseSettingsUpdate, @@ -269,6 +270,14 @@ const api = { }).json(), }); }, + useRotatePersonalAccessKey() { + return useMutation({ + mutationFn: async () => + hdxServer(`me/accessKey`, { + method: 'PATCH', + }).json(), + }); + }, useDeleteTeamMember() { return useMutation< { message: string }, diff --git a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx index 248ec5db90..3951e1fa61 100644 --- a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx +++ b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { type ReactNode, useState } from 'react'; import { CopyToClipboard } from 'react-copy-to-clipboard'; import { Box, Button, Card, Divider, Group, Modal, Text } from '@mantine/core'; import { notifications } from '@mantine/notifications'; @@ -28,7 +28,7 @@ function APIKeyCopyButton({ } > -
+
{value}
@@ -36,15 +36,80 @@ function APIKeyCopyButton({ ); } +function RotateKeyConfirmModal({ + opened, + onClose, + onConfirm, + title, + testIdPrefix, + children, +}: { + opened: boolean; + onClose: () => void; + onConfirm: () => void; + title: string; + /** + * Yields `${prefix}-modal`, `-cancel` and `-confirm`. The ingestion flow + * passes `rotate-api-key` to preserve the testids that + * tests/e2e/page-objects/TeamPage.ts already depends on. + */ + testIdPrefix: string; + children: ReactNode; +}) { + return ( + + {title} + + } + > + + {children} + + + + + + + ); +} + export default function ApiKeysSection() { const { data: team, refetch: refetchTeam } = api.useTeam(); - const { data: me, isLoading: isLoadingMe } = api.useMe(); + const { data: me, isLoading: isLoadingMe, refetch: refetchMe } = api.useMe(); const rotateTeamApiKey = api.useRotateTeamApiKey(); + const rotatePersonalAccessKey = api.useRotatePersonalAccessKey(); const hasAdminAccess = true; const [ rotateApiKeyConfirmationModalShow, setRotateApiKeyConfirmationModalShow, ] = useState(false); + const [ + rotateAccessKeyConfirmationModalShow, + setRotateAccessKeyConfirmationModalShow, + ] = useState(false); const rotateTeamApiKeyAction = () => { rotateTeamApiKey.mutate(undefined, { @@ -70,6 +135,27 @@ export default function ApiKeysSection() { setRotateApiKeyConfirmationModalShow(false); }; + const onConfirmRotateAccessKey = () => { + setRotateAccessKeyConfirmationModalShow(false); + rotatePersonalAccessKey.mutate(undefined, { + onSuccess: () => { + notifications.show({ + color: 'green', + message: + 'Revoked your old personal access key and generated a new one.', + }); + refetchMe(); + }, + onError: e => { + notifications.show({ + color: 'red', + message: e.message, + autoClose: 5000, + }); + }, + }); + }; + return ( API Keys @@ -78,7 +164,10 @@ export default function ApiKeysSection() { Ingestion API Key {team?.apiKey && ( - + )} {hasAdminAccess && ( - - - - + + Rotating the API key will invalidate your existing API key and + generate a new one for you. This action is not reversible. + + {!isLoadingMe && me != null && ( Personal API Access Key - + + + + + setRotateAccessKeyConfirmationModalShow(false)} + onConfirm={onConfirmRotateAccessKey} + title="Rotate Personal API Access Key" + testIdPrefix="rotate-access-key" + > + + Rotating your personal access key immediately revokes the + current one and generates a new one. This action is{' '} + not reversible. + + + Anything still using the old key will start failing with 401 + until you update it — MCP / AI agent configs (Claude Code, + Cursor, VS Code, Codex), external API v2 clients, Terraform / + IaC providers, and CI scripts. Your browser session is not + affected; you will stay signed in. + + )} diff --git a/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx new file mode 100644 index 0000000000..8fa0097cb4 --- /dev/null +++ b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx @@ -0,0 +1,186 @@ +import { act, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import api from '@/api'; +import ApiKeysSection from '@/components/TeamSettings/ApiKeysSection'; + +jest.mock('@/api', () => ({ + __esModule: true, + default: { + useMe: jest.fn(), + useTeam: jest.fn(), + useRotateTeamApiKey: jest.fn(), + useRotatePersonalAccessKey: jest.fn(), + }, + hdxServer: jest.fn(), +})); + +// Annotated as the loose `jest.Mock` rather than `jest.mocked(...)`'s exact +// MockedFunction: these hooks return TanStack `UseQueryResult` / +// `UseMutationResult` objects with ~25 members, and the tests only need the +// three the component reads. The loose type keeps `mockReturnValue` at `any` +// so the partial fixtures below need no type assertions. +const mockUseMe: jest.Mock = jest.mocked(api.useMe); +const mockUseTeam: jest.Mock = jest.mocked(api.useTeam); +const mockUseRotateTeamApiKey: jest.Mock = jest.mocked(api.useRotateTeamApiKey); +const mockUseRotatePersonalAccessKey: jest.Mock = jest.mocked( + api.useRotatePersonalAccessKey, +); + +/** Options object the component hands to `mutate`, captured so the tests can + * drive `onSuccess` / `onError` without a real mutation. */ +type MutateOptions = { + onSuccess?: () => void; + onError?: (e: Error) => void; +}; + +let capturedPersonalOptions: MutateOptions | undefined; +const refetchMe = jest.fn(); +const rotatePersonalMutate = jest.fn( + (_vars: undefined, options?: MutateOptions) => { + capturedPersonalOptions = options; + }, +); + +const PERSONAL_MODAL_COPY = /Rotating your personal access key/; + +function setMe(accessKey: string | null, isLoading = false) { + mockUseMe.mockReturnValue({ + data: + accessKey === null + ? null + : { + id: 'u1', + email: 'a@b.com', + accessKey, + name: 'User', + createdAt: '', + }, + isLoading, + refetch: refetchMe, + }); +} + +/** + * Mantine mounts modal content one tick after `opened` flips, so every + * open-the-modal step has to await the content rather than the modal root — + * the root stays in the DOM (empty) the whole time. + */ +async function openPersonalRotateModal( + user: ReturnType, +) { + await user.click(screen.getByTestId('rotate-access-key-button')); + await screen.findByText(PERSONAL_MODAL_COPY); +} + +beforeEach(() => { + jest.clearAllMocks(); + capturedPersonalOptions = undefined; + + setMe('personal_key_abc'); + mockUseTeam.mockReturnValue({ + data: { apiKey: 'ingestion_key_xyz' }, + refetch: jest.fn(), + }); + mockUseRotateTeamApiKey.mockReturnValue({ mutate: jest.fn() }); + mockUseRotatePersonalAccessKey.mockReturnValue({ + mutate: rotatePersonalMutate, + }); +}); + +describe('ApiKeysSection', () => { + // The two keys previously shared dataTestId="api-key" on an attribute + // (`data-test-id`) that no test runner queries by default. Both getByTestId + // calls below throw on duplicates, so this locks in the split. + it('renders the ingestion and personal keys under distinct test ids', () => { + renderWithMantine(); + + expect(screen.getByTestId('ingestion-api-key')).toHaveTextContent( + 'ingestion_key_xyz', + ); + expect(screen.getByTestId('personal-access-key')).toHaveTextContent( + 'personal_key_abc', + ); + }); + + it('opens the personal rotate modal with the breakage warning', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + + const modal = screen.getByTestId('rotate-access-key-modal'); + expect(modal).toHaveTextContent(/not reversible/); + expect(modal).toHaveTextContent(/MCP/); + expect(modal).toHaveTextContent(/stay signed in/); + // The ingestion modal must not have opened alongside it. + expect( + screen.queryByText(/Rotating the API key will invalidate/), + ).not.toBeInTheDocument(); + }); + + it('closes the personal rotate modal on cancel without mutating', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + await user.click(screen.getByTestId('rotate-access-key-cancel')); + + expect(rotatePersonalMutate).not.toHaveBeenCalled(); + await waitFor(() => + expect(screen.queryByText(PERSONAL_MODAL_COPY)).not.toBeInTheDocument(), + ); + }); + + it('rotates once on confirm and closes the modal', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + await user.click(screen.getByTestId('rotate-access-key-confirm')); + + expect(rotatePersonalMutate).toHaveBeenCalledTimes(1); + await waitFor(() => + expect(screen.queryByText(PERSONAL_MODAL_COPY)).not.toBeInTheDocument(), + ); + }); + + it('refetches me and notifies on a successful rotation', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + await user.click(screen.getByTestId('rotate-access-key-confirm')); + act(() => capturedPersonalOptions?.onSuccess?.()); + + expect(refetchMe).toHaveBeenCalledTimes(1); + expect( + await screen.findByText(/Revoked your old personal access key/), + ).toBeInTheDocument(); + }); + + it('surfaces the error message when rotation fails', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + await user.click(screen.getByTestId('rotate-access-key-confirm')); + act(() => capturedPersonalOptions?.onError?.(new Error('rotate blew up'))); + + expect(refetchMe).not.toHaveBeenCalled(); + expect(await screen.findByText('rotate blew up')).toBeInTheDocument(); + }); + + it('renders neither the personal key nor its rotate button when me is null', () => { + setMe(null); + + renderWithMantine(); + + expect(screen.queryByTestId('personal-access-key')).not.toBeInTheDocument(); + expect( + screen.queryByTestId('rotate-access-key-button'), + ).not.toBeInTheDocument(); + // The ingestion key is unaffected by the me payload. + expect(screen.getByTestId('ingestion-api-key')).toBeInTheDocument(); + }); +}); diff --git a/packages/app/tests/e2e/features/team.spec.ts b/packages/app/tests/e2e/features/team.spec.ts index e7dc837e52..98145a1dfb 100644 --- a/packages/app/tests/e2e/features/team.spec.ts +++ b/packages/app/tests/e2e/features/team.spec.ts @@ -167,11 +167,17 @@ test.describe('Team Settings Page', { tag: ['@team', '@full-stack'] }, () => { ).toBeVisible(); }); - await test.step('Verify rotate button is visible', async () => { + await test.step('Verify rotate buttons are visible', async () => { await expect(teamPage.rotateButton).toBeVisible(); + await expect(teamPage.rotateAccessKeyTrigger).toBeVisible(); }); }); + // Both rotate flows are only opened and cancelled here, never confirmed. + // Every spec shares one account via a single storageState, and + // dashboard-external-api-*.spec.ts read the personal access key then fire + // bearer requests — a confirmed rotation in a parallel worker would 401 them. + // The confirm path is covered by packages/api me.int.test.ts. test('should open and cancel rotate API key modal', async () => { await test.step('Open rotate API key modal', async () => { await teamPage.openApiAndAgentsTab(); @@ -179,12 +185,40 @@ test.describe('Team Settings Page', { tag: ['@team', '@full-stack'] }, () => { }); await test.step('Verify modal shows irreversible warning', async () => { - await expect(teamPage.page.getByText('not reversible')).toBeVisible(); + // Scoped to this modal: both rotate dialogs carry "not reversible". + await expect( + teamPage.rotateApiKeyDialog.getByText('not reversible'), + ).toBeVisible(); }); await test.step('Cancel and verify modal closes', async () => { await teamPage.cancelRotateApiKey(); - await expect(teamPage.page.getByText('not reversible')).toBeHidden(); + await expect( + teamPage.rotateApiKeyDialog.getByText('not reversible'), + ).toBeHidden(); + }); + }); + + test('should open and cancel rotate personal access key modal', async () => { + await test.step('Open rotate personal access key modal', async () => { + await teamPage.openApiAndAgentsTab(); + await teamPage.clickRotateAccessKey(); + }); + + await test.step('Verify modal warns about irreversibility and agent configs', async () => { + await expect( + teamPage.rotateAccessKeyDialog.getByText('not reversible'), + ).toBeVisible(); + await expect( + teamPage.rotateAccessKeyDialog.getByText(/MCP \/ AI agent configs/), + ).toBeVisible(); + }); + + await test.step('Cancel and verify modal closes', async () => { + await teamPage.cancelRotateAccessKey(); + await expect( + teamPage.rotateAccessKeyDialog.getByText('not reversible'), + ).toBeHidden(); }); }); diff --git a/packages/app/tests/e2e/page-objects/TeamPage.ts b/packages/app/tests/e2e/page-objects/TeamPage.ts index 07acbf0ef3..a7e34b78b9 100644 --- a/packages/app/tests/e2e/page-objects/TeamPage.ts +++ b/packages/app/tests/e2e/page-objects/TeamPage.ts @@ -39,6 +39,11 @@ export class TeamPage { private readonly rotateApiKeyButton: Locator; private readonly rotateApiKeyConfirm: Locator; private readonly rotateApiKeyCancel: Locator; + private readonly rotateApiKeyModal: Locator; + private readonly rotateAccessKeyButton: Locator; + private readonly rotateAccessKeyConfirm: Locator; + private readonly rotateAccessKeyCancel: Locator; + private readonly rotateAccessKeyModal: Locator; // Connections elements private readonly addConnectionButton: Locator; @@ -103,6 +108,11 @@ export class TeamPage { this.rotateApiKeyButton = page.getByTestId('rotate-api-key-button'); this.rotateApiKeyConfirm = page.getByTestId('rotate-api-key-confirm'); this.rotateApiKeyCancel = page.getByTestId('rotate-api-key-cancel'); + this.rotateApiKeyModal = page.getByTestId('rotate-api-key-modal'); + this.rotateAccessKeyButton = page.getByTestId('rotate-access-key-button'); + this.rotateAccessKeyConfirm = page.getByTestId('rotate-access-key-confirm'); + this.rotateAccessKeyCancel = page.getByTestId('rotate-access-key-cancel'); + this.rotateAccessKeyModal = page.getByTestId('rotate-access-key-modal'); this.addConnectionButton = page.getByTestId('add-connection-button'); @@ -199,6 +209,22 @@ export class TeamPage { await this.rotateApiKeyCancel.click(); } + async clickRotateAccessKey() { + await this.rotateAccessKeyButton.click(); + } + + // Confirming rotates the shared E2E account's personal access key, which + // 401s the bearer requests in dashboard-external-api-*.spec.ts running in + // parallel workers. Covered by me.int.test.ts instead — see the note in + // team.spec.ts. + async confirmRotateAccessKey() { + await this.rotateAccessKeyConfirm.click(); + } + + async cancelRotateAccessKey() { + await this.rotateAccessKeyCancel.click(); + } + // --- Connections --- async clickAddConnection() { @@ -396,6 +422,18 @@ export class TeamPage { return this.rotateApiKeyButton; } + get rotateApiKeyDialog() { + return this.rotateApiKeyModal; + } + + get rotateAccessKeyTrigger() { + return this.rotateAccessKeyButton; + } + + get rotateAccessKeyDialog() { + return this.rotateAccessKeyModal; + } + get members() { return this.teamMembersSection; } diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index bb211a7d7f..f93d5b5f00 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -2485,6 +2485,20 @@ export const MeApiResponseSchema = z.object({ export type MeApiResponse = z.infer; +// Response for `PATCH /me/accessKey`. +// +// Deliberately not RotateApiKeyApiResponseSchema (`{ newApiKey }`): `team.apiKey` +// is the shared ingestion key, while `user.accessKey` is the per-user bearer +// token for the external API v2 and the MCP server. The two are rendered side by +// side in Team Settings, so the wire names must not blur together. +export const RotateAccessKeyApiResponseSchema = z.object({ + newAccessKey: z.string(), +}); + +export type RotateAccessKeyApiResponse = z.infer< + typeof RotateAccessKeyApiResponseSchema +>; + // IaC (Terraform) export // // Shared so `GET /iac/import-manifest` and the generators in From 1bf03d33a0da71ca6fd7dfdca4496fda07a07564 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 17 Aug 2026 12:20:34 -0400 Subject: [PATCH 2/4] fix(app): keep the rotated access key reachable and block double rotation Two review findings on the new personal access key flow, plus the sentence-case convention the card was not following. The success handler discarded the newAccessKey from the response and relied on refetchMe() to pick it up. If that refetch failed, every useMe consumer kept rendering the old key, which the rotation had already revoked, so the user could copy a dead credential with no way to reach the working one short of a reload. The mutation now seeds the me cache from the response instead, which needs no network round trip and removes the failure window. Confirming closed the modal but left the controls enabled, and Mantine keeps modal content mounted through the exit transition, so a fast double click sent two PATCHes and the second revoked the key the first had just generated. The shared confirm modal now takes confirmDisabled, wired to isPending for both the ingestion and personal flows. Applies sentence case to every label in the card per agent_docs/code_style.md, including the pre-existing ingestion key strings, so the two halves match. Acronyms keep their casing, so "Rotate API key" and "Rotate personal API access key". The E2E label assertions and the MCP.md references move with them. --- MCP.md | 4 +-- packages/app/src/api.ts | 17 ++++++++++- .../TeamSettings/ApiKeysSection.tsx | 28 +++++++++++++------ .../__tests__/ApiKeysSection.test.tsx | 25 +++++++++++++---- packages/app/tests/e2e/features/team.spec.ts | 6 ++-- 5 files changed, 60 insertions(+), 20 deletions(-) diff --git a/MCP.md b/MCP.md index ffedf22873..132a7a9d79 100644 --- a/MCP.md +++ b/MCP.md @@ -9,8 +9,8 @@ data sources directly. - A running HyperDX instance (see [CONTRIBUTING.md](/CONTRIBUTING.md) for local development setup, or [DEPLOY.md](/DEPLOY.md) for self-hosted deployment) -- A **Personal API Access Key** — find yours in the HyperDX UI under **Team - Settings > API Keys > Personal API Access Key** +- A **Personal API access key** — find yours in the HyperDX UI under **Team + Settings > API keys > Personal API access key** > **Note:** HyperDX v1 ([hyperdx.io](https://hyperdx.io)) does not yet support > the MCP server. The documentation below applies to self-hosted HyperDX v2 diff --git a/packages/app/src/api.ts b/packages/app/src/api.ts index e6b90a2db8..0260eaf281 100644 --- a/packages/app/src/api.ts +++ b/packages/app/src/api.ts @@ -24,7 +24,12 @@ import type { WebhookTestApiResponse, WebhookUpdateApiResponse, } from '@hyperdx/common-utils/dist/types'; -import { useInfiniteQuery, useMutation, useQuery } from '@tanstack/react-query'; +import { + useInfiniteQuery, + useMutation, + useQuery, + useQueryClient, +} from '@tanstack/react-query'; import { IS_LOCAL_MODE } from './config'; import { getLocalDashboardTags } from './dashboard'; @@ -271,11 +276,21 @@ const api = { }); }, useRotatePersonalAccessKey() { + const queryClient = useQueryClient(); return useMutation({ mutationFn: async () => hdxServer(`me/accessKey`, { method: 'PATCH', }).json(), + // Seed the cache from the response rather than refetching `me`. The old + // key is already revoked by the time this runs, so a refetch that fails + // would leave every `useMe` consumer rendering a dead credential with no + // way to reach the new one short of a reload. + onSuccess: data => { + queryClient.setQueryData(['me'], prev => + prev == null ? prev : { ...prev, accessKey: data.newAccessKey }, + ); + }, }); }, useDeleteTeamMember() { diff --git a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx index 3951e1fa61..8f68b44942 100644 --- a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx +++ b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx @@ -42,12 +42,20 @@ function RotateKeyConfirmModal({ onConfirm, title, testIdPrefix, + confirmDisabled = false, children, }: { opened: boolean; onClose: () => void; onConfirm: () => void; title: string; + /** + * Blocks a second rotation while the first is still in flight. The modal + * closes on confirm, but Mantine keeps its content mounted for the exit + * transition, so a fast double click would otherwise fire two PATCHes and + * revoke the key the first one just generated. + */ + confirmDisabled?: boolean; /** * Yields `${prefix}-modal`, `-cancel` and `-confirm`. The ingestion flow * passes `rotate-api-key` to preserve the testids that @@ -86,6 +94,7 @@ function RotateKeyConfirmModal({ variant="danger" className="mt-2 px-4 float-end" size="sm" + disabled={confirmDisabled} onClick={onConfirm} > Confirm @@ -98,7 +107,7 @@ function RotateKeyConfirmModal({ export default function ApiKeysSection() { const { data: team, refetch: refetchTeam } = api.useTeam(); - const { data: me, isLoading: isLoadingMe, refetch: refetchMe } = api.useMe(); + const { data: me, isLoading: isLoadingMe } = api.useMe(); const rotateTeamApiKey = api.useRotateTeamApiKey(); const rotatePersonalAccessKey = api.useRotatePersonalAccessKey(); const hasAdminAccess = true; @@ -144,7 +153,6 @@ export default function ApiKeysSection() { message: 'Revoked your old personal access key and generated a new one.', }); - refetchMe(); }, onError: e => { notifications.show({ @@ -158,10 +166,10 @@ export default function ApiKeysSection() { return ( - API Keys + API keys - Ingestion API Key + Ingestion API key {team?.apiKey && ( setRotateApiKeyConfirmationModalShow(true)} > - Rotate API Key + Rotate API key )} @@ -183,8 +191,9 @@ export default function ApiKeysSection() { opened={rotateApiKeyConfirmationModalShow} onClose={() => setRotateApiKeyConfirmationModalShow(false)} onConfirm={onConfirmUpdateTeamApiKey} - title="Rotate API Key" + title="Rotate API key" testIdPrefix="rotate-api-key" + confirmDisabled={rotateTeamApiKey.isPending} > Rotating the API key will invalidate your existing API key and @@ -195,7 +204,7 @@ export default function ApiKeysSection() { {!isLoadingMe && me != null && ( - Personal API Access Key + Personal API access key setRotateAccessKeyConfirmationModalShow(true)} > - Rotate Access Key + Rotate access key setRotateAccessKeyConfirmationModalShow(false)} onConfirm={onConfirmRotateAccessKey} - title="Rotate Personal API Access Key" + title="Rotate personal API access key" testIdPrefix="rotate-access-key" + confirmDisabled={rotatePersonalAccessKey.isPending} > Rotating your personal access key immediately revokes the diff --git a/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx index 8fa0097cb4..15aa8a78c1 100644 --- a/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx +++ b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx @@ -35,7 +35,6 @@ type MutateOptions = { }; let capturedPersonalOptions: MutateOptions | undefined; -const refetchMe = jest.fn(); const rotatePersonalMutate = jest.fn( (_vars: undefined, options?: MutateOptions) => { capturedPersonalOptions = options; @@ -57,7 +56,6 @@ function setMe(accessKey: string | null, isLoading = false) { createdAt: '', }, isLoading, - refetch: refetchMe, }); } @@ -145,7 +143,26 @@ describe('ApiKeysSection', () => { ); }); - it('refetches me and notifies on a successful rotation', async () => { + // The modal closes on confirm, but Mantine keeps its content mounted through + // the exit transition, so without this guard a fast double click fires two + // PATCHes and the second revokes the key the first just generated. + it('disables confirm while a rotation is already in flight', async () => { + mockUseRotatePersonalAccessKey.mockReturnValue({ + mutate: rotatePersonalMutate, + isPending: true, + }); + const user = userEvent.setup(); + renderWithMantine(); + + await openPersonalRotateModal(user); + const confirm = screen.getByTestId('rotate-access-key-confirm'); + expect(confirm).toBeDisabled(); + + await user.click(confirm); + expect(rotatePersonalMutate).not.toHaveBeenCalled(); + }); + + it('notifies on a successful rotation', async () => { const user = userEvent.setup(); renderWithMantine(); @@ -153,7 +170,6 @@ describe('ApiKeysSection', () => { await user.click(screen.getByTestId('rotate-access-key-confirm')); act(() => capturedPersonalOptions?.onSuccess?.()); - expect(refetchMe).toHaveBeenCalledTimes(1); expect( await screen.findByText(/Revoked your old personal access key/), ).toBeInTheDocument(); @@ -167,7 +183,6 @@ describe('ApiKeysSection', () => { await user.click(screen.getByTestId('rotate-access-key-confirm')); act(() => capturedPersonalOptions?.onError?.(new Error('rotate blew up'))); - expect(refetchMe).not.toHaveBeenCalled(); expect(await screen.findByText('rotate blew up')).toBeInTheDocument(); }); diff --git a/packages/app/tests/e2e/features/team.spec.ts b/packages/app/tests/e2e/features/team.spec.ts index 98145a1dfb..f6c8c084f9 100644 --- a/packages/app/tests/e2e/features/team.spec.ts +++ b/packages/app/tests/e2e/features/team.spec.ts @@ -72,7 +72,7 @@ test.describe('Team Settings Page', { tag: ['@team', '@full-stack'] }, () => { await test.step('Verify API & Agents tab headings exist', async () => { await expect( - teamPage.apiKeys.getByText('API Keys', { exact: true }), + teamPage.apiKeys.getByText('API keys', { exact: true }), ).toBeVisible(); await expect( teamPage.mcpServer.getByText('Connect your AI Agents', { exact: true }), @@ -160,10 +160,10 @@ test.describe('Team Settings Page', { tag: ['@team', '@full-stack'] }, () => { await test.step('Verify API key labels are visible', async () => { await expect( - teamPage.apiKeys.getByText('Ingestion API Key'), + teamPage.apiKeys.getByText('Ingestion API key'), ).toBeVisible(); await expect( - teamPage.apiKeys.getByText('Personal API Access Key'), + teamPage.apiKeys.getByText('Personal API access key'), ).toBeVisible(); }); From 7c0f804cb334511007c0839c9ce25892e4dc9c4c Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Tue, 18 Aug 2026 10:45:09 -0400 Subject: [PATCH 3/4] refactor(app): rotate keys through the shared useConfirm dialog Review feedback: use the existing useConfirm rather than a bespoke modal component. Both the ingestion and personal key flows now go through it, so RotateKeyConfirmModal is gone. This also removes the confirmDisabled/isPending plumbing added for the double-rotation guard. useConfirm resolves its promise exactly once, so a double click on Confirm during the modal's exit transition cannot fire a second PATCH; the guarantee is structural rather than a prop each caller has to wire. Tradeoff: useConfirm passes no title to the Modal and renders its body at size="sm" opacity={0.7}, so the ingestion dialog loses its heading and both warnings are muted. That matches the other four consumers, which are all destructive delete confirmations. Adding an optional title to useConfirm would be a separate change across all of them. The per-flow confirm and cancel testids collapse into the shared confirm-* ones, so the page object drops its duplicated locators. --- .../TeamSettings/ApiKeysSection.tsx | 159 ++++-------------- .../__tests__/ApiKeysSection.test.tsx | 127 +++++++------- packages/app/tests/e2e/features/team.spec.ts | 27 ++- .../app/tests/e2e/page-objects/TeamPage.ts | 49 ++---- 4 files changed, 128 insertions(+), 234 deletions(-) diff --git a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx index 8f68b44942..edbc77f7aa 100644 --- a/packages/app/src/components/TeamSettings/ApiKeysSection.tsx +++ b/packages/app/src/components/TeamSettings/ApiKeysSection.tsx @@ -1,10 +1,11 @@ -import { type ReactNode, useState } from 'react'; +import { useState } from 'react'; import { CopyToClipboard } from 'react-copy-to-clipboard'; -import { Box, Button, Card, Divider, Group, Modal, Text } from '@mantine/core'; +import { Box, Button, Card, Divider, Group, Text } from '@mantine/core'; import { notifications } from '@mantine/notifications'; import { IconCheck, IconClipboard } from '@tabler/icons-react'; import api from '@/api'; +import { useConfirm } from '@/useConfirm'; function APIKeyCopyButton({ value, @@ -36,91 +37,29 @@ function APIKeyCopyButton({ ); } -function RotateKeyConfirmModal({ - opened, - onClose, - onConfirm, - title, - testIdPrefix, - confirmDisabled = false, - children, -}: { - opened: boolean; - onClose: () => void; - onConfirm: () => void; - title: string; - /** - * Blocks a second rotation while the first is still in flight. The modal - * closes on confirm, but Mantine keeps its content mounted for the exit - * transition, so a fast double click would otherwise fire two PATCHes and - * revoke the key the first one just generated. - */ - confirmDisabled?: boolean; - /** - * Yields `${prefix}-modal`, `-cancel` and `-confirm`. The ingestion flow - * passes `rotate-api-key` to preserve the testids that - * tests/e2e/page-objects/TeamPage.ts already depends on. - */ - testIdPrefix: string; - children: ReactNode; -}) { - return ( - - {title} - - } - > - - {children} - - - - - - - ); -} - export default function ApiKeysSection() { const { data: team, refetch: refetchTeam } = api.useTeam(); const { data: me, isLoading: isLoadingMe } = api.useMe(); const rotateTeamApiKey = api.useRotateTeamApiKey(); const rotatePersonalAccessKey = api.useRotatePersonalAccessKey(); + const confirm = useConfirm(); const hasAdminAccess = true; - const [ - rotateApiKeyConfirmationModalShow, - setRotateApiKeyConfirmationModalShow, - ] = useState(false); - const [ - rotateAccessKeyConfirmationModalShow, - setRotateAccessKeyConfirmationModalShow, - ] = useState(false); - const rotateTeamApiKeyAction = () => { + // `confirm` resolves exactly once, so a double click on its Confirm button + // during the modal's exit transition cannot fire a second rotation. + const onRotateTeamApiKey = async () => { + const confirmed = await confirm( + <> + Rotating the API key will invalidate your existing API key and generate + a new one for you. This action is not reversible. + , + 'Rotate key', + { variant: 'danger' }, + ); + if (!confirmed) { + return; + } + rotateTeamApiKey.mutate(undefined, { onSuccess: () => { notifications.show({ @@ -139,13 +78,23 @@ export default function ApiKeysSection() { }); }; - const onConfirmUpdateTeamApiKey = () => { - rotateTeamApiKeyAction(); - setRotateApiKeyConfirmationModalShow(false); - }; + const onRotateAccessKey = async () => { + const confirmed = await confirm( + <> + Rotating your personal access key immediately revokes the current one + and generates a new one. This action is not reversible. Anything + still using the old key will start failing with 401 until you update it, + including MCP / AI agent configs (Claude Code, Cursor, VS Code, Codex), + external API v2 clients, Terraform / IaC providers, and CI scripts. Your + browser session is not affected; you will stay signed in. + , + 'Rotate key', + { variant: 'danger' }, + ); + if (!confirmed) { + return; + } - const onConfirmRotateAccessKey = () => { - setRotateAccessKeyConfirmationModalShow(false); rotatePersonalAccessKey.mutate(undefined, { onSuccess: () => { notifications.show({ @@ -181,25 +130,12 @@ export default function ApiKeysSection() { )} - setRotateApiKeyConfirmationModalShow(false)} - onConfirm={onConfirmUpdateTeamApiKey} - title="Rotate API key" - testIdPrefix="rotate-api-key" - confirmDisabled={rotateTeamApiKey.isPending} - > - - Rotating the API key will invalidate your existing API key and - generate a new one for you. This action is not reversible. - - {!isLoadingMe && me != null && ( @@ -213,32 +149,11 @@ export default function ApiKeysSection() { - setRotateAccessKeyConfirmationModalShow(false)} - onConfirm={onConfirmRotateAccessKey} - title="Rotate personal API access key" - testIdPrefix="rotate-access-key" - confirmDisabled={rotatePersonalAccessKey.isPending} - > - - Rotating your personal access key immediately revokes the - current one and generates a new one. This action is{' '} - not reversible. - - - Anything still using the old key will start failing with 401 - until you update it — MCP / AI agent configs (Claude Code, - Cursor, VS Code, Codex), external API v2 clients, Terraform / - IaC providers, and CI scripts. Your browser session is not - affected; you will stay signed in. - - )} diff --git a/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx index 15aa8a78c1..ff9c6226bd 100644 --- a/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx +++ b/packages/app/src/components/TeamSettings/__tests__/ApiKeysSection.test.tsx @@ -1,8 +1,10 @@ -import { act, screen, waitFor } from '@testing-library/react'; +import type { ReactNode } from 'react'; +import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import api from '@/api'; import ApiKeysSection from '@/components/TeamSettings/ApiKeysSection'; +import { useConfirm } from '@/useConfirm'; jest.mock('@/api', () => ({ __esModule: true, @@ -15,10 +17,14 @@ jest.mock('@/api', () => ({ hdxServer: jest.fn(), })); +// The real ConfirmProvider lives in pages/_app.tsx and pulls in next/router, +// so the shared dialog is mocked here and exercised in the E2E specs instead. +jest.mock('@/useConfirm', () => ({ useConfirm: jest.fn() })); + // Annotated as the loose `jest.Mock` rather than `jest.mocked(...)`'s exact // MockedFunction: these hooks return TanStack `UseQueryResult` / // `UseMutationResult` objects with ~25 members, and the tests only need the -// three the component reads. The loose type keeps `mockReturnValue` at `any` +// few the component reads. The loose type keeps `mockReturnValue` at `any` // so the partial fixtures below need no type assertions. const mockUseMe: jest.Mock = jest.mocked(api.useMe); const mockUseTeam: jest.Mock = jest.mocked(api.useTeam); @@ -26,6 +32,7 @@ const mockUseRotateTeamApiKey: jest.Mock = jest.mocked(api.useRotateTeamApiKey); const mockUseRotatePersonalAccessKey: jest.Mock = jest.mocked( api.useRotatePersonalAccessKey, ); +const mockUseConfirm: jest.Mock = jest.mocked(useConfirm); /** Options object the component hands to `mutate`, captured so the tests can * drive `onSuccess` / `onError` without a real mutation. */ @@ -40,8 +47,18 @@ const rotatePersonalMutate = jest.fn( capturedPersonalOptions = options; }, ); - -const PERSONAL_MODAL_COPY = /Rotating your personal access key/; +const rotateTeamMutate = jest.fn(); + +/** Resolution of the next `confirm(...)` call, i.e. did the user accept. */ +let confirmAccepts = true; +// Params are declared so `mock.calls[n][i]` stays typed; the body ignores them. +const confirmSpy = jest.fn( + ( + _message: ReactNode, + _confirmLabel?: string, + _options?: { variant?: 'primary' | 'danger' }, + ) => Promise.resolve(confirmAccepts), +); function setMe(accessKey: string | null, isLoading = false) { mockUseMe.mockReturnValue({ @@ -59,28 +76,25 @@ function setMe(accessKey: string | null, isLoading = false) { }); } -/** - * Mantine mounts modal content one tick after `opened` flips, so every - * open-the-modal step has to await the content rather than the modal root — - * the root stays in the DOM (empty) the whole time. - */ -async function openPersonalRotateModal( - user: ReturnType, -) { - await user.click(screen.getByTestId('rotate-access-key-button')); - await screen.findByText(PERSONAL_MODAL_COPY); +/** Renders the ReactNode the component passed to `confirm` so the dialog copy + * stays asserted even though the dialog itself is mocked out. */ +function renderConfirmMessage(callIndex = 0) { + const message = confirmSpy.mock.calls[callIndex][0]; + return render(<>{message}); } beforeEach(() => { jest.clearAllMocks(); capturedPersonalOptions = undefined; + confirmAccepts = true; setMe('personal_key_abc'); + mockUseConfirm.mockReturnValue(confirmSpy); mockUseTeam.mockReturnValue({ data: { apiKey: 'ingestion_key_xyz' }, refetch: jest.fn(), }); - mockUseRotateTeamApiKey.mockReturnValue({ mutate: jest.fn() }); + mockUseRotateTeamApiKey.mockReturnValue({ mutate: rotateTeamMutate }); mockUseRotatePersonalAccessKey.mockReturnValue({ mutate: rotatePersonalMutate, }); @@ -101,74 +115,49 @@ describe('ApiKeysSection', () => { ); }); - it('opens the personal rotate modal with the breakage warning', async () => { + it('asks for a danger confirmation naming what the old key breaks', async () => { const user = userEvent.setup(); renderWithMantine(); - await openPersonalRotateModal(user); + await user.click(screen.getByTestId('rotate-access-key-button')); - const modal = screen.getByTestId('rotate-access-key-modal'); - expect(modal).toHaveTextContent(/not reversible/); - expect(modal).toHaveTextContent(/MCP/); - expect(modal).toHaveTextContent(/stay signed in/); - // The ingestion modal must not have opened alongside it. - expect( - screen.queryByText(/Rotating the API key will invalidate/), - ).not.toBeInTheDocument(); - }); + expect(confirmSpy).toHaveBeenCalledTimes(1); + expect(confirmSpy.mock.calls[0][1]).toBe('Rotate key'); + expect(confirmSpy.mock.calls[0][2]).toEqual({ variant: 'danger' }); - it('closes the personal rotate modal on cancel without mutating', async () => { - const user = userEvent.setup(); - renderWithMantine(); - - await openPersonalRotateModal(user); - await user.click(screen.getByTestId('rotate-access-key-cancel')); - - expect(rotatePersonalMutate).not.toHaveBeenCalled(); - await waitFor(() => - expect(screen.queryByText(PERSONAL_MODAL_COPY)).not.toBeInTheDocument(), - ); + const { container } = renderConfirmMessage(); + expect(container).toHaveTextContent(/not reversible/); + expect(container).toHaveTextContent(/MCP \/ AI agent configs/); + expect(container).toHaveTextContent(/stay signed in/); }); - it('rotates once on confirm and closes the modal', async () => { + it('does not rotate when the confirmation is declined', async () => { + confirmAccepts = false; const user = userEvent.setup(); renderWithMantine(); - await openPersonalRotateModal(user); - await user.click(screen.getByTestId('rotate-access-key-confirm')); + await user.click(screen.getByTestId('rotate-access-key-button')); - expect(rotatePersonalMutate).toHaveBeenCalledTimes(1); - await waitFor(() => - expect(screen.queryByText(PERSONAL_MODAL_COPY)).not.toBeInTheDocument(), - ); + await waitFor(() => expect(confirmSpy).toHaveBeenCalled()); + expect(rotatePersonalMutate).not.toHaveBeenCalled(); }); - // The modal closes on confirm, but Mantine keeps its content mounted through - // the exit transition, so without this guard a fast double click fires two - // PATCHes and the second revokes the key the first just generated. - it('disables confirm while a rotation is already in flight', async () => { - mockUseRotatePersonalAccessKey.mockReturnValue({ - mutate: rotatePersonalMutate, - isPending: true, - }); + it('rotates once when the confirmation is accepted', async () => { const user = userEvent.setup(); renderWithMantine(); - await openPersonalRotateModal(user); - const confirm = screen.getByTestId('rotate-access-key-confirm'); - expect(confirm).toBeDisabled(); + await user.click(screen.getByTestId('rotate-access-key-button')); - await user.click(confirm); - expect(rotatePersonalMutate).not.toHaveBeenCalled(); + await waitFor(() => expect(rotatePersonalMutate).toHaveBeenCalledTimes(1)); }); it('notifies on a successful rotation', async () => { const user = userEvent.setup(); renderWithMantine(); - await openPersonalRotateModal(user); - await user.click(screen.getByTestId('rotate-access-key-confirm')); - act(() => capturedPersonalOptions?.onSuccess?.()); + await user.click(screen.getByTestId('rotate-access-key-button')); + await waitFor(() => expect(rotatePersonalMutate).toHaveBeenCalled()); + capturedPersonalOptions?.onSuccess?.(); expect( await screen.findByText(/Revoked your old personal access key/), @@ -179,13 +168,25 @@ describe('ApiKeysSection', () => { const user = userEvent.setup(); renderWithMantine(); - await openPersonalRotateModal(user); - await user.click(screen.getByTestId('rotate-access-key-confirm')); - act(() => capturedPersonalOptions?.onError?.(new Error('rotate blew up'))); + await user.click(screen.getByTestId('rotate-access-key-button')); + await waitFor(() => expect(rotatePersonalMutate).toHaveBeenCalled()); + capturedPersonalOptions?.onError?.(new Error('rotate blew up')); expect(await screen.findByText('rotate blew up')).toBeInTheDocument(); }); + it('confirms separately before rotating the ingestion key', async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.click(screen.getByTestId('rotate-api-key-button')); + + await waitFor(() => expect(rotateTeamMutate).toHaveBeenCalledTimes(1)); + expect(rotatePersonalMutate).not.toHaveBeenCalled(); + const { container } = renderConfirmMessage(); + expect(container).toHaveTextContent(/invalidate your existing API key/); + }); + it('renders neither the personal key nor its rotate button when me is null', () => { setMe(null); diff --git a/packages/app/tests/e2e/features/team.spec.ts b/packages/app/tests/e2e/features/team.spec.ts index f6c8c084f9..1afece9e29 100644 --- a/packages/app/tests/e2e/features/team.spec.ts +++ b/packages/app/tests/e2e/features/team.spec.ts @@ -184,40 +184,39 @@ test.describe('Team Settings Page', { tag: ['@team', '@full-stack'] }, () => { await teamPage.clickRotateApiKey(); }); - await test.step('Verify modal shows irreversible warning', async () => { - // Scoped to this modal: both rotate dialogs carry "not reversible". + await test.step('Verify dialog shows irreversible warning', async () => { await expect( - teamPage.rotateApiKeyDialog.getByText('not reversible'), + teamPage.confirmDialogBox.getByText(/invalidate your existing API key/), ).toBeVisible(); }); - await test.step('Cancel and verify modal closes', async () => { - await teamPage.cancelRotateApiKey(); + await test.step('Cancel and verify dialog closes', async () => { + await teamPage.cancelConfirmDialog(); await expect( - teamPage.rotateApiKeyDialog.getByText('not reversible'), + teamPage.confirmDialogBox.getByText(/invalidate your existing API key/), ).toBeHidden(); }); }); - test('should open and cancel rotate personal access key modal', async () => { - await test.step('Open rotate personal access key modal', async () => { + test('should open and cancel rotate personal access key dialog', async () => { + await test.step('Open rotate personal access key dialog', async () => { await teamPage.openApiAndAgentsTab(); await teamPage.clickRotateAccessKey(); }); - await test.step('Verify modal warns about irreversibility and agent configs', async () => { + await test.step('Verify dialog warns about irreversibility and agent configs', async () => { await expect( - teamPage.rotateAccessKeyDialog.getByText('not reversible'), + teamPage.confirmDialogBox.getByText(/not reversible/), ).toBeVisible(); await expect( - teamPage.rotateAccessKeyDialog.getByText(/MCP \/ AI agent configs/), + teamPage.confirmDialogBox.getByText(/MCP \/ AI agent configs/), ).toBeVisible(); }); - await test.step('Cancel and verify modal closes', async () => { - await teamPage.cancelRotateAccessKey(); + await test.step('Cancel and verify dialog closes', async () => { + await teamPage.cancelConfirmDialog(); await expect( - teamPage.rotateAccessKeyDialog.getByText('not reversible'), + teamPage.confirmDialogBox.getByText(/MCP \/ AI agent configs/), ).toBeHidden(); }); }); diff --git a/packages/app/tests/e2e/page-objects/TeamPage.ts b/packages/app/tests/e2e/page-objects/TeamPage.ts index a7e34b78b9..a859da1e9f 100644 --- a/packages/app/tests/e2e/page-objects/TeamPage.ts +++ b/packages/app/tests/e2e/page-objects/TeamPage.ts @@ -36,14 +36,10 @@ export class TeamPage { private readonly teamNameCancelButton: Locator; // API Keys elements + // Both rotate flows go through the shared `useConfirm` dialog, so only the + // triggers are per-key. Its locators are confirmDialog* below. private readonly rotateApiKeyButton: Locator; - private readonly rotateApiKeyConfirm: Locator; - private readonly rotateApiKeyCancel: Locator; - private readonly rotateApiKeyModal: Locator; private readonly rotateAccessKeyButton: Locator; - private readonly rotateAccessKeyConfirm: Locator; - private readonly rotateAccessKeyCancel: Locator; - private readonly rotateAccessKeyModal: Locator; // Connections elements private readonly addConnectionButton: Locator; @@ -59,6 +55,8 @@ export class TeamPage { private readonly confirmDeleteMemberButton: Locator; private readonly cancelDeleteMemberButton: Locator; private readonly confirmDialogConfirmBtn: Locator; + private readonly confirmDialogCancelBtn: Locator; + private readonly confirmDialogModal: Locator; constructor(page: Page) { this.page = page; @@ -106,13 +104,7 @@ export class TeamPage { this.teamNameCancelButton = page.getByTestId('team-name-cancel-button'); this.rotateApiKeyButton = page.getByTestId('rotate-api-key-button'); - this.rotateApiKeyConfirm = page.getByTestId('rotate-api-key-confirm'); - this.rotateApiKeyCancel = page.getByTestId('rotate-api-key-cancel'); - this.rotateApiKeyModal = page.getByTestId('rotate-api-key-modal'); this.rotateAccessKeyButton = page.getByTestId('rotate-access-key-button'); - this.rotateAccessKeyConfirm = page.getByTestId('rotate-access-key-confirm'); - this.rotateAccessKeyCancel = page.getByTestId('rotate-access-key-cancel'); - this.rotateAccessKeyModal = page.getByTestId('rotate-access-key-modal'); this.addConnectionButton = page.getByTestId('add-connection-button'); @@ -125,6 +117,8 @@ export class TeamPage { this.confirmDeleteMemberButton = page.getByTestId('confirm-delete-member'); this.cancelDeleteMemberButton = page.getByTestId('cancel-delete-member'); this.confirmDialogConfirmBtn = page.getByTestId('confirm-confirm-button'); + this.confirmDialogCancelBtn = page.getByTestId('confirm-cancel-button'); + this.confirmDialogModal = page.getByTestId('confirm-modal'); } async goto() { @@ -201,28 +195,16 @@ export class TeamPage { await this.rotateApiKeyButton.click(); } - async confirmRotateApiKey() { - await this.rotateApiKeyConfirm.click(); - } - - async cancelRotateApiKey() { - await this.rotateApiKeyCancel.click(); - } - async clickRotateAccessKey() { await this.rotateAccessKeyButton.click(); } - // Confirming rotates the shared E2E account's personal access key, which + // Confirming rotates the shared E2E account's keys. For the personal key that // 401s the bearer requests in dashboard-external-api-*.spec.ts running in - // parallel workers. Covered by me.int.test.ts instead — see the note in - // team.spec.ts. - async confirmRotateAccessKey() { - await this.rotateAccessKeyConfirm.click(); - } - - async cancelRotateAccessKey() { - await this.rotateAccessKeyCancel.click(); + // parallel workers, so the rotate specs only cancel. See the note in + // team.spec.ts; the confirm path is covered by me.int.test.ts. + async cancelConfirmDialog() { + await this.confirmDialogCancelBtn.click(); } // --- Connections --- @@ -422,16 +404,13 @@ export class TeamPage { return this.rotateApiKeyButton; } - get rotateApiKeyDialog() { - return this.rotateApiKeyModal; - } - get rotateAccessKeyTrigger() { return this.rotateAccessKeyButton; } - get rotateAccessKeyDialog() { - return this.rotateAccessKeyModal; + /** Shared `useConfirm` dialog, used by both rotate flows. */ + get confirmDialogBox() { + return this.confirmDialogModal; } get members() { From 4ce2be813c4483858a05db6226423472bf981580 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Tue, 18 Aug 2026 10:54:32 -0400 Subject: [PATCH 4/4] docs: require useConfirm for confirmation dialogs Adds a required-pattern section for useConfirm to code_style.md, next to the other mandated components. Beyond the usage example it records the parts that are not obvious from the source: the promise resolves exactly once so double click protection is free, the confirm/cancel test ids are shared and must not be duplicated per flow, and component tests have to mock it because ConfirmProvider pulls in next/router. It also documents the missing title and the muted body, with the instruction to extend useConfirm rather than fork a one-off modal. The AGENTS.md pointer said to read code_style.md "only when actively coding", which invites deferring it during planning and then never returning. This PR shipped title-case labels and a hand-rolled modal for exactly that reason, with both rules already written down. It now says to read it before writing or planning any packages/app UI change, and calls out that these patterns are invisible from the surrounding file, so matching the component you are editing is not sufficient. --- AGENTS.md | 12 +++++++--- agent_docs/code_style.md | 50 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 24c1c24400..74c995b493 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,8 +61,12 @@ directory: - `agent_docs/architecture.md` - Detailed architecture patterns and data models - `agent_docs/tech_stack.md` - Technology stack details and component patterns - `agent_docs/development.md` - Development workflows, testing, and common tasks -- `agent_docs/code_style.md` - Code patterns and best practices (read only when - actively coding) +- `agent_docs/code_style.md` - Code patterns and best practices. **Read this + before writing or planning any `packages/app` UI change**, not just while + typing code. It carries required patterns that are invisible from the + surrounding file (sentence-case UI text, mandated Button/ActionIcon variants, + `useConfirm` for confirmation dialogs, `EmptyState`), so copying the + conventions of the component you are editing is not sufficient. - `agent_docs/observability.md` - Instrumentation standards (tracing, metrics, context) and the shared helpers (read when adding or changing a feature) @@ -87,7 +91,9 @@ before stopping. similar files before implementing 4. **Component size**: Keep files under 300 lines; break down large components 5. **UI Components**: Use custom Button/ActionIcon variants (`primary`, - `secondary`, `danger`) - see `agent_docs/code_style.md` for required patterns + `secondary`, `danger`), `useConfirm` for "are you sure?" dialogs rather than + a hand-rolled `Modal`, and sentence case for all user-facing text - see + `agent_docs/code_style.md` for required patterns 6. **Testing**: Tests live in `__tests__/` directories; use Jest for unit/integration tests 7. **Observability**: This is an observability product - instrument new code as diff --git a/agent_docs/code_style.md b/agent_docs/code_style.md index d40dbb2c0b..9cbcca85d0 100644 --- a/agent_docs/code_style.md +++ b/agent_docs/code_style.md @@ -131,6 +131,56 @@ The variant → token mapping is centralized in `packages/app/src/theme/themes/s **Note**: Existing `` call sites are untouched; the semantic variants are opt-in. Prefer the variant for any **new** callout, and migrate nearby `color="..."` alerts when you touch them. +### Confirmation dialogs: use `useConfirm` (REQUIRED) + +**Use `useConfirm` (`@/useConfirm`) for any "are you sure?" step. Do not +hand-roll a `` with Cancel/Confirm buttons.** The provider is already +mounted app-wide in `pages/_app.tsx`, so there is no setup at the call site. + +```tsx +const confirm = useConfirm(); + +const handleDelete = async () => { + if ( + await confirm( + <> + Deleting {name} is not reversible. + , + 'Delete', + { variant: 'danger' }, + ) + ) { + await deleteThing.mutateAsync({ id }); + } +}; +``` + +- The message is a `ReactNode`, so it can carry emphasis and multiple sentences. +- Pass `{ variant: 'danger' }` for destructive actions; the confirm label + defaults to `Confirm`. +- It resolves **exactly once**, so a double click on Confirm during the modal's + exit transition cannot fire the action twice. A hand-rolled modal has to guard + that itself. +- Test ids are shared and already exist: `confirm-modal`, + `confirm-confirm-button`, `confirm-cancel-button`. **Do not invent per-flow + confirm/cancel test ids** — E2E page objects key off the shared ones. + +**Known limits.** It passes no `title` to the Modal and renders the body at +`size="sm" opacity={0.7}`, and CSS opacity applies to the whole subtree so a +nested `` cannot opt back out. If a flow genuinely needs a heading or +full-contrast body, **extend `useConfirm`** (an optional prop, applied to all +call sites) rather than forking a one-off modal. + +**In component tests**, mock it — `ConfirmProvider` pulls in `next/router`, +which is not available in jsdom: + +```tsx +jest.mock('@/useConfirm', () => ({ useConfirm: jest.fn() })); +``` + +Assert on the arguments (and render the message `ReactNode` if you need to check +the copy). Exercise the real dialog in E2E instead. + ### EmptyState Component (REQUIRED) **Use `EmptyState` (`@/components/EmptyState`) for all empty/no-data states.** Do not create ad-hoc inline empty states.