Deep merge User & System Filters for DataGrid #1616
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/components/Grid/useDataGridSource.tsx (1)
Line range hint
278-344: Consistent implementation of deep merging strategyThe implementation consistently uses lodash
mergeacross all variable compositions (variablesWithFilter, firstPage query, and onFetchRows), which is a solid architectural choice. This unified approach ensures that filters are properly preserved throughout the DataGrid's lifecycle.Consider documenting this pattern in the component's documentation to ensure future modifications maintain this consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Grid/useDataGridSource.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Grid/useDataGridSource.tsx (2)
36-36: LGTM: Import of lodash merge function
The addition of merge from lodash is appropriate for implementing deep merging of variables and input.
278-279: LGTM: Simplified variables merging logic
The use of lodash merge ensures proper deep merging of variables and input, which addresses the PR objective of preventing filter overwrites.
Let's verify the structure of merged variables:
✅ Verification successful
Merging behavior appears to be correctly implemented
From the context we can see that:
- The
variablesWithFilteris used specifically for filtered queries - Pagination fields (count, sort, order) are properly excluded before merging
- The merged result is used consistently in both cache updates and filtered queries
- The deep merge with lodash ensures proper object composition
The implementation aligns with the intended behavior and shows proper separation of concerns between filtered and unfiltered data handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests or usage patterns of variablesWithFilter to ensure proper merging
rg -l "variablesWithFilter" --type ts
Length of output: 79
Script:
#!/bin/bash
# Let's examine the usage context of variablesWithFilter in the file
rg "variablesWithFilter" -B 5 -A 5 --type ts src/components/Grid/useDataGridSource.tsx
Length of output: 850
Script:
#!/bin/bash
# Let's examine the structure of variables and input to understand the merge operation
ast-grep --pattern 'const variables = useMemo' -A 10
Length of output: 54
Script:
#!/bin/bash
# Let's try a different approach to examine the variables structure
rg "const variables =" -B 5 -A 10 --type ts src/components/Grid/useDataGridSource.tsx
Length of output: 87
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
CarsonF
changed the title
This fixes a problem where user input filters were getting overwritten. Now
variablesandinputdeep merge properly for data requests.Summary by CodeRabbit