Fix folder-move UI: tree state, stale list, optimistic feedback - #6
Merged
Conversation
Closes #2, #3, #4. Adds tests/Desktop.Tests, which the presentation layer did not have. Writing the reproductions first paid for itself immediately: #2 reproduced, and #3 did not. With an instant-write in-memory vault the item list updated correctly all along, which ruled out the hypothesis in the issue -- the view-model logic was never wrong. The real cause of #3 is that LoadAsync begins with a server sync. Straight after one of our own writes that can only re-fetch state the server has not applied yet, so the change appears to have been lost. Reloads following a local write no longer sync; an explicit Refresh still does, because that is what Refresh is for. Both directions are pinned by tests. #2 was as diagnosed: RebuildFolderTree discards the FolderNode objects that hold the expansion state. Collapsed paths are captured before the rebuild and reapplied after -- collapsed rather than expanded, so a folder that appears for the first time comes up open. #4 dims affected rows the moment a drop happens and rolls the dimming back if the write fails, per the approach on #3. Dimming rather than removing: a removed row that had to be restored would reappear at a different position in a sorted list and read as a glitch. Optimism stops at moves -- one reversible field on one item. It does not touch the merge path, which writes, verifies, and only then deletes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2, closes #3, closes #4.
Adds
tests/Desktop.Tests— the presentation layer had none.Writing the reproductions first changed the diagnosis. #2 reproduced; #3 did not. With an
instant-write in-memory vault the item list updated correctly all along, so the view-model logic
was never the problem and the hypothesis I filed on the issue was wrong.
The actual cause of #3:
LoadAsyncstarts with a server sync, and immediately after one of ourown writes that can only re-fetch state the server has not applied yet — so the change looks lost.
Reloads after a local write no longer sync; explicit Refresh still does. Both pinned by tests.
#2 was as diagnosed —
RebuildFolderTreediscards the objects holding the expansion state.Collapsed paths are now captured and reapplied, keyed on collapsed rather than expanded so a
newly appearing folder opens by default.
#4 dims affected rows on drop and rolls back on failure, following the approach agreed on #3.
Dimming rather than removing, because a restored row would reappear elsewhere in a sorted list.
Optimism is scoped to moves — one reversible field on one item. The merge path still writes,
verifies, and only then deletes.
11 new tests, 98 total, zero warnings.