-
Notifications
You must be signed in to change notification settings - Fork 0
fix: prevent infinite growth of links in relations #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @MinCrohn, 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 resolves an issue where the graphical representation of relations could continuously grow due to an additive update mechanism. The change ensures that 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to fix an issue where the links array in Relations elements grows indefinitely with each update. The proposed solution is to enforce a 'replace' merge strategy for all updates on Relations elements. While this correctly changes the behavior for the links array, it unfortunately introduces a critical side effect, breaking partial style updates. Additionally, the tests that verify the links update behavior have not been updated to reflect the change, and will fail. My review includes a detailed comment on the problematic code with suggestions for a more robust solution.
| update(changes, options) { | ||
| super.update(changes, relationsSchema, options); | ||
| super.update(changes, relationsSchema, { | ||
| ...options, | ||
| mergeStrategy: 'replace', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this change to use mergeStrategy: 'replace' correctly fixes the issue of the links array growing on each update, it introduces two significant problems:
-
Broken Style Updates: This change causes a side effect where partial style updates no longer work. The
_applyRelationstylehandler also usesmergeStrategy. With'replace', it will replace the entire style on the underlying graphics object, instead of merging. For instance, updating just thewidthof a link will cause itscolorto be reset. This is a critical bug. -
Failing Tests: This change alters the behavior of
linksupdates from merging to replacement. However, the existing tests insrc/tests/render/Relations.test.jsandsrc/tests/undo-redo/Relations.test.jsstill expect the old merging behavior (e.g.,expect(updatedLinks.length).toBe(2)). These tests will now fail and need to be updated to reflect the new intended behavior.
A more targeted fix is needed. One approach is to refactor _applyRelationstyle to not depend on mergeStrategy. Alternatively, you could explore ways to apply the replace strategy only to the links array without affecting other properties during the update process.
Please address the style update bug and update the relevant tests.
No description provided.