Add updated_at sync cursors for incremental server sync - #273
Closed
KacperKozak wants to merge 2 commits into
Closed
Add updated_at sync cursors for incremental server sync#273KacperKozak wants to merge 2 commits into
KacperKozak wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds native-managed ChangesSync cursor propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JSStore
participant AppDataRepository
participant TelemetryDao
participant TelemetryDatabase
JSStore->>AppDataRepository: Submit board or alert input
AppDataRepository->>TelemetryDao: Stamp and persist updatedAt
TelemetryDao->>TelemetryDatabase: Update row and cursor index
TelemetryDatabase-->>AppDataRepository: Stored entity
AppDataRepository-->>JSStore: Return mapped updatedAt
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt`:
- Line 681: Update the native board-setting write transaction near the
"updatedAt" mapping so writes to lastBattery and legalMode also advance the
parent boards.updated_at cursor. Ensure the board_settings update and
parent-board timestamp update occur transactionally, preserving the existing
updatedAt value used for the returned Board fields.
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt`:
- Around line 388-396: Make all native cursor writes monotonic: in
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt:388-396,
update setAlertRuleEnabled to persist MAX(updated_at, :updatedAt); in
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt:927-937,
have the board replacement flow derive its cursor from the maximum persisted and
current timestamps; and in AppDataRepository.kt:1084-1097, apply the same
maximum-cursor logic to alert replacement before invoking the existing
upsert/replacement operation.
In `@modules/vescape-core/ios/telemetry/AppDataRepository.swift`:
- Around line 117-128: Make board and alert cursor updates monotonic across
device clock rollback by preserving the greater of the current timestamp and
each row’s existing updated_at within the same write transaction. In
modules/vescape-core/ios/telemetry/AppDataRepository.swift lines 117-128, update
the board upsert around updatedAt to retain the existing board cursor; in lines
335-360, apply the same maximum-preserving behavior to both alert full upserts
and targeted enable/disable updates.
In `@src/modules/alerts/store/alertsStore.ts`:
- Around line 15-20: Keep optimistic cursors current for all local mutations: in
src/modules/alerts/store/alertsStore.ts lines 15-20, apply withLocalCursor() to
the replacement values used by update() and setEnabled(); in
src/modules/board/store/boardStore.ts lines 89-97, assign a fresh optimistic
cursor in updateBoard() before updating state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f4332f-fb5e-4c1f-a175-933c657bd983
📒 Files selected for processing (22)
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/alerts/AlertEngine.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryBucketBuilder.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDatabase.ktmodules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryEntities.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/alerts/AlertEngineTest.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/ProfileStatsRepositoryTest.ktmodules/vescape-core/android/src/test/java/expo/modules/vescapecore/telemetry/SyncCursorMigrationTest.ktmodules/vescape-core/ios/alerts/AlertEngine.swiftmodules/vescape-core/ios/alerts/AlertEngineTests.swiftmodules/vescape-core/ios/telemetry/AppDataRepository.swiftmodules/vescape-core/ios/telemetry/SyncCursorMigrationTests.swiftmodules/vescape-core/ios/telemetry/TelemetryDao.swiftmodules/vescape-core/ios/telemetry/TelemetryDatabase.swiftmodules/vescape-core/src/e2eFake.tsmodules/vescape-core/src/index.tssrc/modules/alerts/lib/customAlertRules.tssrc/modules/alerts/store/alertPresetStore.test.tssrc/modules/alerts/store/alertsStore.tssrc/modules/board/store/boardStore.test.tssrc/modules/board/store/boardStore.ts
| "alertPresetsOnboarded" to (values["alertPresetsOnboarded"] ?: false), | ||
| "legalMode" to (values["legalMode"] ?: mapOf("enabled" to false)), | ||
| "link" to link, | ||
| "updatedAt" to updatedAt, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Advance the parent board cursor for native board-setting writes.
lastBattery and legalMode are returned as Board fields, but their direct native writes only update board_settings. Without transactionally updating boards.updated_at, an incremental board scan can miss those changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt`
at line 681, Update the native board-setting write transaction near the
"updatedAt" mapping so writes to lastBattery and legalMode also advance the
parent boards.updated_at cursor. Ensure the board_settings update and
parent-board timestamp update occur transactionally, preserving the existing
updatedAt value used for the returned Board fields.
Comment on lines
+388
to
+396
| /** | ||
| * Targeted toggle. Unlike the `@Insert` upserts it never round-trips an entity, so `updated_at` | ||
| * has to move here explicitly — without it, toggling a rule leaves the sync cursor stale and the | ||
| * change never reaches the server. | ||
| * | ||
| * @parity /modules/vescape-core/ios/telemetry/AppDataRepository.swift `setAlertRuleEnabled` | ||
| */ | ||
| @Query("UPDATE alerts SET enabled = :enabled, updated_at = :updatedAt WHERE board_id = :boardId AND id = :id") | ||
| suspend fun setAlertRuleEnabled(boardId: String, id: String, enabled: Boolean, updatedAt: Long) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make all native cursor writes monotonic. Bucket merges already preserve the maximum cursor, but targeted alert updates and replacing board/alert upserts can overwrite a newer persisted timestamp after a device clock rollback.
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt#L388-L396: update withMAX(updated_at, :updatedAt).modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt#L927-L937: derive the board cursor from the maximum of persisted and current timestamps before replacing the row.modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt#L1084-L1097: derive the alert cursor from the maximum of persisted and current timestamps before replacing the row.
📍 Affects 2 files
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt#L388-L396(this comment)modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt#L927-L937modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt#L1084-L1097
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt`
around lines 388 - 396, Make all native cursor writes monotonic: in
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/TelemetryDao.kt:388-396,
update setAlertRuleEnabled to persist MAX(updated_at, :updatedAt); in
modules/vescape-core/android/src/main/java/expo/modules/vescapecore/telemetry/AppDataRepository.kt:927-937,
have the board replacement flow derive its cursor from the maximum persisted and
current timestamps; and in AppDataRepository.kt:1084-1097, apply the same
maximum-cursor logic to alert replacement before invoking the existing
upsert/replacement operation.
Comment on lines
+117
to
+128
| // One stamp for the board row's sync cursor and every board-setting row written below. Native | ||
| // stamps it rather than trusting the bridge value: it must come from the device clock that | ||
| // already writes `created_at` and must move on every upsert, including partial edits. | ||
| let updatedAt = nowMs() | ||
|
|
||
| write { db in | ||
| try db.execute( | ||
| sql: "INSERT OR REPLACE INTO boards (id, name, ble_id, transport, created_at) VALUES (?, ?, ?, ?, ?)", | ||
| arguments: [id, name, bleId, transport, createdAt] | ||
| sql: """ | ||
| INSERT OR REPLACE INTO boards (id, name, ble_id, transport, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| """, | ||
| arguments: [id, name, bleId, transport, createdAt, updatedAt] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep board and alert cursors monotonic across clock rollback.
A later write can set updated_at below its existing value when the device clock moves backward. Incremental sync clients that have already advanced past the old cursor can then miss that mutation. Compute max(nowMs(), existing updated_at) within the same write transaction, or use conflict updates that preserve the maximum.
modules/vescape-core/ios/telemetry/AppDataRepository.swift#L117-L128: preserve the existing board cursor when stamping an upsert.modules/vescape-core/ios/telemetry/AppDataRepository.swift#L335-L360: preserve the existing alert cursor for both full upserts and targeted enable/disable updates.
📍 Affects 1 file
modules/vescape-core/ios/telemetry/AppDataRepository.swift#L117-L128(this comment)modules/vescape-core/ios/telemetry/AppDataRepository.swift#L335-L360
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/vescape-core/ios/telemetry/AppDataRepository.swift` around lines 117
- 128, Make board and alert cursor updates monotonic across device clock
rollback by preserving the greater of the current timestamp and each row’s
existing updated_at within the same write transaction. In
modules/vescape-core/ios/telemetry/AppDataRepository.swift lines 117-128, update
the board upsert around updatedAt to retain the existing board cursor; in lines
335-360, apply the same maximum-preserving behavior to both alert full upserts
and targeted enable/disable updates.
Comment on lines
+15
to
+20
| /** | ||
| * Native owns `updatedAt` (the incremental-sync cursor) and stamps it from its own clock. Rules | ||
| * mirrored into local state before that write lands carry this optimistic value until the next | ||
| * `load()` replaces them with the persisted rows. | ||
| */ | ||
| const withLocalCursor = (rule: AlertRuleInput): AlertRule => ({ ...rule, updatedAt: Date.now() }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep optimistic cursors current for every local mutation. Creation and explicit upsert stamp local cursors, but normal edit paths preserve stale values until a reload.
src/modules/alerts/store/alertsStore.ts#L15-L20: usewithLocalCursor()for optimisticupdate()andsetEnabled()replacements.src/modules/board/store/boardStore.ts#L89-L97: stamp a fresh optimistic cursor inupdateBoard()before updating state.
📍 Affects 2 files
src/modules/alerts/store/alertsStore.ts#L15-L20(this comment)src/modules/board/store/boardStore.ts#L89-L97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/alerts/store/alertsStore.ts` around lines 15 - 20, Keep
optimistic cursors current for all local mutations: in
src/modules/alerts/store/alertsStore.ts lines 15-20, apply withLocalCursor() to
the replacement values used by update() and setEnabled(); in
src/modules/board/store/boardStore.ts lines 89-97, assign a fresh optimistic
cursor in updateBoard() before updating state.
This was referenced Jul 26, 2026
KacperKozak
added
area:core
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
updated_atmonotonic cursor column toboards,alerts, andtelemetry_minute_bucketson both platforms, so the client can answer "everything changed since T" for the incremental sync landing invescape-server.Every other mutable table already had one (
app_settings,board_settings,tune_profiles,privacy_zones,map_points;board_warningshaslast_detected_at). Boards and alerts hadcreated_atonly, so a board rename or an alert toggle was invisible to sync. Minute buckets had no cursor at all despite being append-and-merge targets. No tombstones/soft-delete here — tracked separately.Room schema 27 -> 28 (
MIGRATION_27_28) and GRDBv28_sync_cursors, both additive, both backfilling existing rows rather than leaving them atDEFAULT 0:boardscreated_atalertscreated_attelemetry_minute_bucketslast_sample_at_msBuckets have no
created_at;last_sample_at_msis the closest record of last change. All three get an index onupdated_atsince cursor sync scans on it.Native stamps the cursor from its own clock on every write, never trusting the bridge value. That includes
setAlertRuleEnabledon both platforms — a targetedUPDATErather than a row rewrite, and the specific regression this change exists to prevent. Bucket merges useMAX(existing, incoming)so a backwards clock step can never walk the cursor back.On the TS side
BoardandAlertRulegainupdatedAt: number, andupsertBoard/upsertAlertRulenow takeBoardInput/AlertRuleInput(Omit<…, 'updatedAt'>) so no call site fabricates a cursor value — the types encode that native owns it.Verification
bun run ts, 534 bun tests, 482 Android tests, lint (0 errors), format, and knip all pass.Implementation notes
bun run test:iosis already red ondev, before this branch.replay/ConfigReplayHarness.swiftfails to resolveConfigSafetyValues,ConfigRWController, andVescPacketReassembler. Verified identical output with this branch's changes stashed. Untouched here.Package.swiftexcludes every GRDB file (TelemetryDatabase.swift,AppDataRepository.swift,TelemetryDao.swift) from the SPM target — GRDB arrives via Pods. The existing GRDB tests (TuneProfileStoreTests,BoardWarningRegistryTests,AppDataRepositorySettingsTests) are likewise absent from the test target.SyncCursorMigrationTests.swiftsits alongside those peers and runs wherever they do. It is syntax-checked viaswiftc -parsebut has not been type-checked or executed.setAlertRuleEnabledassertion reads DAO source text, not executed SQL. Room's@Queryis BINARY-retention (invisible to reflection) and the generated impl keeps the statement method-local. There is no schema export and noandroidTestsource set, so the migration tests follow the existingProxy-over-SupportSQLiteDatabasestyle (seeMapPointEntityTest). A realMigrationTestHelperrun would need that harness wired up first.AppDataRepository(iOS) gained adbWriterseam viaforTesting(dbWriter:), mirroringTuneProfileStore(dbWriter:)/BoardWarningStore(dbWriter:). It was a pool-bound singleton with no way to point at an in-memory database.TelemetryMinuteBucketinterface deliberately does not gainupdatedAt. It is a presentation projection (converted units, derivedid,boundaryBefore), not a row mirror, and the server reads the native DB rather than that shape. Easy to add if sync ends up routed through JS.Summary by CodeRabbit
Review outcome
Reviewed by Codex (
/rr-codex). Two Medium findings, both resolved against thevescape-serversync contract.Finding 1 — cursor does not advance across a backwards clock step. Real, and it affects all three tables (boards/alerts regress below the watermark; buckets freeze). Not fixed here, deliberately. The obvious fix — a ratcheting logical clock
max(existing + 1, now)— is actively wrong:updated_atis the server's last-write-wins key, and a ratcheting counter never comes back down, so one clock rewind would permanently bias every future conflict for that device.updated_attherefore stays a truthful wall clock.The bucket
MAX(existing, now)fold is kept: it is bounded and self-correcting (truthful again once the clock passes the old value), and with the server's recommended>=watermark query a frozen stamp is still picked up. The comments on both platforms were reworded — they previously claimed a monotonicity guarantee the code does not deliver. Tracked in #275, where the preferred fix is a separate strictly-monotonic local sequence column, since the Sync Cursor is client-held and never crosses the wire.Finding 2 — backfill hides pre-upgrade changes. Rejected. It requires a server watermark newer than
created_at, which cannot exist: the cursor is client-held, so a client upgrading to schema 28 starts from watermark 0 and sends everything — backfill cannot affect completeness. The server side independently confirmedcreated_atis the right conservative choice for boards/alerts (a migrating device failing to clobber other devices is the better failure), and calledlast_sample_at_msthe genuinely truthful analogue for buckets.Follow-ups
device_id(a mutable BLE id), not a stable board id; likely wrong for sync independently of this change.