ref(projectfilters) move checkbox to left #108090
Conversation
github-actions
bot
added
the
Scope: Frontend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Add checkboxes to EnvironmentPageFilter, FilterSelector, and TestSuiteDropdown - Fix project filter search by adding textValue property (was broken due to JSX label) - Fix menu width calculation to use textValue instead of stringifying JSX label - Use HybridFilterRef with imperative handle pattern for all filters
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.
|
|
||
| return ( | ||
| <HybridFilter | ||
| ref={hybridFilterRef} |
There was a problem hiding this comment.
Prop spread ordering may override internal ref
Low Severity
In ProjectPageFilter, ref={hybridFilterRef} is placed before {...selectProps}, meaning selectProps can override the internal ref. Since ref isn't omitted from ProjectPageFilterProps and isn't destructured, it ends up in selectProps. If an external ref is passed, hybridFilterRef.current becomes null and all checkbox onChange handlers silently do nothing. The EnvironmentPageFilter correctly places {...selectProps} before ref, ensuring the internal ref always wins.
Additional Locations (1)
Updates test to include leadingItems with checkboxes for each filter option, matching the new implementation pattern after HybridFilter refactoring.
| map.set(value, { | ||
| label: middleEllipsis(value, 70, /[\s-_:]/), | ||
| value, | ||
| leadingItems: ({isSelected}: {isSelected: boolean}) => ( |
There was a problem hiding this comment.
The reason we have to set this manually is because the hybrid filter no longer manages it for us. As it turns out, hybridFilter has a "hideCheck" option that removes the checkbox from compactSelect, which is how it was able to hide it, and then abuse trailingItems or leadingItems to toggle where the rendering of it was supposed to be...
There was a problem hiding this comment.
hybridFilter should not have ever been exposed as a component outside of pagefilters, I am hoping that the prevent instance can soon be removed, and that we can do something about the dashboard case, but it requires more investigation
There was a problem hiding this comment.
we talked about this briefly in slack, but why can’t we just get rid of hideCheck completely and just use the built-in checkbox from compactSelect ?
compactSelect already has a way to have multi-selects “stay open” after a selection has been made, so I guess the main problem is that selecting one value in the checkbox immediately triggers onChange and updates the url and everything.
Maybe this should be a built-in feature of compactSelect, or maybe even all compactSelects with multiple: true should behave that way? Isn’t it weird that clicking a checkbox sometimes immediately applies and sometimes not, like e.g. here:
https://demo.sentry.io/issues/alerts/rules/?statsPeriod=90d
Teams and AlertType instantly apply, but Projects does not. This isn’t great in terms of “least surprise” ...
Screen.Recording.2026-02-12.at.09.19.28.mov
If we’d unify this, I think using what the ProjectsFilter does right now for everything (built into compactSelect) wouldn’t be bad:
- you click the label, it instantly applies
- you click the checkbox or cmd-click the label, it selects but doesn’t apply and shows the
Apply/Cancelin the footer andresetin the header.
It would be a change in UX for some places but arguably for the better.
@Jesse-Box FYI
2 participants
Moves checkbox position to the beginning of the row and unwinds the mess of checkwraps.
Fix DE-752