Skip to content

ref(forms): migrate projectFiltersSettings to scraps form system#115783

Merged
TkDodo merged 25 commits into
masterfrom
tkdodo/ref/de-985-migrate-projectfiltersettings
May 21, 2026
Merged

ref(forms): migrate projectFiltersSettings to scraps form system#115783
TkDodo merged 25 commits into
masterfrom
tkdodo/ref/de-985-migrate-projectfiltersettings

Conversation

@TkDodo
Copy link
Copy Markdown
Collaborator

@TkDodo TkDodo commented May 19, 2026

Migrates the project inbound filters settings page from the legacy form system (Form, JsonForm, FormField, FieldFromConfig) to the new scraps form system (AutoSaveForm, useScrapsForm, FieldGroup).

Standard filter toggles & legacy browser filter

Each filter toggle (browser-extensions, localhost, web-crawlers, etc.) is now an AutoSaveForm with field.Switch. The legacy browser filter uses AutoSaveForm with a string[] field value and a custom field.Base render for the browser grid UI. Both share mutation options via a getFilterMutationOptions factory. Optimistic updates go through the filter list query cache instead of local useState, so AutoSaveForm's reset-after-save works correctly across rapid toggles.

Project-level boolean filters

Hydration errors and ChunkLoadError toggles use AutoSaveForm backed by the detailed project query cache. Optimistic updates write to both the query cache and ProjectsStore to keep the sidebar and other consumers in sync.

Custom filter textareas

The saveOnBlur: false textarea fields (IP addresses, releases, error messages, etc.) are unified into a single useScrapsForm with explicit Save/Cancel buttons, replacing the old JsonForm + inboundFilters.tsx config. inboundFilters.tsx is deleted since it was only imported here.

FormSearch & field registry

Wraps the migrated forms with FormSearch for settings search indexing and regenerates generatedFieldRegistry.ts.

Refs DE-985

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 19, 2026

DE-985

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 19, 2026
@TkDodo
Copy link
Copy Markdown
Collaborator Author

TkDodo commented May 19, 2026

bugbot run

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

📊 Type Coverage Diff

Metric Before After Delta
Coverage 93.60% 93.60% ±0%
Typed 135,623 135,660 🟢 +37
Untyped 9,281 9,271 🟢 -10
🔍 2 new type safety issues introduced

Type assertions (as) (2 new)

File Line Detail
static/app/views/settings/project/projectFilters/projectFiltersSettings.tsx 592 as StandardFilterIdfilter.id as StandardFilterId
static/app/views/settings/project/projectFilters/projectFiltersSettings.tsx 783 as keyof typeof filterDescriptionsfilter.id as keyof typeof filterDescriptions

This is informational only and does not block the PR.

Comment thread static/app/views/settings/project/projectFilters/projectFiltersSettings.tsx Outdated
@TkDodo TkDodo marked this pull request as ready for review May 19, 2026 17:00
@TkDodo TkDodo requested a review from a team as a code owner May 19, 2026 17:00
@priscilawebdev
Copy link
Copy Markdown
Member

I like that we show this info only 1x now, but shouldn't it take the whole available horizontal space?

image

@priscilawebdev
Copy link
Copy Markdown
Member

Screenshot 2026-05-20 at 12 22 28

We should remove the spacing around this

@priscilawebdev
Copy link
Copy Markdown
Member

Since we now only show one piece of information at the bottom, does it make sense to update the copy as well? Users might wonder which filter it refers to.

image

@priscilawebdev
Copy link
Copy Markdown
Member

priscilawebdev commented May 20, 2026

This is flickering a bit

Screen.Recording.2026-05-20.at.12.29.26.mov

TkDodo and others added 4 commits May 20, 2026 13:41
…gate

The IP address filter (filters:blacklisted_ips) was incorrectly moved inside
the projects:custom-inbound-filters Feature wrapper during the form migration.
In master it was rendered by the parent JsonForm, outside the feature gate.

Move it back outside so it remains visible for all projects regardless of
the feature flag. Add a test to prevent regression.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
@TkDodo
Copy link
Copy Markdown
Collaborator Author

TkDodo commented May 20, 2026

I like that we show this info only 1x now, but shouldn't it take the whole available horizontal space?

I’ve done this the same way as we have it in Teams settings:

Screenshot 2026-05-20 at 13 35 51

We should remove the spacing around this

It’s consistent with other messages:

Screenshot 2026-05-20 at 13 32 52

Since we now only show one piece of information at the bottom, does it make sense to update the copy as well? Users might wonder which filter it refers to.

I don’t understand what you mean with “update the copy as well”. Please elaborate what you think should be done differently.

This is flickering a bit

Yeah it’s flickering because the toggles disable while the mutation is in-flight but the mutation is very fast here.

const isDisabledByAutoSave = autoSaveContext?.status === 'pending';

We could change it for this instance but I’d like to avoid race conditions because users click multiple toggles fast.

since you are touching this file, could you please add the t() around this string

done: e0c5b5a

also since we are touching this file, shouldn't we remove the _features?

good point, not sure what they were there for in the first place: e471789, 732dc3d

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd29cb3. Configure here.

Comment thread static/app/views/settings/project/projectFilters/projectFiltersSettings.tsx Outdated
@priscilawebdev
Copy link
Copy Markdown
Member

Thanks applying my feedback.

Regarding the bluish/purplish alert at the bottom: you mentioned it should align with what we render when changing an org slug, but in the org slug flow, the alert appears above the “Cancel” and “Save” buttons and spans the full available width. Shouldn’t we keep this consistent here as well, even if the copy is shorter?

image image

@priscilawebdev
Copy link
Copy Markdown
Member

About the copy: previously, the "Changing this filter will apply to all new events." message was rendered directly below the field being changed, so the context was clear. Now, it’s ambiguous whether it refers to A, B, C, D, or E (see screenshot). It’s a minor nit, but I’d probably update the wording to something more general to avoid confusion.

image

A few suggestions:

"Changes will only apply to new events."
"Changes to these filters will only apply to new events."

@priscilawebdev
Copy link
Copy Markdown
Member

I noticed that when the user doesn’t have access and the form is disabled, we don’t render the "Cancel" and "Save" buttons. But shouldn’t we still render them in a disabled state?

Other already-migrated forms in Settings do show those buttons, even when disabled, such as in Dynamic Sampling:

image

@priscilawebdev
Copy link
Copy Markdown
Member

priscilawebdev commented May 21, 2026

I noticed that when the user doesn’t have access and the form is disabled, we don’t render the "Cancel" and "Save" buttons. But shouldn’t we still render them in a disabled state?

Other already-migrated forms in Settings do show those buttons, even when disabled, such as in Dynamic Sampling:

image

never mind. in production, we only show the save/cancel buttons if the form is dirty:

image

Comment thread static/app/views/settings/project/projectFilters/index.spec.tsx Outdated
Comment thread static/app/views/settings/project/projectFilters/index.spec.tsx Outdated
Comment thread static/app/views/settings/project/projectFilters/index.spec.tsx Outdated
@TkDodo
Copy link
Copy Markdown
Collaborator Author

TkDodo commented May 21, 2026

but in the org slug flow, the alert appears above the “Cancel” and “Save” buttons and spans the full available width.

the org slug flow was probably migrated 1:1 from the old flow. I think we’ve agreed that showing cancel/save buttons after typing something in is not the flow we want. Either fields auto-save, or they have a consistent save/cancel button shown with a message next to them. We can flex the message to take the full width but right now, messages next to save/cancel don’t have that. Happy to streamline in a follow-up.

Other already-migrated forms in Settings do show those buttons, even when disabled, such as in Dynamic Sampling

Hmm we already have a banner at the top saying “These settings can only be edited by users with the organization-level owner, manager, or team-level admin roles.” so adding another disabled button doesn’t really help here I believe

never mind. in production, we only show the save/cancel buttons if the form is dirty:

yeah I changed UX a little bit here because again, those save/cancel buttons popping in per row is not what we want.

Comment thread static/app/views/settings/project/projectFilters/index.spec.tsx Outdated
Comment thread static/app/views/settings/project/projectFilters/index.spec.tsx Outdated
Copy link
Copy Markdown
Member

@priscilawebdev priscilawebdev left a comment

Choose a reason for hiding this comment

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

🙌

@TkDodo TkDodo merged commit 738f7dc into master May 21, 2026
71 checks passed
@TkDodo TkDodo deleted the tkdodo/ref/de-985-migrate-projectfiltersettings branch May 21, 2026 09:26
JonasBa pushed a commit that referenced this pull request May 21, 2026
…5783)

Migrates the project inbound filters settings page from the legacy form
system (`Form`, `JsonForm`, `FormField`, `FieldFromConfig`) to the new
scraps form system (`AutoSaveForm`, `useScrapsForm`, `FieldGroup`).

**Standard filter toggles & legacy browser filter**

Each filter toggle (browser-extensions, localhost, web-crawlers, etc.)
is now an `AutoSaveForm` with `field.Switch`. The legacy browser filter
uses `AutoSaveForm` with a `string[]` field value and a custom
`field.Base` render for the browser grid UI. Both share mutation options
via a `getFilterMutationOptions` factory. Optimistic updates go through
the filter list query cache instead of local `useState`, so
`AutoSaveForm`'s reset-after-save works correctly across rapid toggles.

**Project-level boolean filters**

Hydration errors and ChunkLoadError toggles use `AutoSaveForm` backed by
the detailed project query cache. Optimistic updates write to both the
query cache and `ProjectsStore` to keep the sidebar and other consumers
in sync.

**Custom filter textareas**

The `saveOnBlur: false` textarea fields (IP addresses, releases, error
messages, etc.) are unified into a single `useScrapsForm` with explicit
Save/Cancel buttons, replacing the old `JsonForm` + `inboundFilters.tsx`
config. `inboundFilters.tsx` is deleted since it was only imported here.

**FormSearch & field registry**

Wraps the migrated forms with `FormSearch` for settings search indexing
and regenerates `generatedFieldRegistry.ts`.

Refs DE-985

---------

Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
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