You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/10/02 01:40:20 UTC
[PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
rusackas opened a new pull request, #44880:
URL: https://github.com/apache/superset/pull/44880
### SUMMARY
Refs #39890. Continues the `useDeferredValue` adoption started in #39928 (`TableExploreTree`), #39970 (`DatasourcePanel`), and #44878 (`TextControl`), this time for the native `Select` dashboard filter plugin, which has two separate `lodash` `debounce()` sites.
### Two debounces, two shapes
- **`onSearch`** — client-side option filtering. This is a pure render-value case, the same shape as `TableExploreTree`/`DatasourcePanel`: the dropdown's options list now reads `deferredSearchInput` directly. There's also a conditional `searchAllOptions` dispatch (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 as `TextControl`'s (#44878) `pendingValue` pattern: the debounce only ever gated when the committed value reached `updateDataMask`, so it became a `pendingLikeValue` state + effect, same as `TextControl`.
Both effects read their mutable dependencies (`searchAllOptions`/`dispatchDataMask`/`initialColtypeMap`, and `updateDataMask`) 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. `useDeferredValue` has 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 one `act()` 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, since `useDeferredValue` settles 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 unmodified `master` checkout (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
1. Add a native Select filter to a dashboard, type into its search box rapidly. Input stays responsive, option list narrows shortly after.
2. With "Search all filter's options" enabled on the filter, confirm a server-side search still fires (and only once settled, not per keystroke).
3. Switch the filter's operator to "contains"/"starts with"/"ends with" and type into the free-text input; confirm the filter still applies shortly after typing stops.
4. "Clear All" on a dashboard with an active search/LIKE value should reset cleanly with no stale dispatch.
5. `npm test -- --testPathPatterns='SelectFilterPlugin'` — 47/48 passing locally (1 pre-existing, unrelated flake, see above).
### ADDITIONAL INFORMATION
- [x] Has associated issue: #39890
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162826760
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
Yes, this is a valid stale-state issue. The guard prevents dispatch while the operator is `Exact`, but `pendingLikeValue` remains populated. When switching back to a LIKE operator, the deferred value can settle again and reapply the previous text without a new edit.
Clear the pending value whenever LIKE mode is exited:
```tsx
useEffect(() => {
if (!isLikeOperator) {
setPendingLikeValue(null);
}
}, [isLikeOperator]);
useEffect(() => {
if (
deferredPendingLikeValue === null ||
!isLikeOperator ||
clearAllTrigger
) {
return;
}
if (deferredPendingLikeValue) {
updateDataMaskRef.current([deferredPendingLikeValue]);
} else {
updateDataMaskRef.current(null);
}
}, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
```
This preserves the current cancellation behavior while ensuring that returning to `Contains`, `StartsWith`, or `EndsWith` does not restore an edit that was made under the previous LIKE mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5954250266
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5944012431
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 8505f10459918d20f62f1a60b12f5ad83eedb9d6 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abf0b8511f4910008553611 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44880--superset-docs-preview.netlify.app](https://deploy-preview-44880--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcvBCYQwEAXQXuZsMING3O3AMhLzB4WoYYzCItu7ub3LeyjrseVCX6KGgvp9XqozVNoTCXMxsqYCNdcJEyFQRTS3TxdqiMjp-E2xlsEHsWF0zNJ_2Fo7OtcNzPR_AQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44880--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4178589518
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,11 +221,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
+ [],
+ );
+ useEffect(() => () => settleLikeValue.cancel(), [settleLikeValue]);
useEffect(() => {
const externalValue =
filterState.value?.[0] != null ? String(filterState.value[0]) : '';
setLikeInputValue(externalValue);
+ // A parent-driven value change supersedes any settled edit, so re-entering
+ // the same text afterwards registers as a fresh edit.
+ setPendingLikeValue(null);
Review Comment:
The `Bob` default forces a different internal mask, so this passes without covering the cleared-default path where re-entering `Jen` can leave `defaultDataMask` unset. Could the regression reset to `undefined` without a replacement default, then assert that re-entering `Jen` publishes a new mask?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162201721
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Debounce Removed Search Dispatch</b></div>
<div id="fix">
The removed `onSearch` debounced the `searchAllOptions` dispatch with `Constants.SLOW_DEBOUNCE`. `useDeferredValue` only lowers render priority — it does not coalesce by time, so every keystroke (any pause ≥ a frame) commits and this effect dispatches `ownState`, cascading via the `setDataMask` effect (line 554) to the parent per keystroke. Please re-add time-based debouncing around this dispatch.
</div>
</div>
<small><i>Code Review Run #59c82d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Debounce Removed LIKE Dispatch</b></div>
<div id="fix">
The removed `debouncedLikeChange` (`Constants.SLOW_DEBOUNCE`, cancelled on clear/unmount) gated `updateDataMaskRef.current(...)`. `useDeferredValue` provides no time-based coalescing, so each keystroke in the LIKE input commits the filter value through `updateDataMask` and the `setDataMask` effect (line 554) to the filter bar. Typing now causes per-keystroke parent state updates; please re-add a debounce.
</div>
</div>
<small><i>Code Review Run #59c82d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162962816
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +629,32 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
+ // Leaving a LIKE operator discards the pending edit so returning to one
+ // doesn't replay text typed under the previous mode.
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (!isLikeOperator) {
+ settleLikeValue.cancel();
+ setPendingLikeValue(null);
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
+ }, [isLikeOperator, settleLikeValue]);
Review Comment:
Changing a configured Contains default to Starts with clears the form's default, but both modes leave isLikeOperator true and retain the old pending text. Pasting the same value into the cleared input then changes none of this effect's dependencies, so the input shows a value while the default remains unset; could every match-type change invalidate the pending edit?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={{}} />);
Review Comment:
The empty object is still truthy, so this rerender keeps both the reset effect and the LIKE dispatch guard active rather than exercising Clear All completion. Could the test release the trigger with a falsy value and assert that a previously typed predicate is not restored after acknowledgement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4170409239
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
Review Comment:
<!-- Bito Reply -->
The reviewer is correct that passing `undefined` to `clearAllTrigger` does not explicitly release the trigger if the underlying prop is optional and omitted by the spread operator. To ensure the trigger is explicitly released and the test correctly exercises the release behavior, you should pass `{ 'test-filter': false }` instead of `undefined`.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx**
```
rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={{ 'test-filter': false }} />);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5983327805
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44880:
URL: https://github.com/apache/superset/pull/44880
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4172255988
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -525,8 +585,13 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
});
updateDataMask(null);
- setSearch('');
+ settleSearch.cancel();
Review Comment:
With an active selection and “Search all filter’s options” enabled, searching `ab` then clicking Clear All cancels the blur-triggered empty search, while the reset guard suppresses its replacement. `ownState.search` stays `ab`, so reopening the cleared filter still shows only that server-side subset; could Clear All also publish the empty server search?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,6 +221,15 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
Review Comment:
After a Contains default of `Jen` settles, toggling “Can select multiple values” clears the default and input but leaves `pendingLikeValue` as `Jen`; re-entering `Jen` then produces no state change, so the displayed value never becomes a default and validation still fails. Could parent-driven resets also invalidate the settled edit, rather than relying only on operator changes and Clear All?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
+ await act(async () => {});
Review Comment:
The restored LIKE debounce waits 500 ms, but this assertion only flushes React updates; if clear-all stopped canceling the timer, `%Jen%` could be restored after the test has already passed. Could this and the operator-switch checks advance through the debounce before asserting no stale mask is published?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4178343242
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +645,31 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
+ // Any match-type change (including between two LIKE modes) discards the
+ // pending edit, so text typed under the previous mode is never replayed and
+ // re-entering the same text afterwards registers as a fresh edit.
+ useEffect(() => {
+ settleLikeValue.cancel();
+ setPendingLikeValue(null);
Review Comment:
Good catch, @sadpandajoe. A match-type change now also resets the input to the committed value, so the discarded `Jen` no longer lingers. Added a test for the Contains to Starts with switch.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,11 +221,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
+ [],
+ );
+ useEffect(() => () => settleLikeValue.cancel(), [settleLikeValue]);
useEffect(() => {
const externalValue =
filterState.value?.[0] != null ? String(filterState.value[0]) : '';
setLikeInputValue(externalValue);
+ // A parent-driven value change supersedes any settled edit, so re-entering
+ // the same text afterwards registers as a fresh edit.
+ setPendingLikeValue(null);
Review Comment:
Added a regression that settles `Jen`, has the parent reset the value to a different default, then re-enters `Jen` and asserts a new Contains mask after the debounce. It goes red with the invalidation removed.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -525,8 +588,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
});
updateDataMask(null);
- setSearch('');
+ settleSearch.cancel();
+ settleLikeValue.cancel();
+ // A search the user already ran left a server-side subset in ownState;
+ // publish the empty search so the cleared filter lists every option.
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (hasSearchedRef.current && currentSearchAllOptions) {
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: { coltypeMap: currentInitialColtypeMap, search: '' },
+ });
Review Comment:
Added a test that settles an `ab` server search with `searchAllOptions`, fires Clear All, and asserts the final mask has `ownState.search === ''`. It fails if the empty-search publication is removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5943994351
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162927243
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
Good catch. useDeferredValue only lowers render priority, so it was dispatching on every keystroke. I put a SLOW_DEBOUNCE quiet period back in front of the search dispatch (the deferred value still drives the options rendering), and clear-all cancels it. Pushed ce7f263.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
Same fix here. The LIKE input now goes through a SLOW_DEBOUNCE again before it hits updateDataMask, and it is canceled on clear-all, operator switch, and unmount. Pushed ce7f263. I could not run the jest suite locally though, so leaning on CI to confirm.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
Agreed, and that is what I did in ce7f263: the search dispatch and the LIKE update each have their own SLOW_DEBOUNCE quiet period now, independent of the deferred rendering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4174345085
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -525,8 +588,26 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
});
updateDataMask(null);
- setSearch('');
+ settleSearch.cancel();
+ settleLikeValue.cancel();
+ // A search the user already ran left a server-side subset in ownState;
+ // publish the empty search so the cleared filter lists every option.
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (hasSearchedRef.current && currentSearchAllOptions) {
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: { coltypeMap: currentInitialColtypeMap, search: '' },
+ });
Review Comment:
The Clear All tests use LIKE input with server search disabled, so they would still pass if this empty-search publication were removed and the cleared dropdown retained its old server-side subset. Could a regression settle an `ab` search with an active selection, clear and acknowledge it, then exhaust timers and assert the final mask has `ownState.search === ''`?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +645,31 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
+ // Any match-type change (including between two LIKE modes) discards the
+ // pending edit, so text typed under the previous mode is never replayed and
+ // re-entering the same text afterwards registers as a fresh edit.
+ useEffect(() => {
+ settleLikeValue.cancel();
+ setPendingLikeValue(null);
Review Comment:
With an unset default, typing `Jen` under Contains and switching to Starts with before the 500 ms debounce expires leaves `Jen` displayed but no default committed, so required-value validation still fails. `filterState.value` stays `undefined`, so its sync effect does not clear the input; could discarding the pending edit also clear the displayed text?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,11 +221,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
+ [],
+ );
+ useEffect(() => () => settleLikeValue.cancel(), [settleLikeValue]);
useEffect(() => {
const externalValue =
filterState.value?.[0] != null ? String(filterState.value[0]) : '';
setLikeInputValue(externalValue);
+ // A parent-driven value change supersedes any settled edit, so re-entering
+ // the same text afterwards registers as a fresh edit.
+ setPendingLikeValue(null);
Review Comment:
The rerender test only changes a label while the value stays `undefined`, so removing this invalidation would still pass while identical LIKE re-entry after a parent reset breaks again. Could a regression settle `Jen`, clear the default through the multi-select toggle, then re-enter `Jen` and assert a new committed Contains mask after the debounce?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4261190356
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,11 +221,23 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
+ [],
+ );
+ useEffect(() => () => settleLikeValue.cancel(), [settleLikeValue]);
useEffect(() => {
const externalValue =
filterState.value?.[0] != null ? String(filterState.value[0]) : '';
setLikeInputValue(externalValue);
+ // A parent-driven value change supersedes any settled edit, so re-entering
+ // the same text afterwards registers as a fresh edit.
+ setPendingLikeValue(null);
Review Comment:
@sadpandajoe good point, the `Bob` default masked it. The regression now resets to `undefined` with no replacement default, then re-enters `Jen` and asserts a new Contains mask. Pushed 303c3d7. I could not run jest locally, so leaning on CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162824938
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
Review Comment:
After typing a Contains value and clicking Clear All, onClearAllComplete removes the trigger while deferredPendingLikeValue can still hold the old text; this effect can then restore the cleared predicate, and the later null deferred value does not undo it. Could the reset invalidate the pending edit synchronously, with a regression that types first and releases the Clear All trigger?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
Agreed—changing Contains to Exact clears the configured default, but returning to Contains reruns this effect with the retained pendingLikeValue and restores the old default without another edit. Could leaving LIKE mode discard that pending edit rather than only skip dispatch?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
Agreed—the remaining Select debounce is 250 ms, so typing 300 ms apart now starts partial server searches that the removed 500 ms gate coalesced; the final term then waits behind an obsolete in-flight request. Could query-producing search and LIKE updates retain a quiet-period debounce independently of deferred rendering?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162826134
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
<!-- Bito Reply -->
Yes, you can retain a quiet-period debounce independently of the deferred rendering. By introducing a separate `debounce` function for the `handleSearch` logic, you can coalesce the `setSearchInput` updates (and consequently the `ownState` dispatch) while still allowing `useDeferredValue` to handle the UI rendering priority independently. This ensures that the server is only queried after the user has paused typing for the specified duration, preventing obsolete in-flight requests.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx**
```
const debouncedSearch = useMemo(
() => debounce((value: string) => {
hasSearchedRef.current = true;
setSearchInput(value);
}, Constants.SLOW_DEBOUNCE),
[]
);
const handleSearch = useCallback((value: string) => {
debouncedSearch(value);
}, [debouncedSearch]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162837761
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
@sadpandajoe good catch. Leaving LIKE mode now clears `pendingLikeValue`, and the dispatch effect ignores any deferred value that lags the live one, so returning to Contains no longer replays the old text. Added a regression test.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
Review Comment:
@sadpandajoe agreed. The dispatch effect now skips a deferred value that does not match the live `pendingLikeValue`, so a stale value cannot restore the predicate once the Clear All trigger is released. Added a regression test that types, clears, then releases the trigger.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5944102861
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44880?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.42857%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.92%. Comparing base ([`3f6d4cd`](https://app.codecov.io/gh/apache/superset/commit/3f6d4cd689648eb5ed320d31dc9feb27e147e644?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8505f10`](https://app.codecov.io/gh/apache/superset/commit/8505f10459918d20f62f1a60b12f5ad83eedb9d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44880?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://app.codecov.io/gh/apache/superset/pull/44880?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Ffilters%2Fcomponents%2FSelect%2FSelectFilterPlugin.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | 91.42% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44880 +/- ##
=======================================
Coverage 81.92% 81.92%
=======================================
Files 2986 2986
Lines 183289 183303 +14
Branches 42385 42389 +4
=======================================
+ Hits 150156 150171 +15
+ Misses 30393 30392 -1
Partials 2740 2740
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44880/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44880/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.52% <91.42%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44880?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4163340685
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +629,32 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
+ // Leaving a LIKE operator discards the pending edit so returning to one
+ // doesn't replay text typed under the previous mode.
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (!isLikeOperator) {
+ settleLikeValue.cancel();
+ setPendingLikeValue(null);
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
+ }, [isLikeOperator, settleLikeValue]);
Review Comment:
Good catch. The pending LIKE edit is now discarded on any operator change, not just when leaving a LIKE mode, so pasting the same text after switching Contains to Starts with registers as a fresh edit and sets the default. Pushed 45af057. I could not run jest locally, so leaning on CI.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={{}} />);
Review Comment:
Right, `{}` is truthy so that rerender never actually released the trigger. It now passes `undefined`, so the test exercises Clear All completing and still asserts the typed predicate is not restored. Pushed 45af057.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5947306785
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162928015
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
<!-- Bito Reply -->
The implementation of a quiet-period debounce for the search dispatch is correct. By using a `useMemo` to create a debounced function that updates the state, you successfully coalesce the `ownState` dispatches while allowing `useDeferredValue` to maintain responsive UI rendering. This approach effectively prevents the issue of excessive server requests during rapid typing.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx**
```
const debouncedSearch = useMemo(
() => debounce((value: string) => {
hasSearchedRef.current = true;
setSearchInput(value);
}, Constants.SLOW_DEBOUNCE),
[]
);
const handleSearch = useCallback((value: string) => {
debouncedSearch(value);
}, [debouncedSearch]);
```
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
<!-- Bito Reply -->
The implementation appears to correctly address the reviewer's concern by re-introducing a debounce mechanism for the LIKE operator input. By using `SLOW_DEBOUNCE` and ensuring the debounced function is properly canceled on clear-all, operator switches, and component unmounts, the fix effectively prevents excessive state updates on every keystroke.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162153143
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
**Suggestion:** Switching away from a LIKE operator leaves the pending value intact, so switching back later reapplies the old text without a new edit.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Stale reference`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8005ba079c9b4731b678540a0ab5d61c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8005ba079c9b4731b678540a0ab5d61c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
**Line:** 611:624
**Comment:**
*Stale Reference: Switching away from a LIKE operator leaves the pending value intact, so switching back later reapplies the old text without a new edit.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44880&comment_hash=6509bd1f9749466dbd2d40abc76af391a3b40f9a943fcb51bf8c0af20e5fd767&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44880&comment_hash=6509bd1f9749466dbd2d40abc76af391a3b40f9a943fcb51bf8c0af20e5fd767&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4170408252
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
Review Comment:
The rerender replaces the whole props object, so `clearAllTrigger={undefined}` does release the earlier `{ test-filter: true }`. The truthy checks in the effects see `undefined` and the guard lifts, so the test exercises the release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5944044058
<!-- Bito Reply -->
The flagged issue is correct. The current implementation uses `useDeferredValue` to handle the `pendingLikeValue`, but when the operator type switches away from `LIKE`, the `deferredPendingLikeValue` may still hold the previous search text until it settles. Because the `useEffect` hook that triggers `updateDataMask` depends on `deferredPendingLikeValue`, it can re-trigger a data update with the old value even after the operator has changed.
To resolve this, you should ensure the `useEffect` hook explicitly ignores updates when the operator is not `LIKE` or when the value is being reset. The current implementation already includes a check `!isLikeOperator` in the `useEffect` dependency, but it should be robust enough to handle the transition state.
If you are seeing this issue, ensure that the `useEffect` correctly cancels or ignores the effect when `isLikeOperator` changes. The current code already has `isLikeOperator` in the dependency array, which should trigger the effect to re-run when the operator changes. If the issue persists, it may be due to the `deferredPendingLikeValue` settling after the operator change. You can add a check to ensure the effect only runs if the operator is still `LIKE` at the time of execution.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx**
```
useEffect(() => {
if (
deferredPendingLikeValue === null ||
!isLikeOperator ||
clearAllTrigger
) {
return;
}
if (deferredPendingLikeValue) {
updateDataMaskRef.current([deferredPendingLikeValue]);
} else {
updateDataMaskRef.current(null);
}
}, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4166630566
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>No-op trigger release</b></div>
<div id="fix">
`clearAllTrigger={undefined}` never releases the trigger: `likeProps` from `buildSelectFilterProps` has no `clearAllTrigger` key (types.ts:76 types it optional), so spread order keeps the prop unset and the clear-all effect never re-runs with the trigger released. The 'release the trigger' step is a no-op, so the test cannot catch a regression that restores the pending edit after release. Pass `{ 'test-filter': false }` instead.
</div>
</div>
<small><i>Code Review Run #f31e81</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44880:
URL: https://github.com/apache/superset/pull/44880#issuecomment-5973005101
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(filters): use useDeferredValue in SelectFilterPlugin [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4174042103
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -2052,30 +2051,152 @@ test('pending LIKE debounce is canceled when operatorType switches back to Exact
reduxState,
);
- fireEvent.change(
- screen.getByPlaceholderText('Type to search (contains)...'),
- {
- target: { value: 'Jen' },
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ const callsBeforeSwitch = setDataMaskMock.mock.calls.length;
+
+ // Same batching as the sibling test above: the keystroke and the switch
+ // away from the LIKE operator land in the same update, before the
+ // deferred value has a chance to settle on its own.
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Exact },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ })}
+ />,
+ );
+ });
+
+ await act(async () => {});
+
+ // Rerendering with fresh (non-memoized) props can independently re-run
+ // this plugin's own default-value initialisation, unrelated to the LIKE
+ // debounce this test is pinning -- so assert no *new* call carries the
+ // 'Jen' ILIKE payload, rather than that setDataMask was never called again.
+ const callsSinceSwitch = setDataMaskMock.mock.calls
+ .slice(callsBeforeSwitch)
+ .map(call => call[0]);
+ expect(callsSinceSwitch).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('switching Exact then back to LIKE does not replay the discarded pending edit', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const buildProps = (operatorType: SelectFilterOperatorType) =>
+ buildSelectFilterProps({
+ formData: { operatorType },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
},
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
- setDataMaskMock.mockClear();
+ act(() => {
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Exact)} />,
+ );
+ });
+ await act(async () => {});
+ const callsBeforeReturn = setDataMaskMock.mock.calls.length;
rerender(
- <SelectFilterPlugin
- {...buildSelectFilterProps({
- formData: { operatorType: SelectFilterOperatorType.Exact },
- filterState: { value: undefined },
- setDataMask: setDataMaskMock,
- })}
- />,
+ <SelectFilterPlugin {...buildProps(SelectFilterOperatorType.Contains)} />,
+ );
+ await act(async () => {});
+
+ expect(
+ setDataMaskMock.mock.calls.slice(callsBeforeReturn).map(call => call[0]),
+ ).not.toContainEqual(
+ expect.objectContaining({
+ extraFormData: expect.objectContaining({
+ filters: [expect.objectContaining({ op: 'ILIKE', val: '%Jen%' })],
+ }),
+ }),
+ );
+});
+
+test('clear-all does not restore a LIKE edit still pending in the deferred value', async () => {
+ jest.useFakeTimers({ advanceTimers: true });
+ const setDataMaskMock = jest.fn();
+ const likeProps = buildSelectFilterProps({
+ formData: { operatorType: SelectFilterOperatorType.Contains },
+ filterState: { value: undefined },
+ setDataMask: setDataMaskMock,
+ });
+ const reduxState = {
+ useRedux: true,
+ initialState: {
+ nativeFilters: {
+ filters: { 'test-filter': { name: 'Test Filter' } },
+ },
+ dataMask: {
+ 'test-filter': {
+ extraFormData: {},
+ filterState: { value: undefined },
+ },
+ },
+ },
+ };
+
+ const { rerender } = render(
+ <SelectFilterPlugin {...likeProps} />,
+ reduxState,
);
+ const input = screen.getByPlaceholderText('Type to search (contains)...');
+ // Type and fire clear-all in one update, then release the trigger, so the
+ // deferred value still holds the typed text when the trigger goes away.
act(() => {
- jest.advanceTimersByTime(500);
+ fireEvent.change(input, { target: { value: 'Jen' } });
+ rerender(
+ <SelectFilterPlugin
+ {...likeProps}
+ clearAllTrigger={{ 'test-filter': true }}
+ />,
+ );
});
+ const callsBeforeRelease = setDataMaskMock.mock.calls.length;
+ rerender(<SelectFilterPlugin {...likeProps} clearAllTrigger={undefined} />);
+ await act(async () => {});
Review Comment:
@sadpandajoe agreed. The clear-all and operator-switch tests now advance through the 500 ms debounce before asserting no stale mask is published. Pushed c2e2874.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -200,6 +221,15 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
const [likeInputValue, setLikeInputValue] = useState<string>(
filterState.value?.[0] != null ? String(filterState.value[0]) : '',
);
+ // null until the user types in the LIKE input; distinguishes a real edit
+ // from the clearAllTrigger reset below, which also clears likeInputValue
+ // but must not re-trigger a dispatch once the deferred value catches up.
+ const [pendingLikeValue, setPendingLikeValue] = useState<string | null>(null);
+ const settleLikeValue = useMemo(
+ () => debounce(setPendingLikeValue, Constants.SLOW_DEBOUNCE),
Review Comment:
@sadpandajoe good catch. Any parent-driven change to `filterState.value` now also discards the settled pending edit, so re-entering `Jen` after the multi-select toggle registers as a fresh edit. Pushed c2e2874; jest does not run locally here, so CI will confirm.
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -525,8 +585,13 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
});
updateDataMask(null);
- setSearch('');
+ settleSearch.cancel();
Review Comment:
@sadpandajoe right. Clear All now publishes the empty server search when a search had run with "Search all filter's options" on, so the cleared filter lists every option again. Pushed c2e2874.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org