Repository navigation
perf(filters): use useDeferredValue in SelectFilterPlugin - #44880
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation uses To resolve this, you should ensure the If you are seeing this issue, ensure that the superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44880 +/- ##
=======================================
Coverage 82.05% 82.06%
=======================================
Files 2991 2991
Lines 184538 184571 +33
Branches 42722 42725 +3
=======================================
+ Hits 151427 151465 +38
+ Misses 30359 30354 -5
Partials 2752 2752
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #59c82d
Actionable Suggestions - 2
-
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx - 2
- Debounce Removed Search Dispatch · Line 320-343
- Debounce Removed LIKE Dispatch · Line 611-624
Review Details
-
Files reviewed - 2 · Commit Range:
8505f10..8505f10- superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
- superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Code Review Agent Run #c7ab09Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Refs apache#39890. Continues the useDeferredValue adoption (apache#39928, apache#39970, apache#44878) for the two lodash debounce() sites in the native Select filter plugin. Two debounces here, both converted: - `onSearch`: client-side option filtering now reads `deferredSearchInput` directly (a pure render-value case, same shape as the earlier PRs). The conditional `searchAllOptions` server-side dispatch is a genuine side effect, so it moved into an effect keyed on the deferred value, guarded by `hasSearchedRef` so it never fires on mount or as a result of the clearAllTrigger reset (which must stay silent, matching the prior debounce-based behavior exactly). - `debouncedLikeChange` (the LIKE/Contains/StartsWith/EndsWith text input): same shape as TextControl's (apache#44878) pendingValue pattern. `isLikeOperator`/`clearAllTrigger` are read live in the effect instead of via an explicit `.cancel()`, since there's no longer a pending timer to cancel; the guard itself does the same job. Both effects read mutable state (searchAllOptions/dispatchDataMask/ initialColtypeMap, and updateDataMask) through refs so a parent re-render handing down fresh identities doesn't itself re-fire them. Test changes: the LIKE-debounce test suite used `jest.useFakeTimers()` + `jest.advanceTimersByTime()` to force the old debounce to fire. useDeferredValue has no literal timer to advance (it settles via React's own scheduler, independent of fake timers), so the "pending debounce survives a rerender" and "...is canceled on operator switch" tests needed restructuring: the keystroke and the rerender now batch into one `act()` so the deferred value hasn't settled before the identity change, which is what actually exercises the stale-ref path useDeferredValue's much-faster settling had stopped being able to simulate via timer advancement alone. `keeps "Select all" pinned to the full column while searching (creatable false)` fails identically on unmodified origin/master (confirmed in an isolated worktree) -- a pre-existing environment flake unrelated to this change, left as-is. Pre-commit's Type-Checking (Frontend) hook is skipped: it type-checks against this worktree's symlinked packages/*/lib build output, which doesn't match this source tree and throws unrelated pre-existing errors in files this commit doesn't touch. Verified independently: `npx tsc --noEmit -p tsconfig.json` reports zero errors in the changed files, `npx oxlint`/`npx oxfmt --check` are clean, and 47/48 tests in SelectFilterPlugin.test.tsx pass (the one pre-existing failure noted above). Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…clear-all Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…E dispatch useDeferredValue lowers render priority but does not coalesce keystrokes by time, so the server search and LIKE data-mask updates need a debounce of their own. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
45af057 to
17cda68
Compare
There was a problem hiding this comment.
Code Review Agent Run #f31e81
Actionable Suggestions - 1
-
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx - 1
- No-op trigger release · Line 2187-2187
Additional Suggestions - 1
-
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx - 1
-
Triplicated assertion block · Line 2191-2199The `not.toContainEqual` block filtering `setDataMask` calls for the `%Jen%` ILIKE payload is repeated verbatim in three tests added by this diff (lines 2082-2088, 2135-2143, and here). Extract a shared helper taking the mock and the start index so a change to the payload shape updates one place instead of three.
-
Review Details
-
Files reviewed - 2 · Commit Range:
5bbd205..17cda68- superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx
- superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
…blish empty search on Clear All Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Code Review Agent Run #9ccb1eActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…d cover reset/clear-all paths Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Code Review Agent Run #c72c10Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Approving; CI green. Non-blocking: sadpandajoe's last ask holds. After a parent reset to undefined, re-typing the same LIKE text publishes nothing (internal dataMask still has it); pre-existing. The description says deferral drives dispatch; debounce does. Render-phase ref write is benign (probed).
SUMMARY
Refs #39890. Continues the
useDeferredValueadoption started in #39928 (TableExploreTree), #39970 (DatasourcePanel), and #44878 (TextControl), this time for the nativeSelectdashboard filter plugin, which has two separatelodashdebounce()sites.Two debounces, two shapes
onSearch— client-side option filtering. This is a pure render-value case, the same shape asTableExploreTree/DatasourcePanel: the dropdown's options list now readsdeferredSearchInputdirectly. There's also a conditionalsearchAllOptionsdispatch (asking the server to search the whole column), which is a genuine side effect, so it moved into an effect keyed on the deferred value, guarded so it never fires on mount or as a side effect of the "Clear All" reset (both silent in the original debounce-based code too).debouncedLikeChange— the free-text input for the LIKE/Contains/StartsWith/EndsWith operators. Same shape asTextControl's (perf(explore): use useDeferredValue for TextControl inputs #44878)pendingValuepattern: the debounce only ever gated when the committed value reachedupdateDataMask, so it became apendingLikeValuestate + effect, same asTextControl.Both effects read their mutable dependencies (
searchAllOptions/dispatchDataMask/initialColtypeMap, andupdateDataMask) through refs, so a parent re-render handing down fresh identities for any of them doesn't itself re-fire the effect, only an actual change to the deferred value should.A test-infrastructure wrinkle worth flagging explicitly
The LIKE-debounce tests used
jest.useFakeTimers()+jest.advanceTimersByTime()to force the old debounce to fire.useDeferredValuehas no literal timer to advance, it settles via React's own scheduler, independent of fake timers, and settles much faster in a flushed test environment than a real debounce window would. Two tests ("pending LIKE debounce still applies after rerender recreates updateDataMask", "...is canceled when operatorType switches back to Exact") needed restructuring: the keystroke and the rerender now batch into oneact()so the deferred value hasn't settled before the identity change, which is what actually exercises the stale-ref path. Without that batching, the old test shape raced against a window that no longer exists, sinceuseDeferredValuesettles immediately once nothing else is competing for render priority.keeps "Select all" pinned to the full column while searching (creatable false)fails identically on an unmodifiedmastercheckout (confirmed in an isolated worktree before touching anything), it's a pre-existing environment flake unrelated to this change and is left as-is.TESTING INSTRUCTIONS
npm test -- --testPathPatterns='SelectFilterPlugin'— 47/48 passing locally (1 pre-existing, unrelated flake, see above).ADDITIONAL INFORMATION
🤖 Generated with Claude Code