sorting editor updates on local items

[teboho]

#2977

Fixing this issue
https://jam.dev/c/42df9ebb-d728-42e2-bc35-cc02584514f5

Summary by CodeRabbit

• Refactor
  • Streamlined sorting interactions for cleaner change handling.
  • Enhanced list editing by updating items immutably, ensuring the UI reflects changes immediately.

[coderabbitai]

Walkthrough

This PR updates two components. In the SortingEditor component, the second argument (undefined) has been removed from itemOnChange calls within PropertyAutocomplete and Select. In the ListEditorRenderer component, a new parameter updateItem is added to the context, and the localItemChange function is refactored to perform immutable updates that ensure the UI refreshes immediately after changes.

Changes

File(s)	Change Summary
shesha-reactjs/.../dataTable/sortingConfigurator/index.tsx	Removed the redundant second argument from itemOnChange calls in both PropertyAutocomplete and Select components.
shesha-reactjs/.../listEditor/renderer.tsx	Introduced a new updateItem parameter in context and refactored localItemChange for immutable item updates with an immediate UI refresh.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant LE as ListEditorRenderer
    participant UI as User Interface

    U->>LE: Trigger item change event
    LE->>LE: Merge existing item with new data (immutable update)
    LE->>LE: Call updateItem(index, updatedItem)
    LE->>UI: Refresh the UI to reflect changes

Loading

Possibly related PRs

• JS Setting for dataSortingEditor type of properties #3105: Adjustments to props passed to the SortingEditor component, indicating a close code-level relationship with the current simplifications.

Suggested reviewers

• James-Baloyi

Poem

I'm a bunny hopping through lines of code,
Nibbling away excess with a joyful mode.
Sorting and listing now dance in perfect tune,
Updates flowing smoothly like a sunny afternoon.
With a twitch of my nose, I cheer every change,
Hopping forward in code, where magic rearrange!
🐇✨

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f4a79c and 441306b.

📒 Files selected for processing (2)

• shesha-reactjs/src/components/dataTable/sortingConfigurator/index.tsx (2 hunks)
• shesha-reactjs/src/components/listEditor/renderer.tsx (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: build-attempt

🔇 Additional comments (4)

shesha-reactjs/src/components/dataTable/sortingConfigurator/index.tsx (2)

38-40: Removed unnecessary second parameter from itemOnChange call

The removal of the undefined second argument simplifies the code while maintaining the same functionality. This change aligns with the modifications in the ListEditorRenderer component.

---

47-49: Removed unnecessary second parameter from itemOnChange call

Similar to the previous change, removing the undefined second argument from the Select's onChange handler improves code cleanliness without affecting functionality.

shesha-reactjs/src/components/listEditor/renderer.tsx (2)

87-87: Added updateItem to destructured context properties

The addition of updateItem to the list of destructured properties from contextAccessor() provides direct access to the item update functionality needed in the refactored localItemChange function.

---

154-163:

❓ Verification inconclusive

Improved item update mechanism for immediate UI feedback

The refactored localItemChange function now ensures that updates are immediately reflected in the UI by:

1. Creating a new object that preserves immutability
2. Directly calling updateItem to update the item at the specified index
3. Forcing a UI refresh to ensure changes are visible

This change addresses issues where item updates might not have been immediately reflected in the UI.

Be aware that using object spread ({ ...item, ...newValue }) performs a shallow merge. If your items contain nested objects that need updating, you might need to implement a deep merge strategy:

---

🏁 Script executed:

#!/bin/bash
# Check if there are any nested objects in the items that might be affected by shallow merging

# Look for array or object property assignments in the codebase that might interact with this component
rg -U "\.\.\..*\{.*\}" --type=tsx --type=ts

Length of output: 73

---

Below is an updated review comment that includes a note to verify whether the shallow merge is appropriate, given that our initial search for nested objects produced an error due to file‑type recognition. Please run the new shell script below to verify if there are any instances of nested objects affected by the shallow merge. Once confirmed, ensure that either the item structure is flat or consider a deep merge where needed.

---

Immediate UI Update with Shallow Merge Consideration

The refactored localItemChange function creates a new object by merging item and newValue, updates the item via updateItem, and forces a UI refresh using refresh(true), ensuring updates are immediately reflected in the UI.

Note: The object spread ({ ...item, ...newValue }) performs a shallow merge. If any items contain nested objects, their nested properties won’t be deeply merged. Please verify whether the item structure might include nested objects that need a deep merge strategy.

Action Items:

• Confirm that the items passed into this component are flat.
• If nested objects are present, consider testing or implementing a deep merge to ensure nested values update as expected.

For further verification, please run the following script to search for object spread patterns in TS/TSX files:

---

🏁 Script executed:

#!/bin/bash
# Adding custom types for TSX and TS files and searching for shallow merge patterns with object spread
rg --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -U "\{\s*\.\.\." .

Length of output: 189

---

Immediate UI Update with Shallow Merge – Manual Verification Needed

The updated localItemChange function constructs an updated item using the object spread ({ ...item, ...newValue }), calls updateItem to update the item at the specified index, and forces a UI refresh via refresh(true). This ensures that UI changes are immediately visible.

Action Items:

• Confirm Item Structure: Verify that the items are flat. Since the object spread performs a shallow merge, any nested objects won’t be deeply merged. If nested objects exist, you may need to adopt a deep merge strategy.

• Manual Verification: A previous shell command intended to search for shallow merge patterns resulted in an error because the `` placeholder was included in the command. Please run the corrected command manually:

  #!/bin/bash
  rg --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -U "\{\s*\.\.\." .

  This will help confirm that there are no cases of nested objects affected unintentionally by the shallow merge.

---

Immediate UI Update – Ensure Deep Merge is Not Required

The refactored localItemChange function now constructs an updated item by shallowly merging item with newValue via { ...item, ...newValue }, then updates the item using updateItem(index, updatedItem) and forces a UI refresh with refresh(true). This guarantees that UI changes are immediately visible.

Action Items:

• Confirm Item Structure: Verify that the items are flat. Since the current merge strategy is shallow, any nested objects within the item will not be deeply merged.
• Manual Verification Needed: Please manually inspect the item structure or run a proper search without the `` placeholder to ensure that no nested objects need deep merging. For example, run:

  rg --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -U "\{\s*\.\.\." .

---

UI Update Reliability with Shallow Merge

The revised localItemChange function now constructs an updated item via a shallow merge ({ ...item, ...newValue }), updates the item using updateItem(index, updatedItem), and forces a UI refresh with refresh(true). This approach ensures that updates are immediately reflected in the UI. However, because object spread only performs a shallow merge, if the item contains nested objects, those nested properties will not be deeply merged.

Please verify that the items passed to this component are flat. If there is any chance that items include nested structures that need proper merging, consider implementing a deep merge strategy.

Manually inspect the item structure or run a targeted search (without placeholders like ``) to confirm that there are no nested objects that could be adversely affected.

---

Enhanced Immediate UI Update with Shallow Merge Consideration

The refactored localItemChange function now creates an updated item by shallowly merging item with newValue using the object spread operator ({ ...item, ...newValue }). It then updates the item via updateItem(index, updatedItem) and forces a UI refresh with refresh(true), ensuring that updates are immediately reflected.

Note: The use of object spread results in a shallow merge. If your items might contain nested objects, their nested properties will not be merged deeply. In that case, please verify that the item structure is flat or consider implementing a deep merge strategy.

Action Items:

• Inspect Item Structure: Confirm that the items passed into this component do not include nested objects that require deep merging.
• Manual Verification: If there's any possibility of nested objects, manually verify the component’s behavior or run an appropriate search (e.g., using rg) to identify any shallow merge issues.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[IvanIlyichev]

Hi @teboho. List editor is used by different components including lists with groups. Please make sure that your changes work in all of them. One of example of issues reintroduiced by your changes: #1690
There were some reasons for mutability. Ideally it should be immutable but it requires more changes

[teboho]

Hi @IvanIlyichev thank you for the feedback,I will take further look

[teboho]

Hi @IvanIlyichev , I saw that the TableViewSelector currently on functional is already not working correctly which means that already on main something is wrong with it, I've alerted @Tshepo1103-lab about it, while I did revert the changes this PR was proposing I still need to test if trying to use those changes still affects the TableViewSelector negatively?

---

I will reapply the changes once TableViewSelector works again and test all the places where the list editor is used to see if they still work as in v43