perf: entity group items filter utility refactor#38108
perf: entity group items filter utility refactor#38108alex-golovanov merged 3 commits intoreleasefrom
Conversation
WalkthroughThis pull request introduces a new utility function, 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.test.ts (1)
15-31: Add test cases for edge scenariosThe current test suite covers basic scenarios well. Consider adding tests for:
- Case sensitivity handling
- Special characters in search terms
- Empty groups array input
test.each([ ["FILE1", [{ name: "Group 1", items: [{ title: "file1" }] }]], // case sensitivity ["file#1", []], // special characters ["file1", [], []], // empty groups array ])("%s -> %j", (searchTerm, output) => { expect(filterEntityGroupsBySearchTerm(searchTerm, [])).toEqual(output); });app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.ts (1)
15-19: Consider making FUSE_OPTIONS configurableThe current implementation hardcodes the Fuse.js options. Consider making these configurable for different use cases.
-const FUSE_OPTIONS: FuseOptions<BaseItem> = { +const DEFAULT_FUSE_OPTIONS: FuseOptions<BaseItem> = { shouldSort: true, threshold: 0.1, keys, }; +export const filterEntityGroupsBySearchTerm = <G extends BaseGroup, T extends BaseItem>( + searchTerm: string, + groups: Array<Group<G, T>>, + options: FuseOptions<BaseItem> = DEFAULT_FUSE_OPTIONS,app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (2)
65-69: Simplify conditional rendering logicThe current implementation uses multiple conditions that could be simplified.
-{filteredItemGroups.length > 0 ? ( - <GroupedList groups={filteredItemGroups} /> -) : null} -{filteredItemGroups.length === 0 && searchTerm !== "" ? ( - <EmptySearchResult - type={createMessage(EDITOR_PANE_TEXTS.search_objects.datasources)} - /> -) : null} +{filteredItemGroups.length > 0 ? ( + <GroupedList groups={filteredItemGroups} /> +) : searchTerm && ( + <EmptySearchResult + type={createMessage(EDITOR_PANE_TEXTS.search_objects.datasources)} + /> +)}
32-34: Consider trimming search term for better UXTrim the search term to handle whitespace consistently.
-const filteredItemGroups = filterEntityGroupsBySearchTerm( - searchTerm, - itemGroups, -); +const filteredItemGroups = filterEntityGroupsBySearchTerm( + searchTerm.trim(), + itemGroups, +);app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
Line range hint
85-113: Consider extracting common group rendering logicThe group rendering pattern is repeated across files. Consider extracting it into a shared component to improve maintainability.
Example structure:
interface EntityGroup { group: string; items: Array<any>; // Type can be made more specific } interface EntityGroupListProps { groups: EntityGroup[]; renderItem: (item: any) => React.ReactNode; } const EntityGroupList: React.FC<EntityGroupListProps> = ({ groups, renderItem }) => { // Common group rendering logic };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.test.ts(1 hunks)app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.ts(1 hunks)app/client/src/IDE/utils/index.ts(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx(5 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx(3 hunks)app/client/src/pages/Editor/IDE/EditorPane/fuzzySearchInFiles.test.ts(0 hunks)app/client/src/pages/Editor/IDE/EditorPane/utils.ts(0 hunks)
💤 Files with no reviewable changes (2)
- app/client/src/pages/Editor/IDE/EditorPane/utils.ts
- app/client/src/pages/Editor/IDE/EditorPane/fuzzySearchInFiles.test.ts
✅ Files skipped from review due to trivial changes (1)
- app/client/src/IDE/utils/index.ts
🔇 Additional comments (4)
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (2)
56-61: LGTM! Clear and descriptive variable naming
The rename from groups to itemGroups better reflects the data structure and improves code readability.
64-67: Verify filter function behavior matches previous implementation
The implementation looks correct, but we should ensure the new filter maintains the same search experience.
Also applies to: 97-103
✅ Verification successful
Implementation verified with comprehensive test coverage
The new filterEntityGroupsBySearchTerm implementation is well-tested and maintains expected search behavior:
- Uses Fuse.js for fuzzy searching with appropriate threshold (0.1)
- Preserves empty search behavior (returns all groups)
- Handles exact matches and non-matches correctly
- Test cases cover empty search, exact match, no match, and partial match scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any tests comparing old vs new filter behavior
rg -l "fuzzySearchInObjectItems.*filterEntityGroupsBySearchTerm" -g "*.test.*"
Length of output: 80
Script:
#!/bin/bash
# Let's search for the old and new filter functions to understand the changes
rg -A 5 "fuzzySearchInObjectItems|filterEntityGroupsBySearchTerm" --type typescript
# Also search for test files related to these functions
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx -x rg -l "fuzzySearchInObjectItems|filterEntityGroupsBySearchTerm" {}
Length of output: 309
Script:
#!/bin/bash
# Let's examine the test file content to understand the test coverage
cat "./app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.test.ts"
# Let's also look for the implementation of both functions
rg -A 10 "fuzzySearchInObjectItems|filterEntityGroupsBySearchTerm" --type ts
Length of output: 11370
Script:
#!/bin/bash
# Let's look at the implementation of filterEntityGroupsBySearchTerm
cat "./app/client/src/IDE/utils/filterEntityGroupsBySearchTerm.ts"
# Also check if there's any trace of the old fuzzySearchInObjectItems implementation
rg "fuzzySearchInObjectItems" -B 5 -A 5
Length of output: 1322
app/client/src/pages/Editor/IDE/EditorPane/Query/List.tsx (1)
105-105:
Fix object length check
The length check Object.keys(itemGroups).length === 0 seems incorrect since itemGroups is an array (based on its usage in the map function above).
-{Object.keys(itemGroups).length === 0 && <BlankState />}
+{(!itemGroups || itemGroups.length === 0) && <BlankState />}Likely invalid or redundant comment.
app/client/src/pages/Editor/IDE/EditorPane/JS/List.tsx (1)
Line range hint 66-72: LGTM! Robust null checking
The implementation includes proper null checks and maintains consistency with the other files. The BlankState rendering condition is particularly well-handled.
Also applies to: 122-122
github-actions
bot
added
IDE Pod
## Description This pull request introduces a new utility function `filterEntityGroupsBySearchTerm` to replace the existing `fuzzySearchInObjectItems` function. Type inference is significantly improved. Additionally, some renaming was done to convey true meaning and improve readability. Fixes [appsmithorg#37769](appsmithorg#37769) ## Automation /ok-to-test tags="@tag.IDE" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12277260297> > Commit: 153de75 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12277260297&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IDE` > Spec: > <hr>Wed, 11 Dec 2024 13:55:20 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a new filtering functionality for entity groups based on search terms. - Added a test suite to validate the new filtering function across various scenarios. - **Bug Fixes** - Updated components to use the new filtering method, improving search result accuracy. - **Chores** - Removed obsolete fuzzy search functionality and associated tests to streamline the codebase. - **Refactor** - Renamed variables for clarity and consistency throughout the application. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
This pull request introduces a new utility function
filterEntityGroupsBySearchTermto replace the existingfuzzySearchInObjectItemsfunction. Type inference is significantly improved. Additionally, some renaming was done to convey true meaning and improve readability.Fixes #37769
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12277260297
Commit: 153de75
Cypress dashboard.
Tags:
@tag.IDESpec:
Wed, 11 Dec 2024 13:55:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Refactor