Skip to content

feat(aci): Move monitor project/environment fields from the header to the form body#111762

Open
malwilley wants to merge 3 commits intomasterfrom
malwilley/detector-form-layout
Open

feat(aci): Move monitor project/environment fields from the header to the form body#111762
malwilley wants to merge 3 commits intomasterfrom
malwilley/detector-form-layout

Conversation

@malwilley
Copy link
Copy Markdown
Member

Ref ISWF-2290

This is the first step of applying the refreshed monitor form designs - moving project/env out of the header and into the form body.

Previously, we were rendering these fields within the DetectorBaseFields component which had title/proj/env, but this PR breaks that up into title, project, and env fields. Someone making a new form can choose between ProjectSection and ProjectEnvironmentSection depending on what is required.

Before:
CleanShot 2026-03-27 at 15 11 46

After:
CleanShot 2026-03-27 at 15 10 46

And this is how the other monitor forms look now:
CleanShot 2026-03-27 at 15 13 58
CleanShot 2026-03-27 at 15 14 10
CleanShot 2026-03-27 at 15 14 18

@malwilley malwilley requested a review from a team as a code owner March 27, 2026 22:44
@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 27, 2026

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 27, 2026
@malwilley malwilley changed the title Malwilley/detector form layout feat(aci): Move monitor project/environment fields from the header to the form body Mar 27, 2026
Comment on lines +11 to +12
const {project, detectorType} = useDetectorFormContext();
const canEditDetector = useCanEditDetector({projectId: project.id, detectorType});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The ProjectField validation checks permissions against the initial project from context, not the currently selected project in the form, leading to incorrect permission errors.
Severity: MEDIUM

Suggested Fix

The validation logic should get the currently selected project ID from the form state using useFormField('projectId') instead of relying on the stale project from DetectorFormContext. This will ensure permissions are checked against the user's actual selection.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/views/detectors/components/forms/common/projectField.tsx#L11-L12

Potential issue: The `ProjectField` component's `validate` function relies on
`canEditDetector`, which is calculated using the `project` from
`useDetectorFormContext`. This context is initialized once and is not updated when the
user changes the project in the dropdown. As a result, the permission check is always
performed against the initial project, not the one currently selected in the form. This
can either incorrectly block a valid submission or incorrectly allow a submission for a
project the user lacks permission to edit on the client side.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Copy Markdown
Member Author

@malwilley malwilley Mar 27, 2026

Choose a reason for hiding this comment

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

Hmm this is just code that I moved over so it's not related to theses changes, but there does appear to be a bug here.

Created a linear issue: ISWF-2330

Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

lgtm, i really like this moving towards single module exports (or as closely as we can with styled components).

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