Skip to content

Publish the outbox's summary and its waiting work together - #550

Merged
SunkenInTime merged 2 commits into
mainfrom
t3/outbox-publishes-whole
Nov 12, 2026
Merged

SunkenInTime merged 2 commits into
mainfrom
t3/outbox-publishes-whole

Conversation

@SunkenInTime

Copy link
Copy Markdown
Owner

Nothing to show on screen: the bug is a race a few milliseconds wide, and the fix is in when the outbox publishes its state. The numbers:

main this PR
New test: edit an item, a teammate's edit to another item arrives before yours is stored your edit is reverted on the canvas and the old value is sent to the server your edit lands
Time to queue a 2,000-item page (test VM) 7.8 s 0.2 s

Why

In cloud mode, an edit could be thrown away without a word. You change an item. A teammate's change to a different item arrives in the moment after your edit and before the outbox has stored it. Your edit is reverted on the canvas, and the next save sends the old value to the server. The chip says synced.

What was happening

_putRecord stored the new record and published the account summary straight away. The open strategy's waiting work (the five per-item maps) was only updated after _syncDesiredLocked finished its loop. For that moment the queue state counted the new change in its summary and left it out of pending.

StrategySaveStateNotifier read pending.isEmpty, cleared the unsaved mark, and the page session ran the redraw from the server that had been waiting for a clean canvas. That redraw dropped the overlay holding the edit. The edit still landed; the older page then replaced it on the canvas, and the next save diffed the reverted canvas and sent the old value.

What this does

  • _publishAccountSummary now publishes the summary and the waiting work together, both built from the stored records by _refreshActiveQueueView. A state with one and not the other can no longer be published.
  • While a page's items are stored one by one, the publish waits until the loop ends. Main republished the summary per item, which is where the 7.8 s went.

How far this was checked

  • New test an edit made offline lands when a teammate's edit to another item arrived first fails on main (server keeps alpha, expected mine) and passes here.
  • fvm flutter test: 3,570 passed, 14 skipped.
  • 10,000 seeded random schedules of edits, lost answers, offline spells and teammate changes pass with this fix and the map fix together. Four of the first 6,000 failed without this fix. That test is its own PR.
  • Reviewed for a state that is wrong in a new way at each kind of caller (reconcile, page changes, claim, acks, failure restore, discard, account switch). None found.

Not covered

  • Not reproduced in the running app. Two real accounts in two browsers, five trials of one person holding a drag while the other placed an agent, lost nothing on main. The window is the gap between finishing an edit and the outbox storing it, with nothing else waiting, and one teammate edit per trial does not land in it. The test forces the gap open.
  • Separate and not fixed here: if the client is ever handed a page read older than a change it already had acknowledged, the canvas can still be rolled back. A test can do it by injecting an old read. Whether the real client delivers reads in that order is not established.

ci: analyze test web gauntlet

Storing a change published the account's summary before the open
strategy's waiting work included it. For that moment the outbox read as
empty, the canvas was marked saved, and a redraw from the server waiting
for that ran over the edit just stored. The edit still landed; the older
page then replaced it on the canvas, and the next save sent the old
value back.
Each item stored while a page is handed to the outbox republished the
whole outbox. A 2,000 item page took 7.8 s to queue; it takes 0.2 s.
@coderabbitai

coderabbitai Bot commented Nov 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4c158210-3498-485c-9a3d-e6e2f1fe7b08
Commits

Reviewing files that changed from the base of the PR and between c995517 and 0b97921.

Files selected for processing: 2

  • lib/providers/collab/strategy_op_queue_provider.dart
  • test/strategy_page_session_provider_test.dart
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@SunkenInTime
SunkenInTime merged commit 7622872 into main Nov 12, 2026
16 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