Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: [M3-8186] - Clean up DebouncedSearchTextField and fix instances where debouncing is not happening #10813

Merged
merged 31 commits into from
Sep 6, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Aug 22, 2024

Description 📝

This PR fixes the issue of debouncing is not happening for the instances of DebouncedSearchTextField component.

Changes 🔄

List any change relevant to the reviewer.

  • Fix debounce issue in PlacementGroupsLanding.
  • Fix debounce issue in LinodeSelectTable.
  • Removed customeValue prop usage in LinodeSelectTable and SelectLinodePanel.
  • Memoized the debounced onChange handler to prevent unnecessary re-creations.
  • Improved the useEffect hook to handle empty values and reduce unnecessary state updates.
  • removed redundant logic of the clear icon from the instanced of DebouncedSearchTextField.

Target release date 🗓️

09/02

How to test 🧪

Verification steps

(How to verify changes)

  • Checkout the branch and verify there is no regression for all the references of PlacementGroupsLanding, SelectLinodePanel and LinodeSelectTable
  • Verify debounce behavior for PlacementGroupsLanding and LinodeSelectTable
  • Verify there is no regression in all the references of DebouncedSearchTextField.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@cpathipa cpathipa requested a review from a team as a code owner August 22, 2024 16:05
@cpathipa cpathipa requested review from jdamore-linode and hkhalil-akamai and removed request for a team August 22, 2024 16:05
@cpathipa cpathipa marked this pull request as draft August 22, 2024 16:06
@cpathipa cpathipa self-assigned this Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

Coverage Report:
Base Coverage: 86.15%
Current Coverage: 86.15%

@cpathipa cpathipa changed the title M3 8186 refactor: [M3-8186] - Clean up DebouncedSearchTextField and fix instances where debouncing is not happening Aug 22, 2024
@cpathipa cpathipa requested a review from mjac0bs August 23, 2024 14:15
@cpathipa cpathipa marked this pull request as ready for review August 23, 2024 14:15
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for making this fix. I originally created the customValue prop but it may not have been necessary.

Left some suggestions.

Failing unit tests can likely be resolved using await waitFor(() => { ... });

}
setQuery(value ?? '');
},
value: preselectedLinodeId ? field.value?.label ?? '' : query,
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticing a regression here:

Steps to reproduce:

  1. Navigate to Linodes landing page
  2. Open the action menu for a linode and click "Clone Linode"
  3. The Linodes list in the create flow will be filtered using the source Linode's label but the search text field will display empty/placeholder.

Expected behavior:
The search text field should be pre-filled with the label of the source Linode (see prod).

@@ -152,7 +150,9 @@ export const SelectLinodePanel = (props: Props) => {
expand={true}
hideLabel
label=""
onSearch={(value) => setUserSearchText(value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onSearch={(value) => setUserSearchText(value)}
onSearch={setUserSearchText}

Comment on lines 93 to 97
onSearch={(value) => {
setSearchText(value);
if (searchText !== value) {
setSearchText(value);
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This optimization is not necessary as React will perform it for us.

Suggested change
onSearch={(value) => {
setSearchText(value);
if (searchText !== value) {
setSearchText(value);
}
}}
onSearch={setSearchText}

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for taking this one on, Chandra. Besides the thoughts Hussain raised, I'm wondering how we feel about continuing to use onSearch as the event handler prop here in general. onChange seems more standard to me, and I'm concerned that if we proceed with onSearch, we'll end up with another mistake like we saw in PlacementGroupsLanding.tsx somewhere down the line.

Besides needing to update a few more files, is there any other impact that replacing theonSearch prop with onChange would have?

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for addressing feedback! This looks good, approved pending a decision regarding @mjac0bs' comment about onSearch vs onChange.

@cpathipa
Copy link
Contributor Author

@hkhalil-akamai @mjac0bs Great feedback.
@mjac0bs Good call, Actually, the root cause of the issue is having onSearch as a dependency in the useEffect, which is causing unwanted re-triggers. Removing it from the dependencies would prevent this from happening in the future. Also, I don't see the use case for having it in the dependency list. The only concern I see with renaming onSearch to onChange is that we might override the onChange prop of TextFieldPropsOverrides. In my opinion, onSearch is a wrapper for onChange in DebouncedSearchTextField. Let me know your thoughts—I'm open to changing the prop name to onChange as well.


import { IconButton } from '../IconButton';

import type { TextFieldProps } from 'src/components/TextField';

export interface DebouncedSearchProps extends TextFieldProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I see, onSearch became an optional prop when customValue was added, and we don't want that - onSearch should be required. This will make it clear what the correct handler to use is. Can we make that change in this interface?

@@ -75,7 +65,7 @@ export const DebouncedSearchTextField = React.memo(
return () => clearTimeout(timeout);
}
return undefined;
}, [debounceTime, onSearch, textFieldValue]);
}, [debounceTime, textFieldValue]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is old code, but I don't understand why we went with a useEffect and setTimeout when we generally opt to use the debounce util throughout the codebase. And with onSearch required, we won't need to check for it. Could we clean this up a little and achieve the same by doing?

<Textfield
    [...other props]
    onChange={debounce(debounceTime ?? 400, (e) => {
      onSearch(e.target.value);
      setTextFieldValue(e.target.value);
    })}
/>

@cpathipa
Copy link
Contributor Author

cpathipa commented Sep 3, 2024

@mjac0bs Addressed the above feedback in the commit 9bb33ae
Changes include

  • Memoized the debounced onChange handler to prevent unnecessary re-creations.
  • Improved the useEffect hook to handle empty values and reduce unnecessary state updates.
  • removed redundant logic of the clear icon from the instanced of DebouncedSearchTextField.

The onSearch and value props are now required to ensure that the component can function correctly. The onSearch prop is essential for handling search. The value prop is necessary for managing the controlled input state. Making these props required enforces proper usage and helps prevent unintended behavior if they are accidentally omitted.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

The only thing I found on this last pass when checking through the places that the component is used throughout CM was a console error in Longview, which seems like it's happening because inputText can be undefined here.

Screenshot 2024-09-03 at 9 15 40 AM

I just restarted the e2e tests because they didn't kick off correctly - but approving pending the above is addressed and those look good.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Sep 3, 2024
@cpathipa
Copy link
Contributor Author

cpathipa commented Sep 3, 2024

@mjac0bs fixed the console error in Longview a1b2e8f

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 5, 2024
@cpathipa cpathipa merged commit d5798d0 into linode:develop Sep 6, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants