fix:updated searchbar component to return result with client side sea… #36360
Conversation
…rch is enabled only
WalkthroughThe changes in this pull request introduce a new optional property, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Hii @shadabbuchh @carinanfonseca , I have raised the PR, could you please assign the reviewer to this PR. thank you. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hii @shadabbuchh @carinanfonseca @NilanshBansal ,could you please assign the reviewer to this PR. thank you. |
|
Adding @rahulbarwal @jacquesikot from the widgets team to check this out. |
rahulbarwal
added
the
Widgets & Accelerators Pod
There was a problem hiding this comment.
Overall I agree with code changes.
The naming of test cases should be improved. Right now, all of them are very technical and tightly bound to what is happening in the code.
Recommendation: Rename them to explain the user behavior and expectation, not the table widget behavior.
|
Hii @rahulbarwal , I have updated the test case names, could you check the changes. thank you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (4)
1-14: Well done, class! Your imports and mocks are on point.I'm pleased to see you've imported the necessary testing utilities and mocked the debounce function and SVG import. This shows good attention to detail and understanding of testing best practices.
However, let's make one small improvement to enhance our code's readability:
Consider adding a space before the closing curly brace on line 4:
-import "@testing-library/jest-dom" +import "@testing-library/jest-dom"This small change will maintain consistency with the spacing in other import statements.
30-58: A+ work on these test cases, class! You're really grasping the concepts.Your first two tests effectively cover crucial user interactions: typing in the search box and clearing the input. I'm pleased to see you're using
act()to properly handle state updates in your tests.To make your tests even more robust, consider this small enhancement:
In the second test, add an assertion to check if the input is focused after clearing:
expect(inputElement).toHaveFocus();This will ensure that the user can immediately start typing after clearing the input, enhancing the user experience.
60-71: Excellent work on this test case! You're thinking outside the box.Your third test demonstrates a good understanding of component lifecycle and prop updates. Using
rerenderto simulate changing props is a smart approach.To make this test even more comprehensive, consider this addition:
Add an assertion to check if the
onSearchcallback is called with the new value after rerendering:expect(onSearchMock).toHaveBeenCalledWith("updated");This will ensure that the component not only updates its internal state but also notifies its parent of the change.
73-87: Bravo on your final test case! You're really covering all the bases.Your fourth test does an excellent job of verifying the component's behavior when client-side search is disabled. I'm impressed that you're checking both the UI state and the callback behavior.
To make this test even more robust, consider this enhancement:
Add a check to ensure that the search input is disabled when client-side search is turned off:
expect(inputElement).toBeDisabled();This will verify that users can't interact with the search input when client-side search is disabled, preventing any confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (1 hunks)
🔇 Additional comments (1)
app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (1)
16-28: Excellent test setup, students! You're setting a great example.Your test suite structure is commendable. The use of
describeto group related tests and the creation of a mock function foronSearchdemonstrate a solid understanding of test organization.I'm particularly impressed with your
renderComponenthelper function. This approach will make your tests more maintainable and easier to read. Keep up the good work!
|
@Naveen-Goud CI is failing for your changes: https://github.com/appsmithorg/appsmith/actions/runs/11230421118/job/31217757085?pr=36745 Please check and fix. |
|
@Naveen-Goud can you please fix the failing CI issues. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (4)
16-29: Excellent job on structuring your test suite!I'm impressed with your use of the 'describe' block and the creation of a renderComponent function. This setup will make your tests more readable and maintainable. However, I have a small suggestion to make your tests even better:
Consider adding a type for the props in the renderComponent function. This will improve type safety and make it clearer what props are available.
Here's how you could modify the function:
interface SearchComponentProps { onSearch: (value: string) => void; placeholder: string; value: string; enableClientSideSearch?: boolean; } const renderComponent = (props: Partial<SearchComponentProps> = {}) => { // ... rest of the function };This change will help catch any prop-related errors early and improve the overall quality of your tests.
30-54: Great job on these test cases, students!Your first two tests cover crucial functionality of the SearchComponent. You've done well to check both the input value and the onSearch callback. However, to make your tests even more robust, I'd like you to add one more assertion to the first test:
In the first test, after firing the change event, add an assertion to check if the onSearchMock was called only once. This will ensure that the debounce function is working as expected. Here's how you can do it:
expect(onSearchMock).toHaveBeenCalledTimes(1);Remember, in testing, it's not just about checking the correct behavior, but also ensuring that unexpected behaviors don't occur. Keep up the good work!
56-73: Excellent work on testing component updates!I'm pleased to see you've included a test for updating the search input based on external changes. This is a crucial aspect of component behavior that often gets overlooked. Your use of the rerender function is spot on!
To make this test even more thorough, consider adding one more step:
After rerendering with the updated value, try typing in the input field again and verify that the onSearch callback is called with the new value. This will ensure that the component remains fully functional after receiving external updates. Here's an example:
fireEvent.change(inputElement, { target: { value: "new search" } }); expect(onSearchMock).toHaveBeenCalledWith("new search");Remember, thorough testing leads to robust components. Keep up the excellent work!
75-96: Well done on covering this important scenario!Your final test case does an excellent job of verifying the component's behavior when client-side search is disabled. It's crucial to test these edge cases to ensure your component behaves correctly in all situations.
To make your test suite even more comprehensive, I'd like you to add one more test case:
Consider adding a test that verifies the component's behavior when client-side search is initially disabled and then enabled. This will ensure that the component responds correctly to all possible state changes.
Here's a outline for this new test:
- Render the component with client-side search disabled
- Verify that the input is empty and disabled
- Rerender the component with client-side search enabled
- Verify that the input is now enabled and can be typed into
Would you like me to provide a full implementation of this test case or open a GitHub issue to track this task?
app/client/packages/design-system/widgets-old/src/SearchComponent/index.tsx (4)
17-17: Well done on adding the new property, class!The addition of the
enableClientSideSearchoptional property is a good step towards improving the flexibility of our SearchComponent. It's like adding a new tool to our classroom toolkit!However, let's not forget the importance of clear communication. Could you please add a brief comment explaining the purpose of this property? It would be like writing a helpful note on our classroom board for everyone to understand.
103-110: Excellent addition to our component's behavior, students!Your implementation in
componentDidUpdateshows great attention to detail. It's like you've added a new rule to our classroom that everyone follows when we change how we're searching.One small suggestion to make our code even better: Consider extracting the search reset logic into a separate method. This would be like creating a special helper in our classroom that we can call whenever we need to reset our search. It might look something like this:
resetSearch = () => { this.setState({ localValue: "" }, () => { this.props.onSearch(""); }); };Then in
componentDidUpdate, you could simply callthis.resetSearch(). This makes our code more modular and easier to understand, just like how we organize our classroom into different learning stations!
120-122: Good job implementing the conditional search, class!Your addition to the
handleSearchmethod is like adding a checkpoint in our learning process. We only proceed with the search if we're allowed to, which is very responsible!To make our code even clearer, let's consider adding an
elseblock. This would be like explaining what happens when we don't meet the condition in our classroom. Here's a suggestion:if (this.props.enableClientSideSearch) { this.onDebouncedSearch(search); } else { // If client-side search is disabled, we might want to log this or handle it in some way console.log('Client-side search is disabled'); }This way, we're explicitly stating what happens in both cases, making our code more self-explanatory, just like how we always clarify instructions in class!
126-128: Wonderful job on updating the clearSearch method, students!Your modification to
clearSearchis like adding an eraser to our search toolbox, but only using it when we're allowed to. That's very thoughtful!To keep our code consistent with the
handleSearchmethod, let's consider adding anelseblock here too. It would be like having the same rules for both writing and erasing on our classroom board. Here's a suggestion:if (this.props.enableClientSideSearch) { this.onDebouncedSearch(""); } else { // If client-side search is disabled, we might want to log this or handle it in some way console.log('Client-side search clear is disabled'); }This way, we're treating both our search and clear operations the same way, which makes our code more uniform and easier to understand. Remember, consistency is key in both coding and classroom management!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (1 hunks)
- app/client/packages/design-system/widgets-old/src/SearchComponent/index.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/client/packages/design-system/widgets-old/src/SearchComponent/index.test.tsx (1)
1-14: Well done on setting up the necessary imports and mocks!Class, let's take a moment to appreciate the importance of proper test setup. The import statements and mocks you've included are crucial for creating a controlled testing environment. By mocking the debounce function and SVG imports, you've ensured that your tests will run consistently and without external dependencies. This is excellent practice in unit testing!
|
Hii @rahulbarwal , I have resolved the lint issue , can you re-run the checks. thank you. |
|
@Naveen-Goud client lint is still failing on your changes: https://github.com/appsmithorg/appsmith/actions/runs/11321677640/job/31481100928?pr=36745#step:9:379 Please run |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@Naveen-Goud are you still working on this? |
Description:
modified the code the return the result when client side search is enable and not to when client side search is disabled.
fixes: #15386
updates in PR:
1.passed enableClientSide prop to the search component and applied conditional rendering to return result.
2.added test file for the changed code.
snapshorts:
Summary by CodeRabbit
New Features
enableClientSideSearchproperty across multiple components, allowing for enhanced client-side searching capabilities.SearchComponent,Table, andTableHeaderto support dynamic search functionalities based on user input.Bug Fixes
SearchComponentto reset search values appropriately.Tests
SearchComponentto validate its functionality and user interactions.