Skip to content

ADE-47: CRR sync drops changes when a changeset batch splits in the middle of a single db_version#435

Merged
arul28 merged 1 commit into
mainfrom
ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version
May 30, 2026
Merged

ADE-47: CRR sync drops changes when a changeset batch splits in the middle of a single db_version#435
arul28 merged 1 commit into
mainfrom
ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented May 30, 2026

Fixes ADE-47

Summary

Describe the change.

What Changed

Key files and behaviors.

Validation

How you tested.

Risks

Anything to watch.

Linked Linear issues

ADE   Open in ADE  ·  ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch  ·  PR #435

Greptile Summary

This PR fixes ADE-47, where CRR sync silently dropped rows when a changeset batch was cut in the middle of a db_version group. It also removes the prior Math.max(...spread) call that could throw a RangeError on large db_version groups.

  • Core fix (syncHostService.ts): Extracts a new exported selectChangesetBatchChunk function that delays the chunk boundary until a db_version transition, ensuring all rows sharing one db_version are sent together. toDbVersion is now tracked directly via lastIncludedDbVersion rather than a spread into Math.max.
  • Tests (syncHostService.test.ts): Four new unit tests cover row-limit splitting, byte-limit splitting, 70k-row single-version batches, and empty-changeset advancement.
  • Docs: Two doc files receive short prose notes explaining the invariant that rows within a db_version must not be split across separate host batches.

Confidence Score: 4/5

Safe to merge; the core fix is correct and well-tested, with one unreachable fallback branch that can be cleaned up post-merge.

The chunk-selection logic correctly defers breaks to db_version boundaries and tracks the watermark directly via lastIncludedDbVersion. The only issue is a now-unreachable three-line fallback (lines 232–234) that was a safety valve in the old code but can never trigger under the new loop invariant.

apps/ade-cli/src/services/sync/syncHostService.ts — minor dead-code cleanup around the single-row fallback.

Important Files Changed

Filename Overview
apps/ade-cli/src/services/sync/syncHostService.ts Extracts chunk-selection logic into the exported selectChangesetBatchChunk function, which fixes the split-db_version bug by deferring the break to the next db_version boundary; contains one unreachable fallback branch (lines 232–234).
apps/ade-cli/src/services/sync/syncHostService.test.ts Adds four targeted unit tests for selectChangesetBatchChunk covering row-limit splitting, byte-limit splitting, the 70k-row no-spread case, and empty-changeset advancement.
docs/features/sync-and-multi-device/README.md Adds a short prose paragraph documenting the host-side batching invariant (all rows for a db_version must travel together) to the sync feature overview.
docs/features/sync-and-multi-device/crdt-model.md Adds a one-sentence note to the CRDT model doc clarifying that the transport must not split rows sharing a db_version across separate batches.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["selectChangesetBatchChunk(changes, fromDbVersion, toDbVersion, maxRows, maxBytes)"] --> B{changes empty?}
    B -- yes --> C{toDbVersion > fromDbVersion?}
    C -- no --> D["return null"]
    C -- yes --> E["return { changes: [], toDbVersion }"]
    B -- no --> F["Iterate over sorted changes"]
    F --> G{chunk.length > 0\nAND new db_version\nAND row/byte limit hit?}
    G -- yes --> H["break — db_version boundary respected"]
    G -- no --> I["push row to chunk\nupdate lastIncludedDbVersion"]
    I --> F
    H --> J["return { changes: chunk, toDbVersion: lastIncludedDbVersion }"]
    I --> J
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/ade-cli/src/services/sync/syncHostService.ts:232-236
The fallback at lines 232–234 is unreachable. The loop's break condition requires `chunk.length > 0`, so the first element of `args.changes` is unconditionally pushed on the first iteration — `chunk` can never be empty after the loop when `args.changes` is non-empty.

```suggestion
  if (chunk.length === 0 && args.toDbVersion <= args.fromDbVersion) return null;
```

Reviews (5): Last reviewed commit: "fix(sync): keep changeset batches on db ..." | Re-trigger Greptile

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 30, 2026

ADE-47

@vercel
Copy link
Copy Markdown

vercel Bot commented May 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview May 30, 2026 7:56am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

Warning

Review limit reached

@arul28, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 18 minutes and 21 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4e889335-ff20-496c-b4bc-8e3205ddc9ba

📥 Commits

Reviewing files that changed from the base of the PR and between 8dc8834 and 9dccf9e.

⛔ Files ignored due to path filters (2)
  • docs/features/sync-and-multi-device/README.md is excluded by !docs/**
  • docs/features/sync-and-multi-device/crdt-model.md is excluded by !docs/**
📒 Files selected for processing (2)
  • apps/ade-cli/src/services/sync/syncHostService.test.ts
  • apps/ade-cli/src/services/sync/syncHostService.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version

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 and usage tips.

@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 30, 2026

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

@copilot review but do not make fixes

Comment thread apps/ade-cli/src/services/sync/syncHostService.ts Outdated
@arul28 arul28 force-pushed the ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch from d87ec57 to aaa1efe Compare May 30, 2026 06:24
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

@copilot review but do not make fixes

@arul28 arul28 force-pushed the ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch from aaa1efe to cad6af6 Compare May 30, 2026 07:03
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

Addressed Greptile review comment 3328291076 by removing the unbounded Math.max spread and adding a 70,000-row same-db_version regression test. @copilot review but do not make fixes

@arul28 arul28 force-pushed the ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch from cad6af6 to 6988a73 Compare May 30, 2026 07:16
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

Rebased onto latest origin/main (df86668). @copilot review but do not make fixes

@arul28 arul28 force-pushed the ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch from 6988a73 to 9dccf9e Compare May 30, 2026 07:53
@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

Rebased onto latest origin/main (8dc8834). @copilot review but do not make fixes

@arul28 arul28 merged commit 8fff945 into main May 30, 2026
28 of 29 checks passed
@arul28 arul28 deleted the ade-47-crr-sync-drops-changes-when-a-changeset-batch-splits-in-the-middle-of-a-single-db-version branch May 30, 2026 08:19
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