use granular state sync events for changes instead of thread messages refresh#69
use granular state sync events for changes instead of thread messages refresh#69KishanBagaria wants to merge 46 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShift incremental sync from chat-level to message-level: add batched DB lookups and rowID-keyed message mapping, detect per-message new/read/edited flags, parse typed associated/reaction data, emit message-level ServerEvent upsert/update/delete; JS revives numeric timestamp/seen fields into Date objects. ChangesMessage-Level State Sync
Sequence Diagram(s)sequenceDiagram
participant Watcher as EventWatcher
participant DB as IMDatabase
participant API as PlatformAPI
participant Sender as ServerEventSender
participant Client as JS Client
Watcher->>DB: messages(newerThanRowID:/orReadSince:/orEditedSince:)
DB-->>Watcher: UpdatedMessagesQueryResult (rows + latest maxima)
Watcher->>API: mapAndHashMessagesByRowID(db, msgRows, currentUserID, accountID)
API-->>Watcher: [rowID: [Message]]
Watcher->>Watcher: build upserts / patches / deletes (dedupe/merge)
Watcher->>Sender: emit ServerEvent.upsert/update/deleteMessages
Sender-->>Client: state_sync JSON
Client->>Client: reviveSwiftMessageAPIValue(event)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (2)
src/swift-json.test.ts (1)
4-44: ⚡ Quick winAdd assertions for the newly added date fields.
Current tests don’t validate
createdAt,mutedUntil, andlastActive, which are now part of revival logic.Proposed diff
it('converts event date fields in already-parsed Swift values', () => { const revived = reviveSwiftMessageAPIValue({ entries: [{ id: 'message-id', + createdAt: 5, + mutedUntil: 6, + lastActive: 7, timestamp: 1, editedTimestamp: 2, seen: { alice: 3, bob: true, @@ expect(revived.entries[0].timestamp).toEqual(new Date(1)) expect(revived.entries[0].editedTimestamp).toEqual(new Date(2)) + expect(revived.entries[0].createdAt).toEqual(new Date(5)) + expect(revived.entries[0].mutedUntil).toEqual(new Date(6)) + expect(revived.entries[0].lastActive).toEqual(new Date(7)) expect(revived.entries[0].seen).toEqual({ alice: new Date(3), bob: true, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/swift-json.test.ts` around lines 4 - 44, Add assertions that the new date fields are correctly revived: in the parseSwiftMessageAPIJSON test assert parsed.createdAt and parsed.mutedUntil and parsed.lastActive are Date objects equal to new Date(<input>) (or remain primitives where appropriate), and in the reviveSwiftMessageAPIValue test assert revived.entries[0].createdAt, revived.entries[0].mutedUntil, and revived.entries[0].lastActive are converted to Date instances (e.g., new Date(…)) or preserve non-numeric values as expected; update the sample inputs in both tests to include numeric values for createdAt/mutedUntil/lastActive so the assertions validate the revival logic in parseSwiftMessageAPIJSON and reviveSwiftMessageAPIValue.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (1)
141-154: ⚡ Quick winMake bulk GUID fetch order deterministic.
Line 149 uses
IN (...)withoutORDER BY, so row order is undefined. Since Line 138 takes.first, this can become non-deterministic on edge datasets. Add an explicit order.Proposed diff
func mappedMessageRows(guids: [String]) throws -> [MappedMessageRow] { guard !guids.isEmpty else { return [] } let messageColumns = try tableColumns("message") @@ let sql = """ SELECT \(messageSelectionSQL(messageColumns: messageColumns)) FROM message AS m \(messageJoins) WHERE m.guid IN (\(placeholders(count: guids.count))) + ORDER BY m.ROWID ASC """ let statement = try Statement.prepare(escapedSQL: sql, for: database) try statement.bind(guids.map { $0 as any SQLiteBindable }) return try statement.mapRowsUntilDone(MappedMessageRow.self) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift around lines 141 - 154, The query in mappedMessageRows(guids:) uses WHERE m.guid IN (...) which returns rows in an undefined order; to make results deterministic and match the input guids, append an ORDER BY that preserves the input guids order (e.g. "ORDER BY CASE m.guid WHEN 'guid1' THEN 1 WHEN 'guid2' THEN 2 ... ELSE N END" built from the guids array) instead of relying on unspecified ordering; update the SQL string construction (the sql local), ensure the CASE entries are generated for each guids element, and adjust the parameters bound to the Statement (used in try statement.bind(...)) so the guid values are provided both for the IN clause and, if you use placeholders in the CASE, for those placeholders as well, then return the mapped rows as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift:
- Around line 108-110: The code appends raw replyToGUID into deletesByThreadID
which leaks internal GUIDs and breaks delete semantics; instead map/hash
replyToGUID using the same GUID→public ID path used when creating message.id and
append that mapped ID. Replace the direct append in EventWatcher+Updates (the
block referencing replyToGUID, deletesByThreadID, and hashedThreadID(for:)) to
call the existing mapping/hashing utility used by normal message mapping (the
function or logic that sets message.id) and use its result when appending delete
IDs.
- Around line 43-50: The code advances updatesCursor to newCursor immediately
after computing MessageUpdatesCursor (constructed with
lastRowID/lastDateRead/lastDateEdited), which causes lost updates if
sender(events) later fails; instead remove the assignment updatesCursor =
newCursor from this computation path and only set updatesCursor = newCursor
after the send completes successfully (i.e., in the path that performs try await
sender(events) in EventWatcher/EventWatcher+Updates send logic), so that
updatesCursor is committed only on successful delivery; update references in
MessageUpdatesCursor creation and the send flow to assign the cursor post-sender
success and ensure the error/catch path does not mutate updatesCursor.
---
Nitpick comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift:
- Around line 141-154: The query in mappedMessageRows(guids:) uses WHERE m.guid
IN (...) which returns rows in an undefined order; to make results deterministic
and match the input guids, append an ORDER BY that preserves the input guids
order (e.g. "ORDER BY CASE m.guid WHEN 'guid1' THEN 1 WHEN 'guid2' THEN 2 ...
ELSE N END" built from the guids array) instead of relying on unspecified
ordering; update the SQL string construction (the sql local), ensure the CASE
entries are generated for each guids element, and adjust the parameters bound to
the Statement (used in try statement.bind(...)) so the guid values are provided
both for the IN clause and, if you use placeholders in the CASE, for those
placeholders as well, then return the mapped rows as before.
In `@src/swift-json.test.ts`:
- Around line 4-44: Add assertions that the new date fields are correctly
revived: in the parseSwiftMessageAPIJSON test assert parsed.createdAt and
parsed.mutedUntil and parsed.lastActive are Date objects equal to new
Date(<input>) (or remain primitives where appropriate), and in the
reviveSwiftMessageAPIValue test assert revived.entries[0].createdAt,
revived.entries[0].mutedUntil, and revived.entries[0].lastActive are converted
to Date instances (e.g., new Date(…)) or preserve non-numeric values as
expected; update the sample inputs in both tests to include numeric values for
createdAt/mutedUntil/lastActive so the assertions validate the revival logic in
parseSwiftMessageAPIJSON and reviveSwiftMessageAPIValue.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f590679-a190-40df-ad5f-ee9c0511c664
📒 Files selected for processing (14)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swiftsrc/IMessage/Sources/IMessage/PlatformAPI.swiftsrc/IMessage/Sources/PlatformSDK/ServerEvent.swiftsrc/api.tssrc/swift-json.test.tssrc/swift-json.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🧰 Additional context used
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swift
[Warning] 32-32: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 38-38: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift
[Warning] 53-53: Function should have complexity 12 or less; currently complexity is 17
(cyclomatic_complexity)
[Warning] 53-53: Function body should span 80 lines or less excluding comments and whitespace: currently spans 101 lines
(function_body_length)
[Warning] 295-295: Prefer not to use extension access modifiers
(no_extension_access_modifier)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift
[Warning] 90-90: Magic numbers should be replaced by named constants
(no_magic_numbers)
🔇 Additional comments (12)
src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift (1)
122-124: Reply linkage mapping looks correct.
reply_to_guidis wired into both the model and row decoding path consistently.Also applies to: 172-172
src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swift (1)
31-31: Dictionary bridge remains in sync with the new message field.The
reply_to_guidvalue is now correctly round-tripped in both directions.Also applies to: 73-73
src/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swift (1)
11-34: Shared associated-message parsing looks solid.Nice consolidation: the parser and
reactionPartshelper centralize behavior used across mapping paths.Also applies to: 36-42
src/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swift (1)
26-26: Good move to shared linked-message parsing.Using
parseAssociatedMessageTargethere reduces drift across reaction/association code paths.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (1)
252-252: Nice addition of explicit reaction-row ordering.The
ORDER BY m.ROWID ASCmakes reaction processing deterministic.src/IMessage/Sources/PlatformSDK/ServerEvent.swift (1)
38-46: Message-level state sync serialization is cleanly structured.The new cases and shared helper keep payload construction consistent across upsert/update/delete.
Also applies to: 102-119, 123-131
src/swift-json.ts (1)
1-8: Nested Swift payload revival implementation looks good.The new recursive helper and
seenobject handling match the intended event-shape normalization.Also applies to: 10-34
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift (1)
43-133: Message-level update query/result flow looks solid.Good change moving to per-message deltas with monotonic cursor maxima computation in one pass.
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher.swift (1)
25-45: Initializer context wiring is consistent.Capturing both
currentUserIDandaccountIDat construction aligns with the new message mapping flow.src/api.ts (1)
110-115: Good event normalization before forwarding.Reviving non-TOAST events before
onEventis the right place to enforce Swift JSON value restoration.src/IMessage/Sources/IMessage/PlatformAPI.swift (1)
120-124:accountIDpropagation into lifecycle is correctly wired.This matches the updated lifecycle/watcher contract and keeps event mapping context consistent.
src/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swift (1)
92-105: Lifecycle guard + constructor handoff look correct.Requiring
accountIDbeforestartWatchingis a good safety check for the new watcher contract.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift (2)
37-44:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCommit
updatesCursoronly after event delivery succeeds.This function advances
updatesCursorbefore the caller has actually deliveredevents. If delivery fails afterward, the next poll starts past these rows and the updates are lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift around lines 37 - 44, Currently the code constructs MessageUpdatesCursor (newCursor) and assigns it to updatesCursor before returning events, which loses updates if delivery fails; change this so you compute newCursor but do not set the shared updatesCursor inside this method. Instead return the newCursor alongside events (e.g., change the method to return (events, newCursor) or a wrapper type) or expose a commit method (e.g., commitMessageUpdatesCursor(_:)) that the caller invokes after successful delivery; update usages accordingly and keep traceMessageUpdates logging but only set updatesCursor when the caller confirms delivery.
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap
replyToGUIDthrough the public message-ID path before deleting.Line 92 bypasses the normal message mapping path and emits a DB GUID directly into
deleteMessages. That can diverge from the ID format used when the hidden reaction-add message was originally upserted, and it also leaks internal IDs upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift around lines 89 - 93, Currently the code appends the internal replyToGUID directly into deletesByThreadID which bypasses the public message-ID mapping and can leak internal DB GUIDs; instead, pass replyToGUID through the existing public message-ID conversion used elsewhere (the same mapper used when upserting hidden reaction-add messages) and append that public ID to deletesByThreadID (keep the Hasher.thread.tokenizeRemembering(originalThreadID) usage for the thread key). Replace the direct use of replyToGUID with the mapped/public ID before adding to deletesByThreadID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift:
- Around line 143-149: The chunked path in mappedMessageRows(guids:) can
re-query duplicate GUIDs across chunks causing duplicate rows; before checking
guids.count <= maxMappedMessageRowsBatchSize or calling guids.chunks(...)
normalize the input by deduplicating the guids array (e.g., create a unique
list/preserve order) and then use that deduped list for the size check and
subsequent chunked calls to mappedMessageRows(guids: Array(chunk)) so both
chunked and non-chunked paths behave consistently.
In `@src/IMessage/Sources/IMessage/Mappers/MessageMapper`+Associated.swift:
- Around line 120-127: The SMS tapback fallback is being hidden and converted
into a structured reaction; update the block that sets message.action and
message.isHidden so it only assigns a structured action and hides the message
when the message is an actionable reaction (i.e., message.isAction is true);
otherwise (SMS fallback) do not set message.action, leave message.isHidden
false, and keep the visible fallback text (constructed with msgRow.isFromMe and
summaryInfo.string("ams")) so SMS reactions render as visible "Liked …" style
messages instead of hidden metadata.
---
Duplicate comments:
In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift:
- Around line 37-44: Currently the code constructs MessageUpdatesCursor
(newCursor) and assigns it to updatesCursor before returning events, which loses
updates if delivery fails; change this so you compute newCursor but do not set
the shared updatesCursor inside this method. Instead return the newCursor
alongside events (e.g., change the method to return (events, newCursor) or a
wrapper type) or expose a commit method (e.g., commitMessageUpdatesCursor(_:))
that the caller invokes after successful delivery; update usages accordingly and
keep traceMessageUpdates logging but only set updatesCursor when the caller
confirms delivery.
- Around line 89-93: Currently the code appends the internal replyToGUID
directly into deletesByThreadID which bypasses the public message-ID mapping and
can leak internal DB GUIDs; instead, pass replyToGUID through the existing
public message-ID conversion used elsewhere (the same mapper used when upserting
hidden reaction-add messages) and append that public ID to deletesByThreadID
(keep the Hasher.thread.tokenizeRemembering(originalThreadID) usage for the
thread key). Replace the direct use of replyToGUID with the mapped/public ID
before adding to deletesByThreadID.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d87dfa75-356f-4791-b016-2d25e8e17854
📒 Files selected for processing (8)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swiftsrc/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swiftsrc/IMessage/Sources/IMessageCore/Array+Chunks.swiftsrc/swift-json.test.tssrc/swift-json.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/swift-json.test.ts
- src/swift-json.ts
📜 Review details
🧰 Additional context used
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMessageCore/Array+Chunks.swift
[Warning] 1-1: Prefer not to use extension access modifiers
(no_extension_access_modifier)
src/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swift
[Warning] 32-32: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 165-165: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 166-166: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 167-167: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 168-168: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 169-169: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 170-170: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 171-171: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 172-172: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 173-173: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 174-174: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 175-175: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 176-176: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 177-177: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 178-178: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 179-179: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 180-180: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 181-181: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 182-182: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift
[Warning] 47-47: Function should have complexity 12 or less; currently complexity is 18
(cyclomatic_complexity)
[Warning] 47-47: Function body should span 80 lines or less excluding comments and whitespace: currently spans 89 lines
(function_body_length)
🔇 Additional comments (2)
src/IMessage/Sources/IMessageCore/Array+Chunks.swift (1)
2-6: Chunking implementation looks correct and safe.
chunks(ofCount:)handles invalid sizes (Line 3) and correctly bounds the last slice (Line 5).src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (1)
266-266: Deterministic reaction row ordering is a good improvement.Adding
ORDER BY m.ROWID ASCon Line 266 helps keep reaction processing stable across runs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/swift-json.ts (1)
39-41: ⚡ Quick winMake array handling recurse through full event envelopes.
On Line 39, array elements are treated as flat entries only. If callers pass an array of event envelopes, nested
entries/presencewon’t be revived in this branch.Proposed diff
if (Array.isArray(value)) { - value.forEach(reviveSwiftEventEntry) + value.forEach(item => { + reviveSwiftMessageAPIValue(item) + }) return value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/swift-json.ts` around lines 39 - 41, The array branch in revive logic treats each item as a flat entry by calling reviveSwiftEventEntry on every element, so if callers pass full event envelopes those nested entries/presence fields are never revived; update the Array.isArray branch to iterate items and for each item call reviveSwiftEventEnvelope when the object looks like a full envelope (e.g., has entries or presence keys) and otherwise call reviveSwiftEventEntry, ensuring nested entries/presence are processed by reviveSwiftEventEnvelope (refer to reviveSwiftEventEntry and reviveSwiftEventEnvelope to locate the relevant logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/swift-json.ts`:
- Around line 39-41: The array branch in revive logic treats each item as a flat
entry by calling reviveSwiftEventEntry on every element, so if callers pass full
event envelopes those nested entries/presence fields are never revived; update
the Array.isArray branch to iterate items and for each item call
reviveSwiftEventEnvelope when the object looks like a full envelope (e.g., has
entries or presence keys) and otherwise call reviveSwiftEventEntry, ensuring
nested entries/presence are processed by reviveSwiftEventEnvelope (refer to
reviveSwiftEventEntry and reviveSwiftEventEnvelope to locate the relevant
logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9024aeaa-44ab-4542-b18b-eac23efd02f5
📒 Files selected for processing (3)
src/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swiftsrc/swift-json.tstodos.md
✅ Files skipped from review due to trivial changes (1)
- todos.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-03T17:00:19.662Z
Learnt from: KishanBagaria
Repo: beeper/platform-imessage PR: 69
File: src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift:89-93
Timestamp: 2026-05-03T17:00:19.662Z
Learning: In the beeper/platform-imessage Swift codebase, keep message IDs (`PlatformSDK.MessageID`) as raw GUIDs. When mapping from DB/event rows to `message.id`, set the ID directly from `msgRow.guid` (no GUID→public-ID hashing or transformation). For multi-part messages, append the part index as `_<part.index>` to the GUID-derived ID. During code review, if changes touch message ID creation/mapping, ensure this raw GUID + optional `_<part.index>` suffix behavior is preserved.
Applied to files:
src/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swift
🔇 Additional comments (2)
src/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swift (1)
40-76: LGTM — the new rowID lookup matches the caller’s needs.This refactor keeps
mapAndHashMessagesbehavior intact while exposing the rowID-keyed helper expected byPlatformAPI.swift. I don’t see any correctness issues here, and the raw GUID message-ID behavior remains unchanged.src/swift-json.ts (1)
13-22: Good key-aware revival forseenparticipant timestamps.The
key === 'seen'handling on Line 15 correctly revives numeric per-participant values without touching non-numeric leaves. Nice targeted fix.
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the coarse thread_messages_refresh event (which forced consumers to refetch an entire thread) with granular state_sync events for message upserts/updates/deletes and message reaction upserts/deletes, based on incremental tracking of message table changes in chat.db.
Changes:
- Introduces message- and reaction-scoped
state_syncserver events and updates the event watcher to emit those instead of thread-wide refreshes. - Adds a DB-level
MessageUpdatesCursorsnapshot + incremental “messages since cursor” query to drive update detection. - Updates JSON date revival on the Node side for Swift-bridge payloads, with accompanying tests.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| todos.md | Marks the granular state-sync migration tasks as completed. |
| src/swift-json.ts | Expands Swift JSON date-field revival (including nested seen maps) and adds an in-place reviver for already-parsed bridge objects. |
| src/swift-json.test.ts | Adds unit tests covering JSON parsing revival and in-place revival mutation behavior. |
| src/IMessage/Sources/PlatformSDK/ServerEvent.swift | Adds new message/message_reaction state_sync event cases and JSON encoding helpers; deprecates thread_messages_refresh. |
| src/IMessage/Sources/IMessageTests/HashingTests.swift | Adds a test verifying thread-ID tokenization/recovery behavior. |
| src/IMessage/Sources/IMessageCore/Array+Chunks.swift | Adds a chunking helper used for batching large IN (...) queries. |
| src/IMessage/Sources/IMessage/PlatformAPI.swift | Switches event watcher startup to use the new cursor snapshot + current user ID; refactors latest-message mapping to reuse new helpers. |
| src/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swift | Refactors associated-message parsing (incl. part IDs) and reaction typing into structured enums/types. |
| src/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swift | Refactors reaction/action mapping to use new associated-message parsing + shared helpers. |
| src/IMessage/Sources/IMessage/Mappers/MessageMapper.swift | Uses shared sender-ID helper for consistent sender resolution. |
| src/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swift | Adds mapAndHashMessagesByRowID and factors out reaction hashing. |
| src/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swift | Carries accountID into the watcher and updates startup to pass cursor/current user context. |
| src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift | Replaces thread refresh emission with granular message/reaction state-sync batching and patch deduplication. |
| src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Unreads.swift | Switches unread-state tracking keys to chat GUID strings. |
| src/IMessage/Sources/IMessage/EventWatcher/EventWatcher.swift | Stores currentUserID/accountID in the watcher and updates chat-state storage type. |
| src/IMessage/Sources/IMessage/EventWatcher/ChatRef+Description.swift | Removes the old ChatRef hashed description helper. |
| src/IMessage/Sources/IMDatabaseTestBench/TestBench.swift | Updates test-bench unread state handling to use new chatStates() return type. |
| src/IMessage/Sources/IMDatabase/Support/ChatRef.swift | Removes ChatRef from IMDatabase support types. |
| src/IMessage/Sources/IMDatabase/Models/MessageUpdatesCursor.swift | Introduces a shared cursor model for incremental message update tracking. |
| src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swift | Adds reply_to_guid bridging for mapped message rows. |
| src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift | Adds replyToGUID to MappedMessageRow for reaction-action chaining. |
| src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift | Replaces “updated chats” query with “updated messages” query + cursor advancement. |
| src/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swift | Changes unread-state query/return type to be keyed by chat GUID. |
| src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift | Adds cursor snapshot query; adds batched mappedMessageRows by GUID/ROWID with chunking. |
| src/api.ts | Revives date fields on Swift-bridge events before handing them to the JS consumer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| chatStates.removeValue(forKey: chatGUID) | ||
| log.info("chat \(chatGUID) was deleted from iMessage") | ||
| return Hasher.thread.tokenizeRemembering(pii: chatGUID) |
| private let updatedMessagesSinceQuery = """ | ||
| SELECT | ||
| m.ROWID, | ||
| m.date_read, | ||
| m.date_edited, | ||
| c.ROWID, | ||
| c.guid | ||
| FROM | ||
| message m | ||
| LEFT JOIN chat_message_join cmj ON cmj.message_id = m.ROWID | ||
| LEFT JOIN chat c ON cmj.chat_id = c.ROWID | ||
| WHERE | ||
| m.ROWID > ? OR m.date_read > ? OR m.date_edited > ? | ||
| GROUP BY | ||
| c.guid | ||
| ORDER BY | ||
| date DESC | ||
| m.ROWID ASC | ||
| """ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e9cc06001
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard replyToGUID != target.messageGUID, | ||
| replyToGUID != target.messageID else { | ||
| return nil | ||
| } | ||
| return replyToGUID |
There was a problem hiding this comment.
reply_to_guid before deciding message deletes
previousReactionActionMessageID compares reply_to_guid directly to normalized target IDs and then returns the raw value as the delete ID. Because associated-message IDs are normalized elsewhere (via parseAssociatedMessageTarget) from prefixed forms like p:<part>/... or bp:..., a prefixed reply_to_guid that still points at the original target can be misclassified as a prior hidden action row and produce a bad deleteMessages ID. In that case reaction-action cleanup is inconsistent (either deleting the wrong logical target or failing to delete the intended prior hidden row).
Useful? React with 👍 / 👎.
| const reviveSwiftEventEntry = (entry: unknown): void => { | ||
| if (isMutableRecord(entry)) reviveSwiftDateFields(entry) |
There was a problem hiding this comment.
The new bridge path revives dates for event envelopes and top-level entry fields only, but message upsert/update entries can contain nested timestamp-bearing objects (for example tweet/quoted tweet payloads). Those nested timestamps now stay as numbers instead of Date, unlike the JSON parse path that used recursive reviving, so consumers can receive mixed runtime types for the same message schema depending on delivery path (initial fetch vs. live state sync).
Useful? React with 👍 / 👎.
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| log.error("message update row \(change.rowID) couldn't be mapped, dropping") | ||
| continue | ||
| } | ||
| let threadID = msgRow.threadID ?? change.chatGUID |
There was a problem hiding this comment.
Multi-chat messages route to only one chat. msgRow.threadID is non-nil for any non-orphaned message, so the ?? change.chatGUID fallback never fires. For a message in chats A and B, db.messages(since:) correctly returns two UpdatedMessageChanges (chatGUID=A and chatGUID=B), but mappedMessageRows(rowIDs:) LEFT JOINs chat_message_join and the dedup at line 55 (uniquingKeysWith: { first, _ in first }) keeps only one chat's threadID. Both loop iterations then write to pendingByThreadID[A][rowID], with the second overwriting the first — chat B is never created in pendingByThreadID/batchesByThreadID and gets no event.
The old code's GROUP BY c.guid made refreshMessagesInThread events fire for every affected chat. This is a regression for the iMessage+SMS dual-service case (very common) and after group-ID changes.
Reactions are worse: batch.reactionUpsertsByMessageID[target.messageID, default: []].append(...) runs twice in the dedup-winner chat (once per change row), so chat A receives a duplicated reaction event while chat B receives nothing.
Probably want let threadID = change.chatGUID here; the message body's own threadID may also need overriding so each per-chat batch carries the right value when the same rowID is appended to multiple batches.
| // For fresh reactions, `reply_to_guid` can point at the original target | ||
| // message. Only delete when it points at a prior hidden reaction/removal | ||
| // action row. | ||
| guard replyToGUID != target.messageGUID, |
There was a problem hiding this comment.
replyToGUID is not normalized before comparison. parseAssociatedMessageTarget strips p:N/ and bp: prefixes from associated_message_guid, but the equality guard here compares the raw reply_to_guid against the already-stripped target.messageGUID/target.messageID. Today this is fine — every reply_to_guid value in the existing fixtures is a bare UUID — but if iMessage ever stores a prefixed form in reply_to_guid (no schema constraint prevents it), the guards become dead and a prefixed/malformed ID would flow through to batch.deletes (and into a deleteMessages event that the renderer can't match).
Safer to normalize first:
let normalized = parseAssociatedMessageTarget(raw).messageGUID
guard normalized != target.messageGUID,
normalized != target.messageID else { return nil }
return normalized
| if change.isNew { | ||
| batch.upserts.append(contentsOf: mappedMessages) | ||
| } | ||
| if let kind = MessageUpdateKind(change) { |
There was a problem hiding this comment.
Minor: when change.isNew is true the mappedMessages are already going into batch.upserts with seen/behavior/isDelivered/isErrored set, so the follow-up .read (or .edited) update patch is redundant data on the wire. Cheap to gate on else if so the update path only runs when the message wasn't already upserted in this tick:
if change.isNew {
batch.upserts.append(contentsOf: mappedMessages)
} else if let kind = MessageUpdateKind(change) {
batch.updates.append(contentsOf: mappedMessages.compactMap { kind.patch(for: $0) })
}Not a correctness issue today, just less work for the renderer in the (uncommon but real) backdated/iCloud-synced case.
when we see a changes in the
messagetable in chat.db, we send athread_messages_refreshevent which is asking the consumers to refetch messages for the updated chat.instead let's be granular and emit a proper event saying exactly what has changed.
a change could be:
how the above changes should state synced:
message upsertfor new messagesmessage updatefor editsmessage updatefor message readsmessage updatefor undo sendsmessage_reaction upsertfor reaction,message upsertfor the reaction row ("X liked Y"), potentialmessage deletefor the last reaction rowmessage_reaction deletefor reaction,message upsertfor the reaction removal row ("X removed a like from Y"), potentialmessage deletefor the last reaction row