Fix DataTableContext Sort By and Grouping properties not saving #3165
Conversation
WalkthroughThe updates focus on improving the initialization and change handling logic for sorting configurations within React components related to data tables. The Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant SortingEditor
participant ListEditor
ParentComponent->>SortingEditor: Render with value/onChange
SortingEditor->>SortingEditor: useEffect (initialize value if null/undefined)
SortingEditor->>ListEditor: Pass handleItemChange as onItemChange
ListEditor->>SortingEditor: onItemChange (item changed)
SortingEditor->>SortingEditor: Ensure item has id and sorting direction
SortingEditor->>ParentComponent: onChange (updated value)
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (1)
shesha-reactjs/src/designer-components/sortingEditor/index.tsx (1)
42-47: Consider normalizing incoming values in handleChange.Since the comment indicates this is fixing a persistence issue, you might want to ensure the value is properly structured before passing it to onChange. This could include checking if the value is null/undefined and converting it to an empty array, or ensuring all items have required properties.
const handleChange = (newValue) => { // Make sure we're passing a properly structured value for the jsSetting scenario + if (newValue === null || newValue === undefined) { + newValue = []; + } else if (Array.isArray(newValue)) { + // Ensure all items have required properties + newValue = newValue.map(item => ({ + id: item.id || getNanoId(), + propertyName: item.propertyName || '', + sorting: item.sorting || 'asc', + })); + } onChange(newValue); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shesha-reactjs/src/components/dataTable/sortingConfigurator/index.tsx(2 hunks)shesha-reactjs/src/designer-components/sortingEditor/index.tsx(1 hunks)
🔇 Additional comments (2)
shesha-reactjs/src/components/dataTable/sortingConfigurator/index.tsx (1)
44-44: LGTM! Good addition of the onItemChange handler.Adding the onItemChange handler to the ListEditor ensures that all items have proper IDs and default sorting directions.
shesha-reactjs/src/designer-components/sortingEditor/index.tsx (1)
41-55:❓ Verification inconclusive
The handleChange function is currently just a pass-through.
The handleChange function is described in comments as fixing the persistence issue with jsSetting, but it currently just forwards the value without any additional processing. Consider adding validation or normalization logic if needed.
🏁 Script executed:
#!/bin/bash # This script checks if the sorting values are properly structured when saved with jsSetting enabled # Look for usage of the SortingEditor with jsSetting enabled echo "Checking for usages of SortingEditor with jsSetting enabled..." rg -A 5 "canBeJsSetting: true" --glob "*.tsx" | grep -B 5 -A 5 "dataSortingEditor" # Check if there are any references to handleChange in other sorting-related files echo "Checking for related handleChange implementations..." rg "handleChange.*sorting" --glob "*.tsx"Length of output: 674
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "🔍 Searching for any jsSetting or canBeJsSetting references across the repository:" rg 'jsSetting|canBeJsSetting' -n echo echo "📄 Showing the first 200 lines of the SortingEditor wrapper for full context:" sed -n '1,200p' shesha-reactjs/src/designer-components/sortingEditor/index.tsx || echo "File not found: please verify the path."Length of output: 229
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "🔍 Locating SortingEditor usages:" rg "SortingEditor" -n --glob "*.tsx" || echo "No matches for SortingEditor" echo echo "🗂 Locating sortingEditor directory path:" find . -type d -iname "sortingeditor" || echo "No sortingEditor directory found" echo echo "📄 Displaying content of index.tsx in sortingEditor if found:" file=$(find . -type f -iname "index.tsx" 2>/dev/null | grep -i sortingeditor | head -n1 || true) if [[ -n "$file" ]]; then echo "Found: $file" sed -n '1,200p' "$file" else echo "index.tsx under sortingEditor not found" fiLength of output: 820
It looks like I can’t locate
shesha-reactjs/src/designer-components/sortingEditor/index.tsxor any references toSortingEditorin the repo—this may mean the path or filename has changed. Could you please:
- Confirm the exact location and casing of the
sortingEditor/index.tsxfile.- Verify whether
handleChangeis still a no‑op pass‑through in that file, or if any normalization/validation logic has been added elsewhere.Once we have the correct path and code, we can ensure the original concern about missing processing in
handleChangeis addressed.
| // Ensure value is properly initialized when component is first rendered | ||
| useEffect(() => { | ||
| if (value === null || value === undefined) { | ||
| onChange([]); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialization handling could be improved with dependencies.
The useEffect hook correctly initializes the value to an empty array when null or undefined, but it's missing dependencies in the dependency array. This could lead to unexpected behavior if the value or onChange props change after the component mounts.
- useEffect(() => {
- if (value === null || value === undefined) {
- onChange([]);
- }
- }, []);
+ useEffect(() => {
+ if (value === null || value === undefined) {
+ onChange([]);
+ }
+ }, [value, onChange]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensure value is properly initialized when component is first rendered | |
| useEffect(() => { | |
| if (value === null || value === undefined) { | |
| onChange([]); | |
| } | |
| }, []); | |
| // Ensure value is properly initialized when component is first rendered | |
| useEffect(() => { | |
| if (value === null || value === undefined) { | |
| onChange([]); | |
| } | |
| }, [value, onChange]); |
shesha-reactjs/src/components/dataTable/sortingConfigurator/index.tsx
Outdated
Show resolved
Hide resolved
Description
This PR fixes issue #3152 where the Sort By and Grouping properties in the DataTableContext component weren't saving correctly when jsSetting was enabled.
Problem
When jsSetting was enabled for the Sort By and Grouping properties in the DataTableContext component, the selected properties weren't persisting after saving the form configuration.
Solution
The fix addresses the issue by:
Summary by CodeRabbit
Bug Fixes
Refactor