feat(uptime-form-errors): Improving assertion error handling#109352
feat(uptime-form-errors): Improving assertion error handling#109352
Conversation
github-actions
bot
added
the
Scope: Frontend
Abdkhan14
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Non-assertion field validation errors silently swallowed by mapFormErrors
- I added
mapUptimeFormErrorsto preserve and flatten non-assertion validation errors (including nesteddataSourceserrors) when no preview-check assertion error is present, and wired all uptime formmapFormErrorscallbacks to use it.
- I added
Or push these changes by commenting:
@cursor push 33d7390b32
Preview (33d7390b32)
diff --git a/static/app/views/alerts/rules/uptime/formErrors.spec.tsx b/static/app/views/alerts/rules/uptime/formErrors.spec.tsx
--- a/static/app/views/alerts/rules/uptime/formErrors.spec.tsx
+++ b/static/app/views/alerts/rules/uptime/formErrors.spec.tsx
@@ -4,6 +4,7 @@
AssertionFormError,
extractPreviewCheckError,
mapPreviewCheckErrorToMessage,
+ mapUptimeFormErrors,
resolveErroredAssertionOp,
} from 'sentry/views/alerts/rules/uptime/formErrors';
import {
@@ -86,6 +87,26 @@
});
});
+describe('mapUptimeFormErrors', () => {
+ it('passes through top-level field validation errors', () => {
+ const body = {url: ['Invalid URL']};
+ expect(mapUptimeFormErrors(body)).toEqual(body);
+ });
+
+ it('flattens dataSources field validation errors', () => {
+ const body = {
+ dataSources: {url: ['Invalid URL format'], method: ['Method is required']},
+ nonFieldErrors: ['Request invalid'],
+ };
+
+ expect(mapUptimeFormErrors(body)).toEqual({
+ nonFieldErrors: ['Request invalid'],
+ url: ['Invalid URL format'],
+ method: ['Method is required'],
+ });
+ });
+});
+
describe('resolveErroredAssertionOp', () => {
it('resolves errored op from assertion_failure_data by matching value', () => {
const target = makeStatusCodeOp({
diff --git a/static/app/views/alerts/rules/uptime/formErrors.tsx b/static/app/views/alerts/rules/uptime/formErrors.tsx
--- a/static/app/views/alerts/rules/uptime/formErrors.tsx
+++ b/static/app/views/alerts/rules/uptime/formErrors.tsx
@@ -41,6 +41,31 @@
};
}
+function flattenDataSourceFormErrors(responseJson: any): any {
+ if (!responseJson || typeof responseJson !== 'object' || Array.isArray(responseJson)) {
+ return responseJson;
+ }
+
+ const dataSources = responseJson.dataSources;
+ if (!dataSources || typeof dataSources !== 'object' || Array.isArray(dataSources)) {
+ return responseJson;
+ }
+
+ const {dataSources: _, ...rest} = responseJson;
+ return {...rest, ...dataSources};
+}
+
+export function mapUptimeFormErrors(
+ responseJson: any,
+ previewCheckError?: PreviewCheckError | null
+): any {
+ const mappedPreviewCheckError = mapToFormErrors(
+ previewCheckError ?? extractPreviewCheckError(responseJson)
+ );
+
+ return mappedPreviewCheckError ?? flattenDataSourceFormErrors(responseJson);
+}
+
function isPreviewCheckError(value: any): value is PreviewCheckError {
return (
value !== null &&
diff --git a/static/app/views/alerts/rules/uptime/uptimeAlertForm.tsx b/static/app/views/alerts/rules/uptime/uptimeAlertForm.tsx
--- a/static/app/views/alerts/rules/uptime/uptimeAlertForm.tsx
+++ b/static/app/views/alerts/rules/uptime/uptimeAlertForm.tsx
@@ -41,7 +41,7 @@
import {createEmptyAssertionRoot, UptimeAssertionsField} from './assertions/field';
import {AssertionSuggestionsButton} from './assertionSuggestionsButton';
-import {extractPreviewCheckError, mapToFormErrors} from './formErrors';
+import {extractPreviewCheckError, mapUptimeFormErrors} from './formErrors';
import {HTTPSnippet} from './httpSnippet';
import {PreviewCheckResultProvider, usePreviewCheckResult} from './previewCheckContext';
import {TestUptimeMonitorButton} from './testUptimeMonitorButton';
@@ -221,7 +221,7 @@
mapFormErrors={responseJson => {
const extractedError = extractPreviewCheckError(responseJson);
previewCheckResult?.setPreviewCheckError(extractedError);
- return mapToFormErrors(extractedError);
+ return mapUptimeFormErrors(responseJson, extractedError);
}}
onFieldChange={onFieldChange}
onPreSubmit={() => {
diff --git a/static/app/views/detectors/components/forms/uptime/index.tsx b/static/app/views/detectors/components/forms/uptime/index.tsx
--- a/static/app/views/detectors/components/forms/uptime/index.tsx
+++ b/static/app/views/detectors/components/forms/uptime/index.tsx
@@ -7,7 +7,7 @@
import type {UptimeDetector} from 'sentry/types/workflowEngine/detectors';
import {
extractPreviewCheckError,
- mapToFormErrors,
+ mapUptimeFormErrors,
} from 'sentry/views/alerts/rules/uptime/formErrors';
import {
PreviewCheckResultProvider,
@@ -100,7 +100,7 @@
mapFormErrors={responseJson => {
const extractedError = extractPreviewCheckError(responseJson);
previewCheckResult?.setPreviewCheckError(extractedError);
- return mapToFormErrors(extractedError);
+ return mapUptimeFormErrors(responseJson, extractedError);
}}
>
<UptimeDetectorForm />
@@ -139,7 +139,7 @@
mapFormErrors={responseJson => {
const extractedError = extractPreviewCheckError(responseJson);
previewCheckResult?.setPreviewCheckError(extractedError);
- return mapToFormErrors(extractedError);
+ return mapUptimeFormErrors(responseJson, extractedError);
}}
>
<UptimeDetectorForm />
Abdkhan14
changed the title
jaydgoss
left a comment
jaydgoss
left a comment
There was a problem hiding this comment.
Good feature — the context-based approach for sharing preview check results and the tree-walking logic to resolve errored assertion ops are well-designed. Two null-safety bugs need fixing before merge, plus a few code quality suggestions inline.
| } | ||
|
|
||
| export function mapToFormErrors(responseJson: any) { | ||
| const error = extractPreviewCheckError(responseJson); |
There was a problem hiding this comment.
mapToFormErrors crashes when responseJson is null/undefined.
The deleted mapAssertionFormErrors had an explicit if (!responseJson) return responseJson; guard at the top. Here, extractPreviewCheckError uses optional chaining so it survives null, but line 47 accesses responseJson.dataSources directly — that will throw a TypeError.
FormModel.submitError passes err.responseJSON which can be undefined when the API returns no body.
Suggestion: add if (!responseJson) return responseJson; at the top of this function.
| 'assertion' in value && | ||
| 'error' in value.assertion | ||
| ); | ||
| } |
There was a problem hiding this comment.
isPreviewCheckError crashes when value.assertion is null.
The in operator throws TypeError when the right-hand side is null, undefined, or any primitive. If the API error response contains {assertion: null}, this will blow up and break all error handling paths.
Suggestion: add null/type guards before the 'error' in value.assertion check:
function isPreviewCheckError(value: any): value is PreviewCheckError {
return (
value !== null &&
typeof value === 'object' &&
'assertion' in value &&
typeof value.assertion === 'object' &&
value.assertion !== null &&
'error' in value.assertion
);
}| mapFormErrors={responseJson => { | ||
| previewCheckResult?.setPreviewCheckError(extractPreviewCheckError(responseJson)); | ||
| return mapToFormErrors(responseJson); | ||
| }} |
There was a problem hiding this comment.
nit: duplicated callback, this exact inline lambda appears in 3 places (here and twice in detectors/.../index.tsx). Consider extracting a shared helper in formErrors.tsx to reduce duplication and keep the behavior consistent:
export function createMapFormErrors(
previewCheckResult: ReturnType<typeof usePreviewCheckResult>
) {
return (responseJson: any) => {
previewCheckResult?.setPreviewCheckError(extractPreviewCheckError(responseJson));
return mapToFormErrors(responseJson);
};
}Then each call site becomes just mapFormErrors={createMapFormErrors(previewCheckResult)}.
| </Tooltip> | ||
| </span> | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: Inline style prop, the frontend guide recommends using design system primitives or Emotion over inline styles. This could use <Flex> or a small styled component to stay consistent with the rest of the codebase.
| <PreviewCheckResultContext.Provider | ||
| value={{ | ||
| ...state, | ||
| setPreviewCheckData, |
There was a problem hiding this comment.
nit: unstable context value, the setPreviewCheckData, setPreviewCheckError, and resetPreviewCheckResult callbacks are plain functions recreated every render, and the spread {...state, ...} creates a new object reference each time. This causes all context consumers to re-render on every provider render.
Consider wrapping callbacks in useCallback and the value object in useMemo:
const setPreviewCheckData = useCallback(
(data: PreviewCheckResult | null) => setState({data, error: null}),
[]
);
// ... same for the others
const value = useMemo(
() => ({...state, setPreviewCheckData, setPreviewCheckError, resetPreviewCheckResult}),
[state, setPreviewCheckData, setPreviewCheckError, resetPreviewCheckResult]
);
| msg: 'bad path', | ||
| assertPath: ['0', '0'], // slice(1) = ['0'] → children[0] = first | ||
| }, | ||
| }, |
There was a problem hiding this comment.
optional: The frontend testing guide recommends against mocking hooks. For the AssertionFormError component tests, you could wrap the component in <PreviewCheckResultProvider> and set state through the provider's API rather than mocking usePreviewCheckResult via jest.spyOn. This would test closer to how the component actually runs.
That said, since this is a custom context (not a standard Sentry hook like useOrganization), this is more pragmatic, totally fine to leave as-is.
There was a problem hiding this comment.
Yeah, pushed a Setter that sets the values on mount, thanks!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
2 participants
Added previewCheckContext:
Used the value from previewCheckContext to resolve the errored assertion op:
/preview -check/was succeeded but the check-in didn't. Ex: Evaluation and verification errorserroredOptoAssertionOpGroupto render errors in-lineAdded tests