Skip to content
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

Fix: Update data of roots with modified content only #535

Merged
merged 2 commits into from
Sep 30, 2024
Merged

Fix: Update data of roots with modified content only #535

merged 2 commits into from
Sep 30, 2024

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Sep 24, 2024

Suggested merge commit message (convention)

Fix: Update data of roots with modified content only. Closes #534.


Additional information

See #534 for more details. TL;DR:

This happens because setData returned from useMultiRootEditor hook, expects all roots to be passed (since it diffs root list enabling to add/remove them) and existing roots are just updated with passed data, even if it's the same. Such update results in each root model data to be wiped out and re-added which is interpreted by revision history as complete data replace (since it works on model operations and does not diff data changes).

This PR slightly modifies the logic to update only the roots which has changed data.


I dug a bit in the context of below comment in the code:

// If any of the roots content has changed, set the editor data.
// Unfortunately, we cannot set the editor data just for one root,
// so we need to overwrite all roots (nextProps.data is an
// object with data for each root).

But didn't find any case or code which would suggest that we can't do it. All tests green, manuals works fine AFAIT too.

src/useMultiRootEditor.tsx Outdated Show resolved Hide resolved
src/useMultiRootEditor.tsx Outdated Show resolved Hide resolved
@coveralls
Copy link

Pull Request Test Coverage Report for Build 0b08803a-48ca-427a-99e2-d30e6c5fb868

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 434d9ed7-0b25-40f8-a191-f513ef65280a: 0.0%
Covered Lines: 584
Relevant Lines: 584

💛 - Coveralls

@f1ames f1ames merged commit 8f5232c into master Sep 30, 2024
6 checks passed
@f1ames f1ames deleted the ck/534 branch September 30, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting data of any root with MultiRoot editor results in revision history marking all roots as changed
4 participants