Skip to content

Apply teammates' changes item by item instead of reloading the page - #208

Merged
SunkenInTime merged 13 commits into
icarus-cloudfrom
incremental-remote-apply
Sep 28, 2026
Merged

SunkenInTime merged 13 commits into
icarus-cloudfrom
incremental-remote-apply

Conversation

@SunkenInTime

@SunkenInTime SunkenInTime commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

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. applyStrategyEditorPageData cleared 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

  • Item-level merge. When the page on screen gets a newer server copy, _mergeRemotePage replaces only the items (each list gets a new mergeRemote). 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.
  • Only the item under your finger waits. EditorEntityLayer wraps 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.
  • Honest conflicts. A held item keeps the content and revision you saw (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.
  • Replacing the page still waits. If the page on screen no longer exists (a teammate deleted it), the full replace waits until nothing is in progress.
  • Lineups update as one graph. Links name their origin and landing, so while you hold any lineup piece, lineup changes wait.
  • Rotation handles look their ability or view cone up by id when you let go, not by the list position captured at build time.

Stacking order (sortIndex): an existing bug, fixed along the way
Live 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-cloud does this today. Elements now follow the rule lineup rows already use: an element keeps its known sortIndex, 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.
  • New tests, each confirmed failing before its fix:
    • test/strategy_page_session_provider_test.dart:
      • stroke kept while an update lands
      • lineup placement
      • held item waits, then updates on release
      • edit to a held item sent at the old revision
      • held item deleted by a teammate
      • text draft holds its text
      • pinned lineup origin
      • page deleted mid-stroke
      • undrawable lineup base
      • the sortIndex group
    • test/editor_operation_scope_test.dart: what a press holds (item / canvas / elsewhere), duplicates.
    • test/canvas_entity_hold_test.dart: presses on real PlacedWidgetBuilder items, and that dragging still works through the layer.
    • test/remote_page_merge_test.dart: the merge helper.
  • The gauntlet: four rounds with two independent reviewers.
    • Codex (GPT-6 Astra), static read: 4, 2, 2, then 0 findings.
    • A Claude agent writing adversarial tests with realistic tombstoned sortIndex data. It checked each merge against a fresh load of the same snapshot, and simulated landing the way convex/elements.ts orders rows. Round 4: 33/33 scenarios passed.
    • Every finding is fixed here.
  • Not verified: a live two-client session. This Windows machine has no Visual Studio toolchain for a native build. Everything is covered by provider and widget tests only.

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 (assertLineupPayload in convex/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

  • New Features
    • Remote updates can now be applied while you work, preserving items you’re actively holding or editing and keeping canvas order where possible.
    • Duplicated items stay with their source during a drag.
  • Bug Fixes
    • Rotation and size changes now apply to the correct item after canvas updates.
    • Active drawings and placements are better preserved as remote changes arrive.

RetriggerConfidence Score: 5/5

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..."

SunkenInTime and others added 5 commits September 27, 2026 03:10
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>
@SunkenInTime

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a167c053-a749-4ca8-b9ff-6812613d265d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 52b6621b-278f-43d2-8010-3fcace423edd

📥 Commits

Reviewing files that changed from the base of the PR and between ff825f2 and 0abaa62.

📒 Files selected for processing (20)
  • lib/interactive_map.dart
  • lib/providers/ability_provider.dart
  • lib/providers/agent_provider.dart
  • lib/providers/collab/active_page_live_sync_provider.dart
  • lib/providers/drawing_provider.dart
  • lib/providers/editor_operation_provider.dart
  • lib/providers/image_provider.dart
  • lib/providers/strategy_page_session_provider.dart
  • lib/providers/text_provider.dart
  • lib/providers/utility_provider.dart
  • lib/strategy/remote_page_merge.dart
  • lib/strategy/strategy_page_apply.dart
  • lib/widgets/draggable_widgets/ability/placed_ability_widget.dart
  • lib/widgets/draggable_widgets/placed_widget_builder.dart
  • lib/widgets/draggable_widgets/utilities/placed_view_cone_widget.dart
  • lib/widgets/editor_operation_scope.dart
  • test/canvas_entity_hold_test.dart
  • test/editor_operation_scope_test.dart
  • test/remote_page_merge_test.dart
  • test/strategy_page_session_provider_test.dart

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Collaborative page sync

Layer / File(s) Summary
Track canvas pointers and entity holds
lib/providers/editor_operation_provider.dart, lib/widgets/editor_operation_scope.dart, lib/interactive_map.dart, lib/widgets/draggable_widgets/placed_widget_builder.dart, lib/providers/ability_provider.dart, lib/providers/agent_provider.dart, test/editor_operation_scope_test.dart, test/canvas_entity_hold_test.dart
Canvas pointers record whether they began on the canvas and which entities they hold. Entity layers identify pressed items, and duplicated abilities or agents join the source item's hold. Tests cover canvas, entity, and duplicate holds.
Merge remote page items
lib/strategy/remote_page_merge.dart, lib/strategy/strategy_page_apply.dart, lib/providers/{ability,agent,drawing,image,text,utility}_provider.dart, lib/widgets/draggable_widgets/ability/placed_ability_widget.dart, lib/widgets/draggable_widgets/utilities/placed_view_cone_widget.dart, test/remote_page_merge_test.dart
Providers merge incoming items by ID using a keep predicate. Page application retains held lineup graphs and reconciles action history. Ability and view-cone rotation commits resolve the item's current index before updating it.
Reconcile hydration and canvas ordering
lib/providers/collab/active_page_live_sync_provider.dart
Live sync records hydrated canvas positions and held deletions, compares remote snapshots with hydrated state, and calculates element and lineup sort indexes.
Apply remote updates during editor activity
lib/providers/strategy_page_session_provider.dart, test/strategy_page_session_provider_test.dart
The session notifier validates remote updates, merges into an active page, and defers replacement or held-back changes when required. Tests cover active drawing and lineup placement, held entities, drafts, deletions, and ordering.

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
Loading

Merge Risk: ⚪ Minimal · up to 0abaa

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 Review

Security architecture risk: 🟡 Moderate · up to 0abaa

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A collaborator's incoming changes can affect what another editor sees on the shared active page. The inspected mutation path remains keyed to that strategy and page rather than widening writes to other pages.

Trust Boundaries and Controls

  • observed — A remote change or deletion does not replace a held item's drawn revision in the client hydration base. Subsequent edits carry an expected revision, so rejection of a stale write depends on the existing server contract.

Resilience and Maintainability Implications

  • observed — Held-back updates are scheduled for reconsideration when editor holds change; stale asynchronous loads are checked before they can apply to a changed session.

Hardening Proposals

  • proposed — Validate the existing server conflict behavior with two-client cases covering a held item changed or deleted remotely, release without an edit, and retry after interruption.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: applying teammates' updates item by item instead of reloading the full page.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…emote-apply

# Conflicts:
#	lib/interactive_map.dart
Comment thread lib/providers/drawing_provider.dart
@greptile-apps

This comment has been minimized.

SunkenInTime and others added 5 commits September 28, 2026 09:16
… 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>
Comment thread lib/providers/collab/active_page_live_sync_provider.dart
@SunkenInTime
SunkenInTime merged commit d967315 into icarus-cloud Sep 28, 2026
6 checks passed
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.

1 participant