Skip to content

feat(aci): Edit detector form #93579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Jun 17, 2025
Merged

feat(aci): Edit detector form #93579

merged 25 commits into from
Jun 17, 2025

Conversation

scttcper
Copy link
Member

@scttcper scttcper commented Jun 14, 2025

building off the work in #93380 this pr adds the ability to edit a dector.

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 14, 2025
Copy link

codecov bot commented Jun 16, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
10593 1 10592 9
View the top 1 failed test(s) by shortest run time
useReplayCount getOne & hasOne should return undefined to start, then the count after data is loaded
Stack Traces | 0.102s run time
Error: expect(received).toBe(expected) // Object.is equality

Expected: 5
Received: undefined
    at Object.<anonymous> (.../utils/replayCount/useReplayCount.spec.tsx:57:45)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Base automatically changed from scttcper/submit-detector-form to master June 16, 2025 18:23
# Conflicts:
#	static/app/views/detectors/components/forms/metricFormData.tsx
#	static/app/views/detectors/edit.tsx
#	static/app/views/detectors/new-settings.tsx
@scttcper scttcper changed the title feat(aci): Allow submitting detector form feat(aci): Edit detector form Jun 16, 2025
@scttcper scttcper marked this pull request as ready for review June 16, 2025 21:44
@scttcper scttcper requested a review from a team as a code owner June 16, 2025 21:44
@@ -61,7 +61,7 @@ export interface MetricDetectorFormData
MetricDetectorDynamicFormData {
aggregate: string;
environment: string;
kind: 'threshold' | 'change' | 'dynamic';
kind: 'static' | 'percent' | 'dynamic';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are more in line with the values the backend is using


// Extract aggregate and visualize from the aggregate string
// Format is typically "count(transaction.duration)"
const aggregateMatch = snubaQuery?.aggregate?.match(/^(\w+)\(([^)]+)\)$/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this always work? What are the other possible values?

I wonder if we should be using a parser for this or returning as an object form the backend? Work for later though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let me dig up what we do in other places

const {mutateAsync: updateDetector} = useUpdateDetector();

const handleSubmit = useCallback<OnSubmitCallback>(
async (data, _, __, ___, formModel) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta love functions with 5 arguments

...getNewMetricDetectorData(data as MetricDetectorFormData),
};

const updatedDetector = await updateDetector(updatedData);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no :(

@scttcper scttcper merged commit 36bc7f3 into master Jun 17, 2025
52 of 54 checks passed
@scttcper scttcper deleted the scttcper/edit-detector-form branch June 17, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants