Apply teammates' changes item by item instead of reloading the page - #208
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…keep held items in place Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@coderabbitai review |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe editor now tracks canvas pointers and held entities. Remote page updates merge into the active page while preserving held items, drafts, and compatible ordering. Remote page replacement can wait until editor activity permits it. ChangesCollaborative page sync
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant StrategyPageSessionNotifier
participant ActivePageLiveSyncNotifier
participant StrategyPageApply
participant EditorProviders
StrategyPageSessionNotifier->>ActivePageLiveSyncNotifier: Compare remote snapshot with hydrated base
StrategyPageSessionNotifier->>StrategyPageApply: Merge page data using changed keys and held entity IDs
StrategyPageApply->>EditorProviders: Merge remote item collections
StrategyPageSessionNotifier->>ActivePageLiveSyncNotifier: Record snapshot and hydration key
Merge Risk: ⚪ Minimal · up to Teammates' changes now merge into the open page item by item. Items you are holding or drafting stay as you left them, and deleting a page waits until your current edit finishes. The review found no concrete defect. A live two-client session has not been exercised, but nothing identified blocks merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The client now merges teammate changes while editing continues. Its page checks and retained item revisions limit the apparent risk of overwriting another person's work, but concurrent behavior and server-side rejection have not been verified end to end. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
✅ Action performedReview finished.
|
…emote-apply # Conflicts: # lib/interactive_map.dart
This comment has been minimized.
This comment has been minimized.
… paths Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reusing unchanged items on a merge needed live sync to track what the canvas drew separately from what the server acked, and each attempt at that left another way to rewind a revision. Server-wins for every item the user isn't holding is the reviewed behaviour; its path rebuild cost matches the full reload it replaced. The three new tests still hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-up to #207. #207 stopped a teammate's change from wiping a stroke in progress by making the whole page wait until you let go. This PR lets both happen at once: you keep drawing while their change shows up.
Why the wait was needed
Any remote change reloaded the whole page.
applyStrategyEditorPageDatacleared all seven canvas lists and refilled them from the server. Anything held only in memory was lost: an unfinished stroke, a text draft, lineup placement, the undo "before" state of an edit in progress, and drag/rotate/resize values kept in widget state.What changes
_mergeRemotePagereplaces only the items (each list gets a newmergeRemote). Nothing is cleared, so strokes, drafts, placement and pending undo state survive. Map, settings and theme apply as before. History is reconciled the same way.EditorEntityLayerwraps each canvas item in its own full-size layer. A press on an item holds that item; a press on empty canvas (drawing, panning) holds nothing. A press anywhere else (sidebar, dialogs, panels, toasts) still holds back the whole update, as in Keep unfinished canvas edits when a teammate's change streams in #207. Open text drafts and a lineup placement's pinned origin/landing count as held. A copy made by duplicate-drag is held with its source.markPageHydrated(keepBaseFor:)). If you drop it after a teammate changed or deleted it, the op carries the old revision, the server rejects it, and the existing conflict flow shows it. It is never silently overwritten or re-added. When you let go without editing, it takes the server's copy.Stacking order (
sortIndex): an existing bug, fixed along the wayLive sync numbered elements by list position, but the server never renumbers (deletions leave tombstones). So after any teammate deletion, your next edit re-sent every later element's
sortIndex. Those were edits you never made, and they could conflict with a teammate working on those items.icarus-clouddoes this today. Elements now follow the rule lineup rows already use: an element keeps its knownsortIndex, and new ones go after the page's highest. Moving an item brings it to the front of its list (as before), and only that item gets a fresh index. Ties from legacy pages or concurrent adds use the order the page was drawn in.Reproduction → verification
Two people on one page. One draws or drags while the other edits. Before: the page reloaded (after #207, it waited for the release). Now: the teammate's edit appears while you draw. If they edited the item you're holding, it updates when you let go, or your drop becomes a conflict.
flutter test: 1231 passed, 5 skipped (the usual skips), none failing.test/strategy_page_session_provider_test.dart:sortIndexgrouptest/editor_operation_scope_test.dart: what a press holds (item / canvas / elsewhere), duplicates.test/canvas_entity_hold_test.dart: presses on realPlacedWidgetBuilderitems, and that dragging still works through the layer.test/remote_page_merge_test.dart: the merge helper.sortIndexdata. It checked each merge against a fresh load of the same snapshot, and simulated landing the wayconvex/elements.tsorders rows. Round 4: 33/33 scenarios passed.Known and not in this PR
Also existing on
icarus-cloud: if you start a lineup from an origin a teammate deletes, the finished lineup lands, but no reader can draw it, and it disappears with no notice. The server doesn't check that a link's origin and landing exist (assertLineupPayloadinconvex/ops.ts). The fix is a server-side check, so it's left for its own PR.Server
Client-only. No Convex schema or function changes, so production data needs no migration.
🤖 Generated with Claude Code
Summary by CodeRabbit
No outstanding findings block merging.
Summary
This PR applies teammates’ page changes item by item instead of reloading the canvas, so work in progress stays visible.
Reviews (5) · Last reviewed commit: "Merge remote-tracking branch 'origin/ica..."