feat: only mark child changed if child is modified in TreeView #79
Conversation
Summary of ChangesHello @ensi321, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of getElement and getField unnecessarily marking child nodes as dirty. The introduction of the ParentChange struct and the markChildChanged function is a good approach to propagate the dirty status up the tree only upon actual modification.
However, I've identified a critical pre-existing issue with how sub-views are handled. When getElement or getField returns a TreeView, it contains a copy of the child's Data struct. Any subsequent modifications on this sub-view (e.g., using setElement or setField) are made to this copy and are lost when the sub-view goes out of scope. While the change is propagated to the parent's changed map, the actual data modification within the sub-view is not persisted, which means it won't be included in a commit.
For the TreeView API to work as expected for modifications on nested structures, TreeView should likely hold a pointer to its Data (data: *Data) rather than an instance (data: Data). This would ensure that all views of a certain piece of data are modifying the same instance. This would be a larger refactoring but is crucial for the correctness of modifying nested data. I've added specific comments where this issue occurs.
| return TreeView(ST.Element){ | ||
| .allocator = self.allocator, | ||
| .pool = self.pool, | ||
| .data = child_data, |
There was a problem hiding this comment.
This line copies the child_data struct into the new TreeView. This leads to a critical issue: any modifications made to the returned sub-view (e.g., by calling setElement or setField on it) are applied to this temporary copy and are lost when the view goes out of scope.
While the new parent_change mechanism correctly marks the parent as dirty, the actual data change within the sub-view is not persisted in the parent's children_data cache. Consequently, when commit is called on the parent, the sub-view's changes are not included.
To fix this, TreeView should probably hold a pointer to Data (data: *Data) instead of a value (data: Data). This would ensure that sub-views modify the shared Data instance stored in the parent's cache.
| return TreeView(ChildST){ | ||
| .allocator = self.allocator, | ||
| .pool = self.pool, | ||
| .data = child_data, |
There was a problem hiding this comment.
Similar to getElement, this line copies the child_data struct, which means any modifications on the returned TreeView are made on a temporary copy and will be lost.
This is a critical issue for correctness, as it prevents modifications to nested data structures from being persisted and committed. For the API to function as expected, TreeView should likely manage its Data via a pointer (data: *Data) to ensure all views operate on a shared data instance.
There was a problem hiding this comment.
the solution may work but I found it pretty complex, since right now the design is top-down (parent tracks child data) and this introduces bottom-up approach
note that our js version handles this by doing very cheap comparision here https://github.com/ChainSafe/ssz/blob/7f5580c2ea69f9307300ddb6010a8bc7ce2fc471/packages/ssz/src/viewDU/container.ts#L103
Currently
getElementandgetFieldcall will mark child dirty whether child is modified or not since parent has no visibility to track it.Pass parent's data.changed and child's gindex to child TreeView, so when child is modified, parent can keep track of the changes.