feat(data-forwarding): migrate edit form to useScrapsForm#108806
Conversation
github-actions
bot
added
the
Scope: Frontend
…uilding Move duplicated superRefine validation logic and provider config building from setup and edit forms into shared buildDataForwardingProviderSchema and buildDataForwardingProviderConfig functions in util/forms.tsx. Co-Authored-By: Claude <noreply@anthropic.com>
Replace the as unknown as DataForwarder cast with a dedicated DataForwarderPayload interface that models the snake_case request body sent to the API. This separates the response shape (DataForwarder, camelCase) from the request shape (DataForwarderPayload, snake_case) and removes the need for any unsafe casts. Co-Authored-By: Claude <noreply@anthropic.com>
AvatarProject.id is typed as string | number, causing the default value for project_ids to be inferred as (string | number)[] instead of string[]. This was hidden by the previous as unknown as cast and surfaced by the satisfies DataForwarderPayload check. Co-Authored-By: Claude <noreply@anthropic.com>
…s/data-forwarding-edit-form
| * Builds a provider-specific validation schema by extending the base form schema | ||
| * with required-field checks for the given provider. | ||
| */ | ||
| export function buildDataForwardingProviderSchema(provider: DataForwarderProviderSlug) { |
There was a problem hiding this comment.
I don’t fully understand why this works - I would’ve expected conditional required fields to be defined as z.string().optional() and then make them required here, but since the fields are all defined as z.string(), they are already required 🤔
also, since it’s just 3 things (SQS, SEGMENT and SPLUNK), why don’t we create 3 schemas that all extend a base schema for fields that they all contains?
There was a problem hiding this comment.
ah I guess the empty string default value makes z.string() work, and then this adds the added length check. making 3 schemas with .min(1) still sounds better imo
There was a problem hiding this comment.
Yeah, this current approach more closely reflects the previous state where we had a single useState and then wrote into each form. Now that this is on par with the old functionality and migrated to the new form system, let me create a separate PR to split these into 3 separate schemas with a base that we can wrap fieldGroup around
There was a problem hiding this comment.
yep I’ll review that one 👍
| useEffect( | ||
| () => { | ||
| formModel.setInitialData({...combinedFormState[provider]}); | ||
| formModel.setFormOptions({onFieldChange: updateCombinedFormState}); | ||
| formModel.validateFormCompletion(); | ||
| }, | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [provider, formModel, updateCombinedFormState] |
…s/data-forwarding-edit-form
…#108807) Migrates the per-project override form from the legacy `JsonForm`/`FormModel` system to `useScrapsForm` (TanStack-based), completing the data forwarding form migration. The old implementation rendered each project override as a flat `Panel`/`PanelHeader` with a `JsonForm` inside. The new implementation uses `Disclosure` from `@sentry/scraps/disclosure`, giving a cleaner collapsed/expanded UX with status tags (`Forwarding`, `Forwarding with Overrides`, `Disabled`) visible in the title row without expanding. Also adds `dataForwarderOverrideSchema` to `util/forms.tsx` and removes all remaining legacy helpers (`getProjectOverrideForm`, `getProviderForm`, the provider form constants, and `CalmTag`), leaving `util/forms.tsx` with only `getCreateTooltip` and the two Zod schemas. Stacked on: #108806 Fix DE-945 --------- Co-authored-by: Claude <noreply@anthropic.com>
JonasBa
merged commit 80cdac4
into
jb/settings/data-forwarding-setup-form
![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 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable autofix in the Cursor dashboard.
| overrides, | ||
| is_enabled, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Override form sends all provider fields to API
Medium Severity
The onSubmit handler spreads all form values (minus is_enabled) into overrides, but defaultValues includes fields for every provider (SQS, Segment, and Splunk). For a Segment forwarder, the API receives queue_url: '', region: '', access_key: '', etc. as overrides — fields that don't belong to that provider. The old code dynamically included only the current provider's fields via getProviderForm, so only relevant overrides were submitted. This regression may cause the backend to store spurious empty-string overrides and make hasOverrides incorrectly evaluate to true on reload.
Additional Locations (1)
| is_enabled, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Form state not reset after "Clear Override" succeeds
Medium Severity
The old code had a useEffect that called formModel.setInitialData whenever projectConfig changed, keeping the form synchronized with backend state after mutations. The new useScrapsForm only reads defaultValues at initialization. After the "Clear Override" button triggers a successful mutation and the query is invalidated, the component re-renders with fresh dataForwarder data (empty overrides), but the form retains its stale field values. The user sees old override values in the form even though they were just cleared on the backend.
Additional Locations (1)
2 participants
Migrates the data forwarding edit form from the legacy
JsonForm/FormModelsystem touseScrapsForm(TanStack-based).Same motivation as the setup form migration (#108805) — removes the manual state synchronization layer and replaces it with
useScrapsFormwith Zod validation viasuperRefine.Also removes the now-unused
getDataForwarderFormGroups,getEnablementForm, andgetProjectConfigurationFormhelpers fromutil/forms.tsx. ThegetProjectOverrideFormhelper is kept since it's still used by the project override form (cleaned up in the follow-up PR).Stacked on: #108805