ref(settings): Migrate sentry app form to useScrapsForm#114138
Conversation
github-actions
Bot
added
the
Scope: Frontend
priscilawebdev
force-pushed
the
priscilawebdev/ref/sentry-app-scraps-form
branch
from
4ebf068 to
ea7d334
Compare
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 691dbf4. Configure here.
📊 Type Coverage Diff
🔍 3 new type safety issues introduced
Non-null assertions (
Type assertions (
This is informational only and does not block the PR. |
priscilawebdev
force-pushed
the
priscilawebdev/ref/sentry-app-scraps-form
branch
from
7e4899c to
de0a626
Compare
priscilawebdev
changed the base branch from
master
to
priscila/ref/sentry-app-token-mutations
Backend rejects oversized scope requests with messages like 'Requested permission of member:write exceeds…'. The legacy form parsed those, mapped each to its permission resource, and rendered the message under the matching dropdown. The tanstack migration dropped that path so scope errors were silently lost. Reintroduce the regex-based scope-to-resource mapping, plumb a permissionErrors map through PermissionsObserver to PermissionSelection, and render the message beneath each select. Events errors keep the toast path since they have no inline UI. The spec asserts the visible text again instead of spying on addErrorMessage. Co-Authored-By: Claude <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const payload: SaveSentryAppPayload = { | ||
| name: value.name, | ||
| organization: value.organization, | ||
| webhookUrl: value.webhookUrl, | ||
| isAlertable: value.isAlertable, |
There was a problem hiding this comment.
Bug: The webhookUrl field is sent as an empty string instead of null when left blank, unlike other optional fields, potentially causing API errors.
Severity: MEDIUM
Suggested Fix
In the payload construction, coerce the webhookUrl value to null if it's an empty string, similar to how redirectUrl and overview are handled. Change webhookUrl: value.webhookUrl to webhookUrl: value.webhookUrl || null.
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/settings/organizationDeveloperSettings/sentryApplicationDetails.tsx#L499-L503
Potential issue: When creating or editing an internal Sentry App without a webhook URL,
the form's initial value for `webhookUrl` is set to an empty string (`''`) instead of
`null`. When the form is submitted, the payload sends `webhookUrl: ''`. This is
inconsistent with other optional fields like `author` and `redirectUrl`, which are
explicitly coerced to `null` if empty. This could lead to API validation failures if the
backend expects `null` for an absent webhook URL, rather than an empty string.
permObj.choices[*].scopes is typed as Scope[], so allScopes.includes narrows the argument to the Scope union and rejects the string value coming back from the regex match. Widen the local to string[] so the runtime check compiles. Co-Authored-By: Claude <noreply@anthropic.com>
getResourceFromScope only walks SENTRY_APP_PERMISSIONS, which doesn't include the special continuous integration scope (org:ci). Backend errors for org:ci dropped silently — no inline message, and the scopes form field captured by setFieldErrors suppressed the toast fallback. Add a CI branch to the scope mapper, plumb a continuousIntegrationError through PermissionsObserver to PermissionSelection, and render it beneath the CI checkbox. Restore the spec assertion to org:ci to match master parity. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Reviewed by Cursor Bugbot for commit 3de7f1a. Configure here.
| if (isInternal() && !value && isAlertable) { | ||
| fieldApi.form.setFieldValue('isAlertable', false); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Unused variable webhookDisabled in listener callback
Low Severity
The webhookDisabled variable inside the webhookUrl field's onChange listener is computed but never referenced. The same condition isInternal() && !value is duplicated directly in the if statement below it, making webhookDisabled dead code left over from the refactor.
Reviewed by Cursor Bugbot for commit 3de7f1a. Configure here.
There was a problem hiding this comment.
this finding is stale
author, overview, and redirectUrl are coerced from '' to null in the submit payload — bring webhookUrl in line. The backend already accepts both via allow_blank=True, so this is just a consistency nit for the request body shape. Co-Authored-By: Claude <noreply@anthropic.com>
If responseJSON.scopes contains a message that doesn't match the 'Requested permission of resource:access' regex, mapScopeErrors returns nothing, but setFieldErrors still latches onto the scopes form field and short-circuits the toast fallback. Surface the first scope message via toast when nothing mapped inline so the user isn't left with a silent failure. Co-Authored-By: Claude <noreply@anthropic.com>
| events: WebhookEvent[]; | ||
| newApp: boolean; | ||
| scopes: Scope[]; | ||
| continuousIntegrationError?: string; | ||
| onEventsChange?: (events: WebhookEvent[]) => void; | ||
| onScopesChange?: (scopes: Scope[]) => void; | ||
| permissionErrors?: Partial<Record<PermissionResource, string>>; | ||
| }; | ||
|
|
||
| type State = { |
There was a problem hiding this comment.
Bug: The PermissionsObserver component does not update its state when its props change, causing the UI to display stale permission data after background data refetches.
Severity: MEDIUM
Suggested Fix
Implement the componentDidUpdate lifecycle method in the PermissionsObserver component. Inside this method, compare the previous props with the current props. If props.scopes or props.events have changed, update the component's state accordingly by calling this.setState.
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/settings/organizationDeveloperSettings/permissionsObserver.tsx#L32-L41
Potential issue: The `PermissionsObserver` component is a class component that
initializes its internal state from props (`scopes`, `events`) only once in the
constructor. It lacks a `componentDidUpdate` lifecycle method to synchronize its state
when new props are received. When a parent component refetches data in the background,
it passes updated props to `PermissionsObserver`. However, the component does not
re-render with the new data, causing its internal state and the UI to become stale. This
leads to a desynchronization where the displayed permissions are incorrect and do not
reflect the actual form state.
| ); | ||
| }; | ||
|
|
||
| const scopes = (app && [...app.scopes]) || []; | ||
| const events = (app && normalize(app.events)) || []; | ||
| const method = app ? 'PUT' : 'POST'; | ||
| const endpoint = app ? `/sentry-apps/${app.slug}/` : '/sentry-apps/'; | ||
|
|
||
| const forms = isInternal() ? internalIntegrationForms : publicIntegrationForms; | ||
| let verifyInstall: boolean; | ||
| if (isInternal()) { | ||
| // force verifyInstall to false for all internal apps | ||
| verifyInstall = false; | ||
| } else { | ||
| // use the existing value for verifyInstall if the app exists, otherwise default to true | ||
| verifyInstall = app ? app.verifyInstall : true; | ||
| } | ||
| const defaultValues = { | ||
| name: app?.name ?? '', | ||
| author: app?.author ?? '', | ||
| webhookUrl: app?.webhookUrl ?? '', | ||
| redirectUrl: app?.redirectUrl ?? '', | ||
| verifyInstall: isInternal() ? false : (app?.verifyInstall ?? true), |
There was a problem hiding this comment.
Bug: The form initializes with empty defaultValues before async data has loaded. The form is not updated after data fetch, risking data loss on save.
Severity: HIGH
Suggested Fix
Conditionally render the form component only after the app data has been successfully fetched. Alternatively, use a useEffect hook to observe changes in the app data and call form.reset() with the new values once the data becomes available to correctly populate the form.
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/settings/organizationDeveloperSettings/sentryApplicationDetails.tsx#L467-L475
Potential issue: When a user navigates directly to the edit page for a Sentry App, the
form can initialize before the application data is fetched. The `useScrapsForm` hook is
called with empty `defaultValues` because the `app` data is `undefined` during the
initial render. After the data loads and the component re-renders, the existing form
instance is not updated with the fetched data. This results in the user being shown a
blank form. If the user saves, it will overwrite the app's configuration with empty
values, leading to data loss.
The backend updater for sentry apps gates each field on 'if self.X is not None' (src/sentry/sentry_apps/logic.py), so a null in the request body silently skips the write — the user's clearing attempt is lost and the old value sticks. Submit '' instead for webhookUrl, redirectUrl and overview, which the backend parsers accept (allow_blank=True) and the updater treats as a real value. author keeps the null path because its parser doesn't allow blank and internal apps may legitimately have no author. Also patch the index list cache on submit success so the list page paints the new name on first frame instead of flashing the cached stale value before the background refetch lands. Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
closes https://linear.app/getsentry/issue/DE-970/migrate-custom-integrations-form-from-legacy-form-system