Skip to content

Offline replay: browser reconnect coverage and remaining self-conflict fixes - #287

Merged
brianorwhatever merged 4 commits into
mainfrom
fix/238-offline-replay
Oct 8, 2026
Merged

brianorwhatever merged 4 commits into
mainfrom
fix/238-offline-replay

Conversation

@brianorwhatever

@brianorwhatever brianorwhatever commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Closes #238.

Context

The core redesign for #238 already landed in #253: operation IDs, durable server receipts, expected-revision and predecessor checks inside the Convex mutation transaction, and receipt-based optimistic reconciliation. #253 deliberately left the issue open. I re-audited current main against the issue checklist. This PR closes the gaps that audit found.

What changed

1. Browser E2E now enforces replay semantics (test-only).
The loopback Convex fixture (tests/fixtures/server.mjs) used to accept every *Replay mutation without checking revisions. A regression of #238 would have passed CI's browser suite. The fixture now mirrors convex/lib/replay.ts:

  • receipts keyed by operation ID, with a fingerprint check;
  • the target set must match the expected entries;
  • predecessor revisions are taken from receipts;
  • REPLAY_CONFLICT is sent as ConvexError errorData.

It also gains per-account controls for a backend outage, committed-but-lost responses, and a collaborator edit.

New tests in tests/offline-replay.e2e.ts:

  • Check then uncheck offline, across a reload. Both edits replay in order and the final server state is unchecked.
  • Lost acknowledgment. The retry reuses the same operation ID and the edit is applied once. A later edit chains through the recovered receipt.
  • Collaborator edit. It surfaces as a recoverable conflict, is not last-write-wins, and can be explicitly reapplied.

2. Two remaining self-conflicts fixed (src/).

  • Pinned draft base. A details draft opened on an optimistic row pinned its predecessor to that row's operation. A later edit by the same user to the same item (e.g. unchecking it in the list) then made the draft conflict.
    • Enqueue now chains through the newest of the client's own later edits that descend from the pinned base.
    • A collaborator edit in between still conflicts. A guard test covers this.
  • Snapshot-less queueing. BatchOperations and ListItem remove queued without a base. For an item not yet in the IndexedDB cache, the edit was recorded with revision: 'unknown' and listIds: []. It was never shown optimistically and always rejected.
    • Both now go through useOptimisticItems (removeItem, new queueBatch), so every list edit uses the same server/cache base rows.
  • Conflict review shows chained edits. Reapplying a conflict also reapplies the later edits that build on it. The review panel now lists them, so none can overwrite a collaborator's newer version without being seen.

The failing tests were written first: scripts/offline-edit-base.test.mjs and scripts/offline-edit-base-ui.test.mjs.

Verification

  • bun test: 688 pass, 0 fail.
  • tsc -b, eslint on the changed files, and vite build all pass.
  • Full E2E locally: 18 files, 124 tests pass.

Review

Two rounds of independent review (Codex GPT-6-Sol) plus Pullfrog. Round 1 found two gaps, both fixed in 1d3ae99 with regression tests:

  • A pinned draft didn't chain through the user's own later edit when that edit was queued after the pinned edit had been acknowledged and observed. The walk now also follows an edit whose base revision equals the link's acknowledged revision.
  • Batch selection missed a row whose temp- ID had just been replaced by its server ID. It now uses matchesItemId.

Round 2 and Pullfrog's re-review found nothing blocking. A further adversarial review found two regressions, both reproduced against main and fixed in a later commit with tests:

  • A draft could chain through an own edit already rejected as a conflict, which left it stuck and part of that edit's discard. It now skips conflicted edits.
  • A draft chained behind a pending own edit was reapplied unseen with it. This is now shown in review.

That review also led to routing edits through the hook (above) and fixed E2E fixture fidelity issues. Round 2 confirmed the revision-equality link doesn't skip collaborator writes, and that client and server hashes agree for assignment-projected rows.

Not covered / follow-ups

  • Long-open drafts can still self-conflict. A details draft held open past compaction (32 or more later acknowledged edits) or past a rebase of its pinned edit can still conflict with the user's own edits. It is recoverable through Review / Apply. Fixing it would mean pinning open drafts during compaction and aliasing rebased operation IDs.
  • Native (iOS/Android) reconnect is not exercised. There is no simulator or device in this environment. That is one of the issue's acceptance items.
  • Direct writes bypass the durable queue. SubItems.tsx, PriorityFocus.tsx, Home.tsx, OnboardingFlow.tsx and NoteEditor.tsx call useMutation directly. Offline, those writes live only in the Convex client's memory and are lost on reload. They can also change revisions under queued edits to the same item. This needs new queue operation types (sub-items, notes), so it is left for a follow-up.
  • Cold start with no backend is blocked. A reload while the backend is unreachable stays on "Loading..." because AuthGuard waits on getUserByTurnkeyId. Queued edits survive and replay on reconnect (the E2E covers this), but the UI can't be used until the backend is reachable. This sits at the auth boundary ([P0] Enforce authenticated identity across browser and agent operations #236).
  • Permanent server errors are retried. "Operation ID reused with different content" and validation errors are plain Errors, so they get 5 backoff retries before parking as failed. They are surfaced and never dropped.
  • Deleted items look like permission loss. A collaborator deleting an item hits authorizeResources first, so the edit is marked "permission lost" (export-only) rather than a reviewable conflict.

🤖 Generated with Claude Code

Brian and others added 2 commits October 8, 2026 00:30
…nnect

The loopback Convex fixture accepted every *Replay mutation without checking
expected revisions or predecessor receipts, so a regression of #238 (an
offline uncheck rejected after its own check replayed) passed the browser
suite. The fixture now mirrors convex/lib/replay.ts: receipts keyed by
operation ID with fingerprint checks, target/expected matching, predecessor
revisions from receipts, and REPLAY_CONFLICT errors sent as ConvexError data.

Adds per-account backend outage and lost-response controls plus a
collaborator-edit hook, and browser tests for:
- offline check then uncheck across a reload replays both, final unchecked
- a committed-but-lost acknowledgment retries with the same operation ID
  and applies once; a later edit chains through the recovered receipt
- another member's concurrent edit surfaces as a recoverable conflict and
  can be explicitly reapplied

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nflicts

Two remaining paths still made a user's own offline work come back as a
REPLAY_CONFLICT (#238):

- A details draft opened on an optimistic row pins its predecessor to that
  row's operation. If the user then made another edit to the same item
  (e.g. unchecked it in the list), the draft still expected the older
  revision and conflicted with the user's own edit. Enqueue now chains a
  pinned base through the newest of this client's own later edits that
  descend from it. A collaborator edit in between still conflicts.

- BatchOperations and ListItem's remove/fallback toggles queued without a
  snapshot. For an item not yet in the IndexedDB cache (a collaborator's new
  row tapped before the cache effect commits), the edit was recorded with
  revision 'unknown' and no list, so it was never projected and always
  rejected. They now pass the rendered rows the user acted on as the edit
  base, as the hook-based paths already do.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app

railway-app Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

🚅 Deployed to the boop-pr-287 environment in Friends

Service Status Web Updated
boop ✅ Success (View Logs) Web Oct 8, 2026 at 8:14 am UTC

@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-287 October 8, 2026 07:36 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues. One gap remains in the pinned-draft fix, details inline.

Reviewed changes

I reviewed the full diff: the replay-enforcing E2E fixture, the new browser reconnect tests, the pinned-draft predecessor chaining in enqueueMutation, and the snapshot plumbing for BatchOperations and ListItem.

  • Replay-enforcing fixture: tests/fixtures/server.mjs now matches convex/lib/replay.ts. It checks the fingerprint on operation ID reuse, requires the target set to match expected, takes predecessor revisions from receipts, and returns REPLAY_CONFLICT as errorData. It also adds controls for per-account outages, dropped acks and collaborator edits.
  • Browser reconnect E2E: three tests cover check then uncheck across a reload, a lost acknowledgment retried with the same operation ID, and a collaborator conflict followed by an explicit reapply.
  • Pinned draft chaining: when a draft pins its base operation, enqueueMutation now moves the predecessor forward through the client's own later edits that list that base as predecessor.
  • Rendered rows as edit base: BatchOperations now takes a required items prop, which ListView fills from useOptimisticItems. ListItem's remove action and fallback toggles now pass [item] as the snapshot, so uncached targets no longer get revision: 'unknown'.

I ran scripts/offline-edit-base.test.mjs and scripts/offline-edit-base-ui.test.mjs locally and all 5 tests pass.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using claude-opus-5-5 | 𝕏

Comment thread src/lib/offline.ts Outdated
Review follow-up. A pinned draft base only walked explicit predecessor
links. If the user's later edit to the same item was queued after the
earlier edit had been acknowledged and observed, that edit carried no
predecessor (it was based on the observed server row), and the stale draft
still conflicted with it. The walk now also follows an own later edit whose
expected revision equals the revision the current link's receipt produced:
it was based on exactly that state, so no collaborator write sits between.

BatchOperations now matches selected rows with matchesItemId, so a
selection that still holds a temp- ID keeps its row (and edit base) after
the row acquires its server ID.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-287 October 8, 2026 07:42 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. The pinned-draft gap from the last review is closed.

Reviewed changes

I reviewed commit 1d3ae99, which changed the pinned-draft chaining in enqueueMutation and the temporary-ID matching for batch selections.

  • Chaining through acked and observed edits: the forward walk in src/lib/offline.ts now also follows the client's own later edit when that edit has no predecessor and its expected.revision matches the current link's acknowledged revision. A collaborator edit in between still changes the base revision, so it still conflicts. The m.id > link.id guard makes sure the loop always terminates.
  • Temporary IDs in batch selections: BatchOperations.selectedRows now uses matchesItemId. A selection that still holds temp-<opId> therefore picks up the row after that row gets its server ID.
  • Regression tests: there is a new node test for the case where the pinned edit is acknowledged and observed first, and a UI test for a selected temporary row. I ran both offline-edit-base*.test.mjs files locally and all 7 tests pass.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

…ases

- Route row remove and batch actions through useOptimisticItems
  (removeItem, new queueBatch), so every list edit queues against the same
  server/cache base rows. This replaces passing on-screen rows from
  ListItem/BatchOperations, which split the edit-base rule across four
  places and could pin a just-discarded operation and drop a tap.
- Do not chain a draft through an own edit already rejected as a
  conflict: the draft waited behind it forever and joined its discard
  cascade (on main it applied independently).
- Conflict review now lists the later edits that will be reapplied with
  the reviewed edit (masking legacy-pending payloads), so a dependent
  draft can't overwrite a collaborator's newer version unseen.
- Extract the descendant walk into newestOwnDescendant.
- E2E fixture: fix the `path` ReferenceError in apply's default branch,
  throw resourceUnavailable-shaped errorData from getItemForSync like
  production, and notify the account's other sockets before dropping a
  committed replay response.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@railway-app
railway-app Bot temporarily deployed to Friends / boop-pr-287 October 8, 2026 08:13 Destroyed

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. Routing list edits through useOptimisticItems is cleaner than the rendered-row snapshots it replaces.

Reviewed changes

I reviewed commit b6c1221, which fixes the two regressions from the adversarial review and moves batch and row-remove edits onto the hook's base rows.

  • Conflicted edits are skipped when chaining: the pinned-draft walk now lives in newestOwnDescendant and ignores the client's own edits that are already in conflict. A draft therefore no longer waits behind a rejected edit or gets pulled into that edit's discard cascade. Since the rejected edit was never written, the draft still chains against the correct server revision.
  • Chained edits shown in conflict review: OfflineRecovery lists the rest of the root's discardCascade as "Later edits applied with it" and gives the apply button the total count. That is accurate. rebaseOperation points direct descendants' predecessors at the new operation, and SyncManager holds those descendants back until the predecessor is acknowledged.
  • Batch and remove go through the hook: BatchOperations now takes queueBatch, and ListItem/NestedListItem take onRemove. Both queue against the server or cache base rows rather than optimistic rows, so a batch edit can no longer pin itself to a stale _operationId. Every ListView call site passes the new handlers.
  • Fixture fidelity: getItemForSync now throws a FORBIDDEN errorData for missing items, as production does. QueryFailed passes errorData through. A dropped replay response now pushes the committed write to the account's other sockets before it closes the connection.

All 25 tests in scripts/offline-edit-base.test.mjs, scripts/offline-edit-base-ui.test.mjs and scripts/offline-hooks.test.mjs pass locally.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@brianorwhatever
brianorwhatever merged commit 8bfcdb7 into main Oct 8, 2026
10 checks passed

This branch was successfully deployed

No deployments
Friends / boop-pr-287 — b6c1221d Deployed Oct 8, 2026 by railway-app[bot]
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.

[P1] Prevent offline replay from discarding valid work

1 participant