From ac840bad971df75d3172da718c0a22b28f7fc57a Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 16:27:47 -0700 Subject: [PATCH 01/13] feat(investigations): Apply update_detector action requests Part of a series that adds follow-up actions to Seer investigations: when a run completes, Seer suggests one next step, and the user can apply a typed action, such as a monitor change, with one click. This adds an Apply button to a suggested monitor change. When the card shows, it asks the backend with a dry run whether the change would apply, and says why when it can't, so the user only clicks a change that will save. Depends on https://github.com/getsentry/sentry/pull/128681 and https://github.com/getsentry/sentry/pull/128723 --- .../__stories__/investigationFixtureApi.tsx | 46 ++++ .../actionRequests/actionRequestCard.tsx | 42 +++- .../actionRequests/actionRequestDetails.tsx | 105 ++++++++ .../actionRequests/applyButton.tsx | 71 ++++++ .../actionRequests/nextStepCard.spec.tsx | 229 +++++++++++++++++- .../actionRequests/nextStepCard.stories.tsx | 60 ++++- .../actionRequests/nextStepCard.tsx | 3 + static/app/views/investigations/api.ts | 121 ++++++++- .../investigations/detail/index.spec.tsx | 7 + .../app/views/investigations/detail/index.tsx | 1 + .../views/investigations/fixtures/index.ts | 2 + static/app/views/investigations/types.ts | 18 +- 12 files changed, 694 insertions(+), 11 deletions(-) create mode 100644 static/app/views/investigations/actionRequests/applyButton.tsx diff --git a/static/app/views/investigations/__stories__/investigationFixtureApi.tsx b/static/app/views/investigations/__stories__/investigationFixtureApi.tsx index e2d7d82ea1e1..8e4d85d980be 100644 --- a/static/app/views/investigations/__stories__/investigationFixtureApi.tsx +++ b/static/app/views/investigations/__stories__/investigationFixtureApi.tsx @@ -23,7 +23,9 @@ import { InvestigationQueryOutputFixture, InvestigationTranscriptBlockFixture, } from 'sentry/views/investigations/fixtures'; +import {getSourceMonitor} from 'sentry/views/investigations/source'; import type { + InvestigationActionRequest, InvestigationComment, InvestigationDetail, InvestigationExecutionDetail, @@ -455,6 +457,50 @@ function handleFixtureRequest( }; } + if ( + parts[1] === 'action-requests' && + parts[3] === 'apply' && + parts.length === 4 && + method === 'POST' + ) { + const detail = getFixtureDetail(state, investigationId); + const actionRequest = detail.actionRequests?.find( + candidate => candidate.id === parts[2] + ); + if (!actionRequest?.action) { + throw new Error(`Unknown fixture action request: ${parts[2]}`); + } + if (data.dryRun === true) { + return {body: {actionRequest: cloneFixture(actionRequest)}}; + } + const {action} = actionRequest; + const applied: InvestigationActionRequest = { + ...actionRequest, + state: 'applied', + appliedBy: state.viewer, + appliedDetails: + action.kind === 'update_detector' + ? { + detectorId: action.detectorId, + detectorName: + getSourceMonitor(detail.source, action.detectorId)?.name ?? + action.detectorId, + fields: Object.keys(action.changes), + } + : null, + canApply: false, + cannotApplyReason: 'not_suggested', + dateApplied: new Date().toISOString(), + }; + setFixtureDetail(state, { + ...detail, + actionRequests: detail.actionRequests?.map(candidate => + candidate.id === applied.id ? applied : candidate + ), + }); + return {body: {actionRequest: cloneFixture(applied)}}; + } + if (parts[1] === 'title-generation' && method === 'GET') { const detail = getFixtureDetail(state, investigationId); return { diff --git a/static/app/views/investigations/actionRequests/actionRequestCard.tsx b/static/app/views/investigations/actionRequests/actionRequestCard.tsx index 2b0bdbad23bf..8897df116ab8 100644 --- a/static/app/views/investigations/actionRequests/actionRequestCard.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestCard.tsx @@ -2,9 +2,20 @@ import {useId} from 'react'; import {Stack} from '@sentry/scraps/layout'; import {Heading, Text} from '@sentry/scraps/text'; +import {toast} from '@sentry/scraps/toast'; -import {tct} from 'sentry/locale'; -import {ActionRequestDetails} from 'sentry/views/investigations/actionRequests/actionRequestDetails'; +import {t, tct} from 'sentry/locale'; +import {useOrganization} from 'sentry/utils/useOrganization'; +import { + ActionRequestApply, + ActionRequestDetails, + getApplySuccessMessage, + useApplyCheck, +} from 'sentry/views/investigations/actionRequests/actionRequestDetails'; +import { + parseApplyError, + useApplyInvestigationActionRequestMutation, +} from 'sentry/views/investigations/api'; import {ensureSentenceEnd} from 'sentry/views/investigations/sentence'; import type { InvestigationActionRequest, @@ -13,6 +24,7 @@ import type { type ActionRequestCardProps = { actionRequest: InvestigationActionRequest; + investigationId: string; heading?: string; source?: InvestigationDetail['source']; }; @@ -20,9 +32,27 @@ type ActionRequestCardProps = { export function ActionRequestCard({ actionRequest, heading, + investigationId, source, }: ActionRequestCardProps) { const headingId = useId(); + const organization = useOrganization(); + const check = useApplyCheck(investigationId, actionRequest); + const applyMutation = useApplyInvestigationActionRequestMutation( + organization.slug, + investigationId, + { + onSuccess: (_response, applied) => { + const message = getApplySuccessMessage(applied.action); + if (message) { + toast.success(message); + } + }, + onError: error => { + toast.error(parseApplyError(error)?.detail ?? t('Unable to apply this change.')); + }, + } + ); if (actionRequest.state !== 'suggested' && actionRequest.state !== 'withdrawn') { return null; @@ -63,6 +93,14 @@ export function ActionRequestCard({ })} + applyMutation.mutate(actionRequest)} + /> ); } diff --git a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx index f71b64c0a9ca..1cd2fad03e5d 100644 --- a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx @@ -1,8 +1,22 @@ +import {IconSiren} from '@sentry/icons/siren'; +import {useQuery, useQueryClient} from '@tanstack/react-query'; + import {Text} from '@sentry/scraps/text'; +import {t} from 'sentry/locale'; +import {useOrganization} from 'sentry/utils/useOrganization'; +import {ApplyButton} from 'sentry/views/investigations/actionRequests/applyButton'; import {MonitorChanges} from 'sentry/views/investigations/actionRequests/monitorChanges'; +import { + actionRequestDryRunQueryOptions, + getInvestigationDetailQueryOptions, + parseApplyError, + type ApplyError, +} from 'sentry/views/investigations/api'; import {getSourceMonitor} from 'sentry/views/investigations/source'; import type { + InvestigationAction, + InvestigationActionCannotApplyReason, InvestigationActionRequest, InvestigationDetail, } from 'sentry/views/investigations/types'; @@ -32,3 +46,94 @@ export function ActionRequestDetails({actionRequest, source}: ActionRequestDetai ) : null; } } + +type ActionRequestApplyProps = { + actionRequest: InvestigationActionRequest; + applying: boolean; + check: ApplyError | null; + onApply: () => void; +}; + +export function ActionRequestApply({ + actionRequest, + applying, + check, + onApply, +}: ActionRequestApplyProps) { + switch (actionRequest.action?.kind) { + case 'update_detector': + return ( + } + label={t('Update monitor')} + busyLabel={t('Updating monitor…')} + cannotApplyMessage={getMonitorCannotApplyMessage( + actionRequest.cannotApplyReason + )} + /> + ); + default: + return null; + } +} + +export function useApplyCheck( + investigationId: string, + actionRequest: InvestigationActionRequest +): ApplyError | null { + const organization = useOrganization(); + const queryClient = useQueryClient(); + const investigationVersion = queryClient.getQueryData( + getInvestigationDetailQueryOptions(organization.slug, investigationId).queryKey + )?.json.version; + const {error} = useQuery({ + ...actionRequestDryRunQueryOptions( + organization.slug, + investigationId, + actionRequest.id, + investigationVersion + ), + enabled: + hasApplyCheck(actionRequest) && + actionRequest.state === 'suggested' && + actionRequest.canApply, + }); + return error ? parseApplyError(error) : null; +} + +function hasApplyCheck(actionRequest: InvestigationActionRequest) { + switch (actionRequest.action?.kind) { + case 'update_detector': + return true; + default: + return false; + } +} + +export function getApplySuccessMessage(action: InvestigationAction | null) { + switch (action?.kind) { + case 'update_detector': + return t('Monitor updated.'); + default: + return null; + } +} + +function getMonitorCannotApplyMessage( + reason: InvestigationActionCannotApplyReason | null +) { + switch (reason) { + case 'permission_denied': + return t("You don't have permission to edit this monitor."); + case 'detector_not_found': + return t('This monitor is no longer available.'); + case 'not_target_detector': + return t('This change targets a different monitor than this investigation.'); + default: + return null; + } +} diff --git a/static/app/views/investigations/actionRequests/applyButton.tsx b/static/app/views/investigations/actionRequests/applyButton.tsx new file mode 100644 index 000000000000..c2586d033e79 --- /dev/null +++ b/static/app/views/investigations/actionRequests/applyButton.tsx @@ -0,0 +1,71 @@ +import {Button} from '@sentry/scraps/button'; +import {Flex} from '@sentry/scraps/layout'; +import {Text} from '@sentry/scraps/text'; + +import {t} from 'sentry/locale'; +import type {ApplyError} from 'sentry/views/investigations/api'; +import type { + InvestigationActionCannotApplyReason, + InvestigationActionRequest, +} from 'sentry/views/investigations/types'; + +type ApplyButtonProps = { + actionRequest: InvestigationActionRequest; + applying: boolean; + busyLabel: string; + cannotApplyMessage: string | null; + check: ApplyError | null; + icon: React.ReactNode; + label: string; + onApply: () => void; +}; + +export function ApplyButton({ + actionRequest, + applying, + busyLabel, + cannotApplyMessage, + check, + icon, + label, + onApply, +}: ApplyButtonProps) { + const {canApply, cannotApplyReason, state} = actionRequest; + const message = + !canApply && state === 'suggested' + ? (cannotApplyMessage ?? getCannotApplyMessage(cannotApplyReason)) + : (check?.detail ?? null); + return ( + + + {message ? ( + + {message} + + ) : null} + + ); +} + +function getCannotApplyMessage(reason: InvestigationActionCannotApplyReason | null) { + switch (reason) { + case 'investigation_archived': + return t('Archived investigations are read-only.'); + case 'not_suggested': + return t('This can no longer be applied.'); + case 'unsupported_action': + return t("Seer can't apply this kind of change yet."); + case 'no_access': + return t("You don't have access to apply this."); + default: + return t("This change can't be applied."); + } +} diff --git a/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx b/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx index 878c8cb78f56..6e93078a9bfb 100644 --- a/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx +++ b/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx @@ -1,4 +1,10 @@ -import {render, screen, within} from 'sentry-test/reactTestingLibrary'; +import { + render, + screen, + userEvent, + waitFor, + within, +} from 'sentry-test/reactTestingLibrary'; import {NextStepCard} from 'sentry/views/investigations/actionRequests/nextStepCard'; import { @@ -58,14 +64,43 @@ function runWith( }); } +const applyUrl = + '/organizations/org-slug/investigations/investigation-1/action-requests/312/apply/'; + +type MockResponse = Parameters[0]; + +function mockApply(response: MockResponse) { + return MockApiClient.addMockResponse({ + url: applyUrl, + method: 'POST', + match: [(_url, options) => !options.data?.dryRun], + ...response, + }); +} + +function mockDryRun(response: MockResponse = {}) { + return MockApiClient.addMockResponse({ + url: applyUrl, + method: 'POST', + match: [MockApiClient.matchData({dryRun: true})], + body: {actionRequest: InvestigationActionRequestFixture()}, + ...response, + }); +} + function getCard() { return screen.getByRole('region', {name: 'Recommended action'}); } describe('NextStepCard', () => { + beforeEach(() => { + mockDryRun(); + }); + it('renders the primary request', () => { render( { it('describes cleared filters, all environments and thresholds', () => { render( { it('renders advice without an action as text only', () => { render( { it('does not add a period after closing punctuation', () => { render( { it('renders nothing without a primary request', () => { render( @@ -171,6 +210,7 @@ describe('NextStepCard', () => { it('only shows a primary from the newest report', () => { render( { it('renders nothing for a state it does not know', () => { render( @@ -210,6 +251,7 @@ describe('NextStepCard', () => { it('renders text from Seer as plain text', () => { render( { it('collapses a request Seer withdrew to its reason', () => { render( { it('collapses a withdrawn request without a reason to its title', () => { render( @@ -268,6 +312,7 @@ describe('NextStepCard', () => { it('holds space while the next step generates', () => { render( { it('holds space before the next step starts', () => { render( { it('holds space for a completed next step until its request arrives', () => { render( { it('renders nothing for a completed next step that suggested nothing', () => { render( @@ -321,6 +369,7 @@ describe('NextStepCard', () => { it('renders nothing while the run is loading', () => { render( { it('shows the newest primary over one Seer withdrew', () => { render( { it('holds space for a new next step over a withdrawn one', () => { render( { it('explains a next step that failed', () => { render( @@ -390,6 +442,7 @@ describe('NextStepCard', () => { it('hides an old primary while a new report is written', () => { render( { it('names the monitor and what it measures', () => { render( { it('formats static thresholds with the aggregate unit', () => { render( { it('formats each threshold with its own aggregate unit', () => { render( { it('does not describe the opening condition when the aggregate changes', () => { render( { it('formats percent thresholds like the monitor page', () => { render( { it('describes the condition that opens an issue for a percent monitor', () => { render( { it('describes the opening condition from a snapshot without a detection type', () => { render( { it('reads a percent monitor from a snapshot without a detection type', () => { render( { it('names the monitor without an opening condition for a dynamic monitor', () => { render( { it('skips a condition type it does not know', () => { render( { it('leaves out monitor details when the snapshot does not parse', () => { render( { it('leaves out a different monitor than the one the suggestion changes', () => { render( { it('shows query and environment changes in place of what they replace', () => { render( { expect(card).toHaveTextContent('Measuresp95(span.duration)'); expect(card).not.toHaveTextContent('p95(span.duration) • transaction:/api/cart'); }); + + it('applies the change', async () => { + const apply = mockApply({ + body: {actionRequest: InvestigationActionRequestFixture({state: 'applied'})}, + }); + render( + + ); + + await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + + expect(apply).toHaveBeenCalledWith( + applyUrl, + expect.objectContaining({method: 'POST', data: {}}) + ); + expect(await screen.findByText('Monitor updated.')).toBeInTheDocument(); + }); + + it('shows the updating state while the change is applied', async () => { + mockApply({body: {}, asyncDelay: 1_000_000}); + render( + + ); + + await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + + expect(await screen.findByRole('button', {name: 'Updating monitor…'})).toBeDisabled(); + }); + + it.each([ + {status: 500, body: {}, message: 'Unable to apply this change.'}, + { + status: 403, + body: { + code: 'permission_denied', + detail: 'You do not have permission to edit this monitor.', + }, + message: 'You do not have permission to edit this monitor.', + }, + ])('says when an apply fails with $status', async ({status, body, message}) => { + mockApply({statusCode: status, body}); + render( + + ); + + await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + + expect(await screen.findByText(message)).toBeInTheDocument(); + }); + + it.each([ + ['permission_denied', "You don't have permission to edit this monitor."], + ['detector_not_found', 'This monitor is no longer available.'], + [ + 'not_target_detector', + 'This change targets a different monitor than this investigation.', + ], + ['investigation_archived', 'Archived investigations are read-only.'], + ])('says why a %s change cannot apply', (cannotApplyReason, message) => { + render( + + ); + + expect( + within(getCard()).getByRole('button', {name: 'Update monitor'}) + ).toBeDisabled(); + expect(getCard()).toHaveTextContent(message); + }); + + it('checks the change before it is applied', async () => { + const dryRun = mockDryRun(); + render( + + ); + + await waitFor(() => expect(dryRun).toHaveBeenCalled()); + expect(screen.getByRole('button', {name: 'Update monitor'})).toBeEnabled(); + }); + + it('holds the update when the check finds a problem', async () => { + mockDryRun({ + statusCode: 400, + body: { + code: 'unsupported_action', + detail: "Seer can't change dynamic monitors yet.", + }, + }); + render( + + ); + + expect( + await screen.findByText("Seer can't change dynamic monitors yet.") + ).toBeInTheDocument(); + expect(screen.getByRole('button', {name: 'Update monitor'})).toBeDisabled(); + }); + + it('does not check a change the viewer cannot apply', () => { + const dryRun = mockDryRun(); + render( + + ); + + expect(dryRun).not.toHaveBeenCalled(); + }); + + it('has no apply button for advice', () => { + render( + + ); + + expect( + screen.queryByRole('button', {name: 'Update monitor'}) + ).not.toBeInTheDocument(); + expect(getCard()).not.toHaveTextContent("Seer can't apply this kind of change yet."); + }); }); diff --git a/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx b/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx index f44e136966fd..3d675985f9cf 100644 --- a/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx +++ b/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx @@ -1,12 +1,17 @@ import {Fragment} from 'react'; +import {useQuery} from '@tanstack/react-query'; import {Container} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; import * as Storybook from 'sentry/stories'; +import {useOrganization} from 'sentry/utils/useOrganization'; +import {InvestigationFixtureApi} from 'sentry/views/investigations/__stories__/investigationFixtureApi'; import {NextStepCard} from 'sentry/views/investigations/actionRequests/nextStepCard'; +import {getInvestigationDetailQueryOptions} from 'sentry/views/investigations/api'; import { InvestigationActionRequestFixture, + InvestigationDetailFixture, InvestigationOrchestrationFixture, } from 'sentry/views/investigations/fixtures'; import type { @@ -38,17 +43,48 @@ function runWith( }); } +function investigationWith(id: string, actionRequests: InvestigationActionRequest[]) { + return InvestigationDetailFixture({id, actionRequests}); +} + function CardStory({ actionRequests, + id, orchestration = completedRun, }: { actionRequests: InvestigationActionRequest[]; + id: string; orchestration?: InvestigationOrchestration; }) { return ( - - - + + + + + + ); +} + +function FixtureCard({ + investigationId, + orchestration, +}: { + investigationId: string; + orchestration: InvestigationOrchestration; +}) { + const organization = useOrganization(); + const {data: investigation} = useQuery( + getInvestigationDetailQueryOptions(organization.slug, investigationId) + ); + return ( + ); } @@ -58,12 +94,22 @@ export default Storybook.story('Investigations — Recommended Action', story => Seer's primary request from the latest report sits under the summary. - + )); + story('Cannot apply', () => ( + + )); + story('Threshold change', () => ( story('Query change', () => ( Advice Seer cannot carry out has no action, so the card shows the advice only. story('Withdrawn', () => ( story('Generating', () => ( )); story('Failed', () => ( - + )); }); diff --git a/static/app/views/investigations/actionRequests/nextStepCard.tsx b/static/app/views/investigations/actionRequests/nextStepCard.tsx index 5405b27e5b3b..82982a2f6576 100644 --- a/static/app/views/investigations/actionRequests/nextStepCard.tsx +++ b/static/app/views/investigations/actionRequests/nextStepCard.tsx @@ -14,6 +14,7 @@ import type { type NextStepCardProps = { actionRequests: InvestigationActionRequest[] | undefined; + investigationId: string; orchestration: InvestigationOrchestration | undefined; /** The action requests predate the run's current next step. */ actionRequestsStale?: boolean; @@ -24,6 +25,7 @@ type NextStepCardProps = { export function NextStepCard({ actionRequests, actionRequestsStale = false, + investigationId, orchestration, orchestrationLoading = false, source, @@ -44,6 +46,7 @@ export function NextStepCard({ ); diff --git a/static/app/views/investigations/api.ts b/static/app/views/investigations/api.ts index dcbee2f6cb1f..3acb54a89d1e 100644 --- a/static/app/views/investigations/api.ts +++ b/static/app/views/investigations/api.ts @@ -1,14 +1,19 @@ import { + queryOptions, useMutation, useQueryClient, type UseMutationOptions, } from '@tanstack/react-query'; +import {z} from 'zod'; -import type {ApiResponse} from 'sentry/utils/api/apiFetch'; -import {apiOptions} from 'sentry/utils/api/apiOptions'; +import {apiFetch, type ApiResponse} from 'sentry/utils/api/apiFetch'; +import {apiOptions, selectJson} from 'sentry/utils/api/apiOptions'; import {getApiUrl} from 'sentry/utils/api/getApiUrl'; import {fetchMutation} from 'sentry/utils/queryClient'; +import {RequestError} from 'sentry/utils/requestError/requestError'; import type { + InvestigationActionRequest, + InvestigationActionRequestApplyResponse, InvestigationCandidate, InvestigationBlock, InvestigationBlockExecutionStart, @@ -669,6 +674,118 @@ export function useRunInvestigationBlockMutation( }); } +const applyErrorSchema = z.object({detail: z.string()}); + +export type ApplyError = z.infer; + +export function parseApplyError(error: Error): ApplyError | null { + const parsed = applyErrorSchema.safeParse( + error instanceof RequestError ? error.responseJSON : undefined + ); + return parsed.success ? parsed.data : null; +} + +function getActionRequestApplyUrl( + organizationSlug: string, + investigationId: string, + actionRequestId: string +) { + return getApiUrl( + '/organizations/$organizationIdOrSlug/investigations/$investigationId/action-requests/$actionRequestId/apply/', + { + path: { + organizationIdOrSlug: organizationSlug, + investigationId, + actionRequestId, + }, + } + ); +} + +export function actionRequestDryRunQueryOptions( + organizationSlug: string, + investigationId: string, + actionRequestId: string, + investigationVersion: number | undefined +) { + const url = getActionRequestApplyUrl( + organizationSlug, + investigationId, + actionRequestId + ); + const endpoint = {method: 'POST', data: {dryRun: true}} as const; + return queryOptions({ + queryKey: [url, endpoint, {investigationVersion}] as const, + queryFn: () => + apiFetch({queryKey: [url, endpoint]}), + select: selectJson, + staleTime: 0, + retry: false, + }); +} + +export function useApplyInvestigationActionRequestMutation( + organizationSlug: string, + investigationId: string, + options?: MutationOptions< + InvestigationActionRequestApplyResponse, + InvestigationActionRequest + > +) { + const queryClient = useQueryClient(); + const detailOptions = getInvestigationDetailQueryOptions( + organizationSlug, + investigationId + ); + const orchestrationOptions = investigationOrchestrationQueryOptions( + organizationSlug, + investigationId + ); + + function refresh() { + void queryClient.invalidateQueries({queryKey: detailOptions.queryKey}); + void queryClient.invalidateQueries({queryKey: orchestrationOptions.queryKey}); + } + + return useMutation({ + ...options, + mutationFn: actionRequest => + fetchMutation({ + url: getActionRequestApplyUrl( + organizationSlug, + investigationId, + actionRequest.id + ), + method: 'POST', + data: {}, + }), + onSuccess: async (response, variables, onMutateResult, context) => { + queryClient.setQueryData(detailOptions.queryKey, current => + current + ? { + ...current, + json: { + ...current.json, + version: current.json.version + 1, + actionRequests: current.json.actionRequests?.map(actionRequest => + actionRequest.id === response.actionRequest.id + ? response.actionRequest + : actionRequest + ), + }, + } + : current + ); + refresh(); + await options?.onSuccess?.(response, variables, onMutateResult, context); + }, + onError: async (error, variables, onMutateResult, context) => { + refresh(); + await options?.onError?.(error, variables, onMutateResult, context); + }, + }); +} + export function useUpdateInvestigationBlockPromptMutation( organizationSlug: string, investigationId: string diff --git a/static/app/views/investigations/detail/index.spec.tsx b/static/app/views/investigations/detail/index.spec.tsx index 67825cdc99ba..895e0c6c7d33 100644 --- a/static/app/views/investigations/detail/index.spec.tsx +++ b/static/app/views/investigations/detail/index.spec.tsx @@ -134,6 +134,13 @@ describe('Investigation detail', () => { body: {viewers: [], total: 0, heartbeatIntervalMs: 5000}, }); MockApiClient.addMockResponse({url: `${detailUrl}comments/`, body: []}); + for (const id of ['312', '320']) { + MockApiClient.addMockResponse({ + url: `${detailUrl}action-requests/${id}/apply/`, + method: 'POST', + body: {}, + }); + } }); it('loads and renders the complete investigation response', async () => { diff --git a/static/app/views/investigations/detail/index.tsx b/static/app/views/investigations/detail/index.tsx index 285edb8535e6..3fa29c9334ad 100644 --- a/static/app/views/investigations/detail/index.tsx +++ b/static/app/views/investigations/detail/index.tsx @@ -574,6 +574,7 @@ function InvestigationPageContent({investigation}: {investigation: Investigation ; }; +export type InvestigationActionCannotApplyReason = InvestigationOrchestrationOpenString< + | 'investigation_archived' + | 'not_suggested' + | 'unsupported_action' + | 'no_access' + | 'permission_denied' + | 'detector_not_found' + | 'not_target_detector' +>; + export type InvestigationActionRequest = { action: InvestigationAction | null; anchor: InvestigationActionRequestAnchor | null; appliedBy: AvatarUser | null; appliedDetails: Record | null; + canApply: boolean; + cannotApplyReason: InvestigationActionCannotApplyReason | null; dateApplied: string | null; dateCreated: string; error: Record | null; @@ -310,6 +322,10 @@ export type InvestigationActionRequest = { title: string; }; +export type InvestigationActionRequestApplyResponse = { + actionRequest: InvestigationActionRequest; +}; + type InvestigationOrchestrationError = { code: string; message: string; From b7942c6fabd75f395b14ac1cf927118511f718c1 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 18:00:14 -0700 Subject: [PATCH 02/13] fix(investigations): Wait for the apply check and show why it fails The button stays disabled while the dry run runs and keeps its last answer through a refetch. Only a rejection with a code blocks it, the reason uses the code's copy, and it's linked for screen readers. The check reruns when Seer changes the action. --- .../__stories__/investigationFixtureApi.tsx | 43 +++- .../actionRequests/actionRequestCard.tsx | 11 +- .../actionRequests/actionRequestDetails.tsx | 88 ++++++--- .../actionRequests/applyButton.tsx | 62 +++--- .../actionRequests/nextStepCard.spec.tsx | 184 +++++++++++++++--- .../actionRequests/nextStepCard.stories.tsx | 22 ++- static/app/views/investigations/api.ts | 39 ++-- static/app/views/investigations/types.ts | 8 + 8 files changed, 346 insertions(+), 111 deletions(-) diff --git a/static/app/views/investigations/__stories__/investigationFixtureApi.tsx b/static/app/views/investigations/__stories__/investigationFixtureApi.tsx index 8e4d85d980be..f3c58d57a66b 100644 --- a/static/app/views/investigations/__stories__/investigationFixtureApi.tsx +++ b/static/app/views/investigations/__stories__/investigationFixtureApi.tsx @@ -46,6 +46,8 @@ type InvestigationFixtureApiProps = { /** Page-level and block comments, keyed by investigation id. */ comments?: Record; details?: InvestigationDetail[]; + /** Dry-run error responses, keyed by action request id. */ + dryRunErrors?: Record; executions?: Record; featureEnabled?: boolean; list?: InvestigationListItem[]; @@ -67,6 +69,11 @@ type FixtureResponse = { headers?: Record; }; +type FixtureErrorResponse = { + body: Record; + status: number; +}; + type FixtureHandlerResult = Promise | typeof NO_MATCH; type FixtureHandler = ( path: string, @@ -89,6 +96,7 @@ export function InvestigationFixtureApi({ commentPageLinks = {}, comments = {}, details = [], + dryRunErrors = {}, executions = {}, featureEnabled = true, list = [], @@ -119,6 +127,7 @@ export function InvestigationFixtureApi({ commentPageLinks, comments, details, + dryRunErrors, executions, featureEnabled, list, @@ -241,16 +250,11 @@ function createFixtureHandler(config: FixtureApiConfig): FixtureHandler { return new Promise(() => {}); } if (config.mode === 'error') { - const method = normalizeMethod(options.method); - const response: ResponseMeta = { - status: 500, - statusText: 'Internal Server Error', - responseJSON: {detail: 'Fixture API error'}, - responseText: 'Fixture API error', - getResponseHeader: () => null, - }; return Promise.reject( - new RequestError(method, path, new Error('Fixture API error'), response) + fixtureRequestError(normalizeMethod(options.method), path, { + status: 500, + body: {detail: 'Fixture API error'}, + }) ); } @@ -264,6 +268,7 @@ type FixtureState = { commentPageLinks: Record; comments: Map; details: Map; + dryRunErrors: Record; executions: Map; list: InvestigationListItem[]; orchestration: Map; @@ -302,6 +307,7 @@ function createFixtureState(config: FixtureApiConfig): FixtureState { ]) ), details, + dryRunErrors: {...config.dryRunErrors}, list, orchestration: new Map( Object.entries(config.orchestration ?? {}).map(([key, run]) => [ @@ -471,6 +477,10 @@ function handleFixtureRequest( throw new Error(`Unknown fixture action request: ${parts[2]}`); } if (data.dryRun === true) { + const dryRunError = state.dryRunErrors[actionRequest.id]; + if (dryRunError) { + throw fixtureRequestError(method, path, dryRunError); + } return {body: {actionRequest: cloneFixture(actionRequest)}}; } const {action} = actionRequest; @@ -848,6 +858,21 @@ function setFixtureDetail(state: FixtureState, detail: InvestigationDetail) { state.list[listIndex] = {...state.list[listIndex], ...detail}; } +function fixtureRequestError( + method: ReturnType, + path: string, + {body, status}: FixtureErrorResponse +) { + const response: ResponseMeta = { + status, + statusText: 'Fixture API error', + responseJSON: body, + responseText: JSON.stringify(body), + getResponseHeader: () => null, + }; + return new RequestError(method, path, new Error('Fixture API error'), response); +} + function normalizeMethod(method: RequestOptions['method']) { return method ?? 'GET'; } diff --git a/static/app/views/investigations/actionRequests/actionRequestCard.tsx b/static/app/views/investigations/actionRequests/actionRequestCard.tsx index 8897df116ab8..dcd231711a6e 100644 --- a/static/app/views/investigations/actionRequests/actionRequestCard.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestCard.tsx @@ -10,6 +10,7 @@ import { ActionRequestApply, ActionRequestDetails, getApplySuccessMessage, + getCannotApplyMessage, useApplyCheck, } from 'sentry/views/investigations/actionRequests/actionRequestDetails'; import { @@ -48,8 +49,14 @@ export function ActionRequestCard({ toast.success(message); } }, - onError: error => { - toast.error(parseApplyError(error)?.detail ?? t('Unable to apply this change.')); + onError: (error, applied) => { + const applyError = parseApplyError(error); + toast.error( + applyError + ? (getCannotApplyMessage(applied.action, applyError.code) ?? + applyError.detail) + : t('Unable to apply this change.') + ); }, } ); diff --git a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx index 1cd2fad03e5d..4dd34ceeab7d 100644 --- a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx @@ -1,22 +1,20 @@ import {IconSiren} from '@sentry/icons/siren'; -import {useQuery, useQueryClient} from '@tanstack/react-query'; +import {useQuery} from '@tanstack/react-query'; import {Text} from '@sentry/scraps/text'; import {t} from 'sentry/locale'; import {useOrganization} from 'sentry/utils/useOrganization'; -import {ApplyButton} from 'sentry/views/investigations/actionRequests/applyButton'; -import {MonitorChanges} from 'sentry/views/investigations/actionRequests/monitorChanges'; import { - actionRequestDryRunQueryOptions, - getInvestigationDetailQueryOptions, - parseApplyError, - type ApplyError, -} from 'sentry/views/investigations/api'; + ApplyButton, + type ApplyCheck, +} from 'sentry/views/investigations/actionRequests/applyButton'; +import {MonitorChanges} from 'sentry/views/investigations/actionRequests/monitorChanges'; +import {actionRequestDryRunQueryOptions} from 'sentry/views/investigations/api'; import {getSourceMonitor} from 'sentry/views/investigations/source'; import type { InvestigationAction, - InvestigationActionCannotApplyReason, + InvestigationActionApplyErrorCode, InvestigationActionRequest, InvestigationDetail, } from 'sentry/views/investigations/types'; @@ -50,7 +48,7 @@ export function ActionRequestDetails({actionRequest, source}: ActionRequestDetai type ActionRequestApplyProps = { actionRequest: InvestigationActionRequest; applying: boolean; - check: ApplyError | null; + check: ApplyCheck; onApply: () => void; }; @@ -71,9 +69,9 @@ export function ActionRequestApply({ icon={} label={t('Update monitor')} busyLabel={t('Updating monitor…')} - cannotApplyMessage={getMonitorCannotApplyMessage( - actionRequest.cannotApplyReason - )} + getCannotApplyMessage={reason => + getCannotApplyMessage(actionRequest.action, reason) + } /> ); default: @@ -84,25 +82,19 @@ export function ActionRequestApply({ export function useApplyCheck( investigationId: string, actionRequest: InvestigationActionRequest -): ApplyError | null { +): ApplyCheck { const organization = useOrganization(); - const queryClient = useQueryClient(); - const investigationVersion = queryClient.getQueryData( - getInvestigationDetailQueryOptions(organization.slug, investigationId).queryKey - )?.json.version; - const {error} = useQuery({ - ...actionRequestDryRunQueryOptions( - organization.slug, - investigationId, - actionRequest.id, - investigationVersion - ), - enabled: - hasApplyCheck(actionRequest) && - actionRequest.state === 'suggested' && - actionRequest.canApply, + const {data, isError, isPending} = useQuery({ + ...actionRequestDryRunQueryOptions(organization.slug, investigationId, actionRequest), + enabled: hasApplyCheck(actionRequest) && actionRequest.canApply, }); - return error ? parseApplyError(error) : null; + if (isError) { + return {status: 'unavailable'}; + } + if (isPending) { + return {status: 'pending'}; + } + return data ? {status: 'blocked', error: data} : {status: 'passed'}; } function hasApplyCheck(actionRequest: InvestigationActionRequest) { @@ -123,9 +115,41 @@ export function getApplySuccessMessage(action: InvestigationAction | null) { } } -function getMonitorCannotApplyMessage( - reason: InvestigationActionCannotApplyReason | null +export function getCannotApplyMessage( + action: InvestigationAction | null, + reason: InvestigationActionApplyErrorCode +) { + return getKindCannotApplyMessage(action, reason) ?? getSharedCannotApplyMessage(reason); +} + +function getKindCannotApplyMessage( + action: InvestigationAction | null, + reason: InvestigationActionApplyErrorCode ) { + switch (action?.kind) { + case 'update_detector': + return getMonitorCannotApplyMessage(reason); + default: + return null; + } +} + +function getSharedCannotApplyMessage(reason: InvestigationActionApplyErrorCode) { + switch (reason) { + case 'investigation_archived': + return t('Archived investigations are read-only.'); + case 'not_suggested': + return t('This can no longer be applied.'); + case 'unsupported_action': + return t("Seer can't apply this kind of change yet."); + case 'no_access': + return t("You don't have access to apply this."); + default: + return null; + } +} + +function getMonitorCannotApplyMessage(reason: InvestigationActionApplyErrorCode) { switch (reason) { case 'permission_denied': return t("You don't have permission to edit this monitor."); diff --git a/static/app/views/investigations/actionRequests/applyButton.tsx b/static/app/views/investigations/actionRequests/applyButton.tsx index c2586d033e79..5ac5de424517 100644 --- a/static/app/views/investigations/actionRequests/applyButton.tsx +++ b/static/app/views/investigations/actionRequests/applyButton.tsx @@ -1,3 +1,5 @@ +import {useId} from 'react'; + import {Button} from '@sentry/scraps/button'; import {Flex} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; @@ -5,16 +7,22 @@ import {Text} from '@sentry/scraps/text'; import {t} from 'sentry/locale'; import type {ApplyError} from 'sentry/views/investigations/api'; import type { - InvestigationActionCannotApplyReason, + InvestigationActionApplyErrorCode, InvestigationActionRequest, } from 'sentry/views/investigations/types'; +export type ApplyCheck = + | {status: 'pending'} + | {status: 'passed'} + | {error: ApplyError; status: 'blocked'} + | {status: 'unavailable'}; + type ApplyButtonProps = { actionRequest: InvestigationActionRequest; applying: boolean; busyLabel: string; - cannotApplyMessage: string | null; - check: ApplyError | null; + check: ApplyCheck; + getCannotApplyMessage: (reason: InvestigationActionApplyErrorCode) => string | null; icon: React.ReactNode; label: string; onApply: () => void; @@ -24,48 +32,48 @@ export function ApplyButton({ actionRequest, applying, busyLabel, - cannotApplyMessage, check, + getCannotApplyMessage, icon, label, onApply, }: ApplyButtonProps) { - const {canApply, cannotApplyReason, state} = actionRequest; - const message = - !canApply && state === 'suggested' - ? (cannotApplyMessage ?? getCannotApplyMessage(cannotApplyReason)) - : (check?.detail ?? null); + const messageId = useId(); + const {canApply, cannotApplyReason} = actionRequest; + const checking = canApply && check.status === 'pending'; + const message = canApply + ? getCheckMessage(check, getCannotApplyMessage) + : ((cannotApplyReason && getCannotApplyMessage(cannotApplyReason)) ?? + t("This change can't be applied.")); return ( - {message ? ( - - {message} - - ) : null} + + {message} + ); } -function getCannotApplyMessage(reason: InvestigationActionCannotApplyReason | null) { - switch (reason) { - case 'investigation_archived': - return t('Archived investigations are read-only.'); - case 'not_suggested': - return t('This can no longer be applied.'); - case 'unsupported_action': - return t("Seer can't apply this kind of change yet."); - case 'no_access': - return t("You don't have access to apply this."); +function getCheckMessage( + check: ApplyCheck, + getCannotApplyMessage: ApplyButtonProps['getCannotApplyMessage'] +) { + switch (check.status) { + case 'blocked': + return getCannotApplyMessage(check.error.code) ?? check.error.detail; + case 'unavailable': + return t("Couldn't check this change in advance. Applying it checks it again."); default: - return t("This change can't be applied."); + return null; } } diff --git a/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx b/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx index 6e93078a9bfb..787a82897cf1 100644 --- a/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx +++ b/static/app/views/investigations/actionRequests/nextStepCard.spec.tsx @@ -1,3 +1,6 @@ +import {QueryClientProvider} from '@tanstack/react-query'; + +import {makeTestQueryClient} from 'sentry-test/queryClient'; import { render, screen, @@ -7,8 +10,10 @@ import { } from 'sentry-test/reactTestingLibrary'; import {NextStepCard} from 'sentry/views/investigations/actionRequests/nextStepCard'; +import {getInvestigationDetailQueryOptions} from 'sentry/views/investigations/api'; import { InvestigationActionRequestFixture, + InvestigationDetailFixture, InvestigationOrchestrationFixture, } from 'sentry/views/investigations/fixtures'; import type { @@ -92,6 +97,12 @@ function getCard() { return screen.getByRole('region', {name: 'Recommended action'}); } +async function findApplyButton() { + const button = screen.getByRole('button', {name: 'Update monitor'}); + await waitFor(() => expect(button).toBeEnabled()); + return button; +} + describe('NextStepCard', () => { beforeEach(() => { mockDryRun(); @@ -741,25 +752,50 @@ describe('NextStepCard', () => { expect(card).not.toHaveTextContent('p95(span.duration) • transaction:/api/cart'); }); - it('applies the change', async () => { - const apply = mockApply({ - body: {actionRequest: InvestigationActionRequestFixture({state: 'applied'})}, + it('applies the change and stores the applied request', async () => { + const actionRequest = InvestigationActionRequestFixture(); + const applied = InvestigationActionRequestFixture({ + state: 'applied', + canApply: false, + cannotApplyReason: 'not_suggested', + }); + const apply = mockApply({body: {actionRequest: applied}}); + const queryClient = makeTestQueryClient(); + const detailKey = getInvestigationDetailQueryOptions( + 'org-slug', + 'investigation-1' + ).queryKey; + queryClient.setQueryData(detailKey, { + headers: {}, + json: InvestigationDetailFixture({ + id: 'investigation-1', + version: 4, + actionRequests: [actionRequest], + }), }); render( + actionRequests={[actionRequest]} + />, + { + additionalWrapper: ({children}) => ( + {children} + ), + } ); - await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + await userEvent.click(await findApplyButton()); expect(apply).toHaveBeenCalledWith( applyUrl, expect.objectContaining({method: 'POST', data: {}}) ); expect(await screen.findByText('Monitor updated.')).toBeInTheDocument(); + const cached = queryClient.getQueryData(detailKey)?.json; + expect(cached?.actionRequests).toEqual([applied]); + expect(cached?.version).toBe(4); }); it('shows the updating state while the change is applied', async () => { @@ -772,20 +808,30 @@ describe('NextStepCard', () => { /> ); - await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + await userEvent.click(await findApplyButton()); expect(await screen.findByRole('button', {name: 'Updating monitor…'})).toBeDisabled(); }); it.each([ - {status: 500, body: {}, message: 'Unable to apply this change.'}, + { + status: 500, + body: {detail: 'Internal Error', errorId: 'abc123'}, + message: 'Unable to apply this change.', + }, { status: 403, + body: {detail: "This action can't be applied.", code: 'permission_denied'}, + message: "You don't have permission to edit this monitor.", + }, + { + status: 409, body: { - code: 'permission_denied', - detail: 'You do not have permission to edit this monitor.', + detail: 'The monitor changed since this was suggested.', + code: 'stale', + current: {timeWindowSeconds: 600}, }, - message: 'You do not have permission to edit this monitor.', + message: 'The monitor changed since this was suggested.', }, ])('says when an apply fails with $status', async ({status, body, message}) => { mockApply({statusCode: status, body}); @@ -797,7 +843,7 @@ describe('NextStepCard', () => { /> ); - await userEvent.click(screen.getByRole('button', {name: 'Update monitor'})); + await userEvent.click(await findApplyButton()); expect(await screen.findByText(message)).toBeInTheDocument(); }); @@ -821,10 +867,24 @@ describe('NextStepCard', () => { /> ); - expect( - within(getCard()).getByRole('button', {name: 'Update monitor'}) - ).toBeDisabled(); - expect(getCard()).toHaveTextContent(message); + const button = within(getCard()).getByRole('button', {name: 'Update monitor'}); + expect(button).toBeDisabled(); + expect(button).toHaveAccessibleDescription(message); + }); + + it('holds the button while the change is checked', () => { + mockDryRun({asyncDelay: 1_000_000}); + render( + + ); + + const button = screen.getByRole('button', {name: 'Update monitor'}); + expect(button).toBeDisabled(); + expect(button).toHaveAttribute('aria-busy', 'true'); }); it('checks the change before it is applied', async () => { @@ -837,18 +897,56 @@ describe('NextStepCard', () => { /> ); - await waitFor(() => expect(dryRun).toHaveBeenCalled()); - expect(screen.getByRole('button', {name: 'Update monitor'})).toBeEnabled(); + expect(await findApplyButton()).toBeEnabled(); + expect(dryRun).toHaveBeenCalledTimes(1); }); - it('holds the update when the check finds a problem', async () => { - mockDryRun({ - statusCode: 400, + it.each([ + { + status: 409, body: { - code: 'unsupported_action', - detail: "Seer can't change dynamic monitors yet.", + detail: 'The monitor changed since this was suggested.', + code: 'stale', + current: {timeWindowSeconds: 600}, }, - }); + message: 'The monitor changed since this was suggested.', + }, + { + status: 403, + body: {detail: "This action can't be applied.", code: 'permission_denied'}, + message: "You don't have permission to edit this monitor.", + }, + { + status: 400, + body: { + detail: 'The monitor change was not valid.', + code: 'validation_failed', + errors: {timeWindowSeconds: ['Invalid Time Window: 900']}, + messages: ['Invalid Time Window: 900'], + }, + message: 'The monitor change was not valid.', + }, + ])( + 'holds the update when the check fails with $body.code', + async ({status, body, message}) => { + mockDryRun({statusCode: status, body}); + render( + + ); + + await waitFor(() => expect(screen.getByRole('status')).toHaveTextContent(message)); + const button = screen.getByRole('button', {name: 'Update monitor'}); + expect(button).toBeDisabled(); + expect(button).toHaveAccessibleDescription(message); + } + ); + + it('keeps the update available when the check itself fails', async () => { + mockDryRun({statusCode: 500, body: {detail: 'Internal Error', errorId: 'abc123'}}); render( { /> ); - expect( - await screen.findByText("Seer can't change dynamic monitors yet.") - ).toBeInTheDocument(); - expect(screen.getByRole('button', {name: 'Update monitor'})).toBeDisabled(); + await waitFor(() => + expect(screen.getByRole('status')).toHaveTextContent( + "Couldn't check this change in advance. Applying it checks it again." + ) + ); + expect(screen.getByRole('button', {name: 'Update monitor'})).toBeEnabled(); + }); + + it('checks the change again when Seer changes it', async () => { + const dryRun = mockDryRun(); + const actionRequest = InvestigationActionRequestFixture(); + const {rerender} = render( + + ); + await waitFor(() => expect(dryRun).toHaveBeenCalledTimes(1)); + + rerender( + + ); + + await waitFor(() => expect(dryRun).toHaveBeenCalledTimes(2)); }); it('does not check a change the viewer cannot apply', () => { diff --git a/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx b/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx index 3d675985f9cf..9aff1d33214b 100644 --- a/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx +++ b/static/app/views/investigations/actionRequests/nextStepCard.stories.tsx @@ -1,4 +1,4 @@ -import {Fragment} from 'react'; +import {Fragment, type ComponentProps} from 'react'; import {useQuery} from '@tanstack/react-query'; import {Container} from '@sentry/scraps/layout'; @@ -49,17 +49,20 @@ function investigationWith(id: string, actionRequests: InvestigationActionReques function CardStory({ actionRequests, + dryRunErrors, id, orchestration = completedRun, }: { actionRequests: InvestigationActionRequest[]; id: string; + dryRunErrors?: ComponentProps['dryRunErrors']; orchestration?: InvestigationOrchestration; }) { return ( @@ -107,6 +110,23 @@ export default Storybook.story('Investigations — Recommended Action', story => /> )); + story('Check failed', () => ( + + )); + story('Threshold change', () => ( ; +export type ApplyError = { + code: InvestigationActionApplyErrorCode; + detail: string; +}; -export function parseApplyError(error: Error): ApplyError | null { +export function parseApplyError(error: unknown): ApplyError | null { const parsed = applyErrorSchema.safeParse( error instanceof RequestError ? error.responseJSON : undefined ); @@ -705,20 +710,31 @@ function getActionRequestApplyUrl( export function actionRequestDryRunQueryOptions( organizationSlug: string, investigationId: string, - actionRequestId: string, - investigationVersion: number | undefined + actionRequest: InvestigationActionRequest ) { const url = getActionRequestApplyUrl( organizationSlug, investigationId, - actionRequestId + actionRequest.id ); const endpoint = {method: 'POST', data: {dryRun: true}} as const; return queryOptions({ - queryKey: [url, endpoint, {investigationVersion}] as const, - queryFn: () => - apiFetch({queryKey: [url, endpoint]}), - select: selectJson, + queryKey: [url, endpoint, {action: actionRequest.action}] as const, + queryFn: async (): Promise => { + try { + await apiFetch({ + queryKey: [url, endpoint], + }); + return null; + } catch (error) { + const applyError = parseApplyError(error); + if (applyError) { + return applyError; + } + throw error; + } + }, + placeholderData: keepPreviousData, staleTime: 0, retry: false, }); @@ -766,7 +782,6 @@ export function useApplyInvestigationActionRequestMutation( ...current, json: { ...current.json, - version: current.json.version + 1, actionRequests: current.json.actionRequests?.map(actionRequest => actionRequest.id === response.actionRequest.id ? response.actionRequest diff --git a/static/app/views/investigations/types.ts b/static/app/views/investigations/types.ts index 37c77ebd7317..a2037ee18ed3 100644 --- a/static/app/views/investigations/types.ts +++ b/static/app/views/investigations/types.ts @@ -299,6 +299,14 @@ export type InvestigationActionCannotApplyReason = InvestigationOrchestrationOpe | 'not_target_detector' >; +export type InvestigationActionApplyErrorCode = InvestigationOrchestrationOpenString< + | InvestigationActionCannotApplyReason + | 'already_applied' + | 'stale' + | 'validation_failed' + | 'withdrawn' +>; + export type InvestigationActionRequest = { action: InvestigationAction | null; anchor: InvestigationActionRequestAnchor | null; From b6c9ec183476b8ed5214e0a101b58d75c3da78a4 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Fri, 23 Oct 2026 18:08:07 -0700 Subject: [PATCH 03/13] fix(investigations): Parse apply errors with one union of the backend codes --- .../actionRequests/actionRequestCard.tsx | 3 +- .../actionRequests/actionRequestDetails.tsx | 59 +++++++++++++------ .../actionRequests/applyButton.tsx | 36 ++--------- .../actionRequests/nextStepCard.spec.tsx | 30 +++++++--- .../actionRequests/nextStepCard.stories.tsx | 2 +- static/app/views/investigations/api.ts | 37 +++++++++--- static/app/views/investigations/types.ts | 21 +------ 7 files changed, 101 insertions(+), 87 deletions(-) diff --git a/static/app/views/investigations/actionRequests/actionRequestCard.tsx b/static/app/views/investigations/actionRequests/actionRequestCard.tsx index dcd231711a6e..46c35213d351 100644 --- a/static/app/views/investigations/actionRequests/actionRequestCard.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestCard.tsx @@ -53,8 +53,7 @@ export function ActionRequestCard({ const applyError = parseApplyError(error); toast.error( applyError - ? (getCannotApplyMessage(applied.action, applyError.code) ?? - applyError.detail) + ? getCannotApplyMessage(applied.action, applyError.code) : t('Unable to apply this change.') ); }, diff --git a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx index 4dd34ceeab7d..b96ecc958097 100644 --- a/static/app/views/investigations/actionRequests/actionRequestDetails.tsx +++ b/static/app/views/investigations/actionRequests/actionRequestDetails.tsx @@ -10,11 +10,13 @@ import { type ApplyCheck, } from 'sentry/views/investigations/actionRequests/applyButton'; import {MonitorChanges} from 'sentry/views/investigations/actionRequests/monitorChanges'; -import {actionRequestDryRunQueryOptions} from 'sentry/views/investigations/api'; +import { + actionRequestDryRunQueryOptions, + type ActionRequestError, +} from 'sentry/views/investigations/api'; import {getSourceMonitor} from 'sentry/views/investigations/source'; import type { InvestigationAction, - InvestigationActionApplyErrorCode, InvestigationActionRequest, InvestigationDetail, } from 'sentry/views/investigations/types'; @@ -45,6 +47,10 @@ export function ActionRequestDetails({actionRequest, source}: ActionRequestDetai } } +type ApplyBlockReason = + | InvestigationActionRequest['cannotApplyReason'] + | ActionRequestError['code']; + type ActionRequestApplyProps = { actionRequest: InvestigationActionRequest; applying: boolean; @@ -62,16 +68,14 @@ export function ActionRequestApply({ case 'update_detector': return ( } label={t('Update monitor')} busyLabel={t('Updating monitor…')} - getCannotApplyMessage={reason => - getCannotApplyMessage(actionRequest.action, reason) - } /> ); default: @@ -86,7 +90,7 @@ export function useApplyCheck( const organization = useOrganization(); const {data, isError, isPending} = useQuery({ ...actionRequestDryRunQueryOptions(organization.slug, investigationId, actionRequest), - enabled: hasApplyCheck(actionRequest) && actionRequest.canApply, + enabled: actionRequest.canApply, }); if (isError) { return {status: 'unavailable'}; @@ -97,12 +101,17 @@ export function useApplyCheck( return data ? {status: 'blocked', error: data} : {status: 'passed'}; } -function hasApplyCheck(actionRequest: InvestigationActionRequest) { - switch (actionRequest.action?.kind) { - case 'update_detector': - return true; +function getApplyMessage(actionRequest: InvestigationActionRequest, check: ApplyCheck) { + if (!actionRequest.canApply) { + return getCannotApplyMessage(actionRequest.action, actionRequest.cannotApplyReason); + } + switch (check.status) { + case 'blocked': + return getCannotApplyMessage(actionRequest.action, check.error.code); + case 'unavailable': + return t("Couldn't check this change in advance. Applying it checks it again."); default: - return false; + return null; } } @@ -117,14 +126,18 @@ export function getApplySuccessMessage(action: InvestigationAction | null) { export function getCannotApplyMessage( action: InvestigationAction | null, - reason: InvestigationActionApplyErrorCode + reason: ApplyBlockReason ) { - return getKindCannotApplyMessage(action, reason) ?? getSharedCannotApplyMessage(reason); + return ( + getKindCannotApplyMessage(action, reason) ?? + getSharedCannotApplyMessage(reason) ?? + t("This change can't be applied.") + ); } function getKindCannotApplyMessage( action: InvestigationAction | null, - reason: InvestigationActionApplyErrorCode + reason: ApplyBlockReason ) { switch (action?.kind) { case 'update_detector': @@ -134,7 +147,7 @@ function getKindCannotApplyMessage( } } -function getSharedCannotApplyMessage(reason: InvestigationActionApplyErrorCode) { +function getSharedCannotApplyMessage(reason: ApplyBlockReason) { switch (reason) { case 'investigation_archived': return t('Archived investigations are read-only.'); @@ -144,12 +157,20 @@ function getSharedCannotApplyMessage(reason: InvestigationActionApplyErrorCode) return t("Seer can't apply this kind of change yet."); case 'no_access': return t("You don't have access to apply this."); + case 'already_applied': + return t('This was already applied.'); + case 'withdrawn': + return t('Seer no longer suggests this.'); + case 'stale': + return t('This changed since Seer suggested it.'); + case 'validation_failed': + return t("This change wasn't valid."); default: return null; } } -function getMonitorCannotApplyMessage(reason: InvestigationActionApplyErrorCode) { +function getMonitorCannotApplyMessage(reason: ApplyBlockReason) { switch (reason) { case 'permission_denied': return t("You don't have permission to edit this monitor."); @@ -157,6 +178,10 @@ function getMonitorCannotApplyMessage(reason: InvestigationActionApplyErrorCode) return t('This monitor is no longer available.'); case 'not_target_detector': return t('This change targets a different monitor than this investigation.'); + case 'dynamic_detector': + return t("Seer can't change dynamic monitors yet."); + case 'stale': + return t('The monitor changed since Seer suggested this.'); default: return null; } diff --git a/static/app/views/investigations/actionRequests/applyButton.tsx b/static/app/views/investigations/actionRequests/applyButton.tsx index 5ac5de424517..90f95a9aecec 100644 --- a/static/app/views/investigations/actionRequests/applyButton.tsx +++ b/static/app/views/investigations/actionRequests/applyButton.tsx @@ -4,47 +4,37 @@ import {Button} from '@sentry/scraps/button'; import {Flex} from '@sentry/scraps/layout'; import {Text} from '@sentry/scraps/text'; -import {t} from 'sentry/locale'; -import type {ApplyError} from 'sentry/views/investigations/api'; -import type { - InvestigationActionApplyErrorCode, - InvestigationActionRequest, -} from 'sentry/views/investigations/types'; +import type {ActionRequestError} from 'sentry/views/investigations/api'; export type ApplyCheck = | {status: 'pending'} | {status: 'passed'} - | {error: ApplyError; status: 'blocked'} + | {error: ActionRequestError; status: 'blocked'} | {status: 'unavailable'}; type ApplyButtonProps = { - actionRequest: InvestigationActionRequest; applying: boolean; busyLabel: string; + canApply: boolean; check: ApplyCheck; - getCannotApplyMessage: (reason: InvestigationActionApplyErrorCode) => string | null; icon: React.ReactNode; label: string; + message: string | null; onApply: () => void; }; export function ApplyButton({ - actionRequest, applying, busyLabel, + canApply, check, - getCannotApplyMessage, icon, label, + message, onApply, }: ApplyButtonProps) { const messageId = useId(); - const {canApply, cannotApplyReason} = actionRequest; const checking = canApply && check.status === 'pending'; - const message = canApply - ? getCheckMessage(check, getCannotApplyMessage) - : ((cannotApplyReason && getCannotApplyMessage(cannotApplyReason)) ?? - t("This change can't be applied.")); return (