Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughMigrates IMDatabase from SQLite to GRDB with a schema/type-safe layer; converts many DB paths to GRDB Row APIs; shifts update detection to a message-centric pipeline and GUID-keyed unread state; enriches associated-message/reaction modeling; adds Swift JSON revival utilities, tests, and supporting packaging/utilities. ChangesSchema, GRDB Migration & Packaging
Row-based Access & Mapped Rows
Mapped SELECT Builders, Batching & Determinism
Message-Centric Updates & Event Pipeline
Watcher State, Subscription & Unreads
Associated-message & Reaction Model
Client-side Swift JSON Revival & Tests
Utilities, Tests & Misc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
pmanot
changed the title
pmanot
force-pushed
the
purav/sql-improvements
branch
from
fe0c7dd to
c0591b6
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Chats.swift (1)
10-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential inconsistency in
service_namehandling.
chat(withGUID:)usesrow.requiredString(at: 2)forservice_name, which will throw if the column is NULL. However,chats()(lines 43-44) usesrow.optionalString(at: 3) ?? "NONE"to handle nullableservice_name. Ifservice_namecan be NULL in the database,chat(withGUID:)could fail unexpectedly.Consider using the same defensive pattern:
Proposed fix
- let serviceName = Chat.ServiceName(rawValue: row.requiredString(at: 2)) + let serviceName = Chat.ServiceName(rawValue: row.optionalString(at: 2) ?? "NONE")🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Chats.swift around lines 10 - 26, chat(withGUID:) can crash if service_name is NULL because it uses row.requiredString(at: 2); change it to the same defensive pattern used in chats() by reading service_name with row.optionalString(at: 2) (or optionalString(at: 2) ?? "NONE") and then initialize serviceName via Chat.ServiceName(rawValue: serviceNameString) so a NULL DB value won’t throw — update the service_name handling in chat(withGUID:) accordingly (replace the requiredString usage and map the optional string to Chat.ServiceName).
🧹 Nitpick comments (8)
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Unreads.swift (1)
95-99: 💤 Low valueVerify PII logging is intentional.
Line 97 logs the raw
chatGUIDwhich may contain user identifiers. Confirm this is acceptable for debugging purposes, or consider using the hashed ID instead.🤖 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/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Unreads.swift around lines 95 - 99, The review flags logging of raw chatGUID in EventWatcher+Unreads.swift (inside the deletedChats.map block that builds deletedThreadIDs) as potential PII; update the log.info call to avoid emitting raw identifiers by either logging the hashed token from Hasher.thread.tokenizeRemembering(pii: chatGUID) or remove the log entirely, and if raw logging is required make it conditional behind an explicit dev/verbose flag and document that choice; ensure references to chatStates.removeValue(forKey: chatGUID), deletedThreadIDs, and Hasher.thread.tokenizeRemembering are used so you replace the raw chatGUID usage only within this mapping block.scripts/imessage-perf.mjs (1)
222-225: 💤 Low value
resolveBinfunction appears unused.The
resolveBinhelper is defined but never called in the script (onlyresolveElectronuses its logic inline). Consider removing it or using it inresolveElectron.Option: Remove or use resolveBin
Either remove
resolveBinor refactorresolveElectronto use it:function resolveElectron() { const macElectronPath = path.join( repoRoot, 'node_modules', 'electron', 'dist', 'Electron.app', 'Contents', 'MacOS', 'Electron', ) - return fsSync.existsSync(macElectronPath) ? macElectronPath : resolveBin('electron') + if (fsSync.existsSync(macElectronPath)) return macElectronPath + // Fall back to .bin/electron or global + const localBin = path.join(repoRoot, 'node_modules', '.bin', 'electron') + return fsSync.existsSync(localBin) ? localBin : 'electron' } - -function resolveBin(name) { - const localPath = path.join(repoRoot, 'node_modules', '.bin', name) - return fsSync.existsSync(localPath) ? localPath : name -}🤖 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 `@scripts/imessage-perf.mjs` around lines 222 - 225, The helper resolveBin is defined but never used; either remove this dead function or refactor resolveElectron to call resolveBin to dedupe logic. Locate the resolveBin function and either delete it, or update resolveElectron to call resolveBin(name) (or path.join(repoRoot, 'node_modules', '.bin', name') via resolveBin) so the binary resolution logic is centralized and no duplicate inline logic remains.src/IMessage/Sources/IMessagePerfBench/main.swift (1)
289-343: 💤 Low valueConsider extracting shared timing logic.
The
measureandmeasureAsyncfunctions have nearly identical structure. For a benchmark tool this is acceptable, but if you plan to extend this, extracting the common result-building logic could reduce duplication.🤖 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/IMessage/Sources/IMessagePerfBench/main.swift` around lines 289 - 343, Both measure(_:) and measureAsync(_:) duplicate the timing loop and BenchmarkResult construction; extract the shared result-building and (optionally) the timing loop to remove duplication. Add a helper like makeBenchmarkResult(name: String, resultCount: Int, samples: [Double], iterations: Int, warmups: Int) -> BenchmarkResult and replace the repeated return blocks in measure and measureAsync with a call to that helper; optionally factor the common warmup+sampling loop into a sync runSamples(operation: () throws -> Int) and an async runSamplesAsync(operation: () async throws -> Int) that return (resultCount, samples) so measure/_Async only handle invocation and call makeBenchmarkResult.src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift (1)
112-128: ⚖️ Poor tradeoffConsider adding an index hint for the OR-based WHERE clause.
The WHERE clause uses
ORacross three conditions (ROWID > ?,date_read > ?,date_edited > ?). SQLite may not efficiently use indexes for OR conditions. If performance becomes an issue on large databases, consider restructuring as a UNION or adding a composite covering index.🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Updates.swift around lines 112 - 128, The WHERE clause in updatedMessagesSinceQuery uses OR across m.\(MessageTable.Column.rowID.sqlName), m.\(MessageTable.Column.dateRead.sqlName) and the computed \(dateEditedExpression), which can prevent SQLite from using indexes efficiently; either refactor the SQL in updatedMessagesSinceQuery into a UNION of three SELECTs (one for each predicate) that preserve the same projected columns and ordering, or add a covering index on the message table that includes the involved columns (e.g. rowID, date_read, date_edited) and ensure the join to \(ChatMessageJoinTable.sqlName)/\(ChatTable.sqlName) still works with that index to improve performance.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (2)
60-73: ⚡ Quick winUse
Row.fetchOneinstead ofRow.fetchAll(...).firstfor single-row queries.The query returns at most one row (single SELECT with no FROM table, just scalar subqueries). Using
fetchAllfollowed by.firstallocates an unnecessary array.♻️ Proposed fix
- return try read { db in - try Row.fetchAll(db, sql: sql).map { row in - ( - lastRowID: row.optionalInt(at: 0) ?? 0, - lastDateRead: row.imCoreDate(at: 1) ?? Date(nanosecondsSinceReferenceDate: 0), - lastDateEdited: row.imCoreDate(at: 2) ?? Date(nanosecondsSinceReferenceDate: 0) - ) - }.first - } ?? ( + return try read { db in + guard let row = try Row.fetchOne(db, sql: sql) else { + return ( + lastRowID: 0, + lastDateRead: Date(nanosecondsSinceReferenceDate: 0), + lastDateEdited: Date(nanosecondsSinceReferenceDate: 0) + ) + } + return ( + lastRowID: row.optionalInt(at: 0) ?? 0, + lastDateRead: row.imCoreDate(at: 1) ?? Date(nanosecondsSinceReferenceDate: 0), + lastDateEdited: row.imCoreDate(at: 2) ?? Date(nanosecondsSinceReferenceDate: 0) + ) + } - lastRowID: 0, - lastDateRead: Date(nanosecondsSinceReferenceDate: 0), - lastDateEdited: Date(nanosecondsSinceReferenceDate: 0) - )🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift around lines 60 - 73, The code in the closure passed to read uses Row.fetchAll(db, sql: sql).map { ... }.first which allocates an array for a single-row query; replace that pattern with Row.fetchOne(db, sql: sql) and transform the single Row (if present) into the tuple (lastRowID, lastDateRead, lastDateEdited) directly inside the read closure (keep the same default fallback tuple used after the read). Update the usage in the same function/closure where Row.fetchAll was called so the logic still returns the same tuple defaults when fetchOne returns nil.
184-207: 💤 Low valueSorting after chunked fetch may produce incorrect ordering.
When
rowIDs.count > maxMappedMessageRowsBatchSize, the query is split into chunks, each fetched withORDER BY m.date DESC. The chunks are thenflatMap-ed and sorted again. However, if dates are equal across chunks, the sort order may differ from a single query. Consider whether this edge case matters for your use case.🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift around lines 184 - 207, The chunked fetch can yield a different order than a single query when dates tie; update mappedMessageRows to use a stable tie-breaker (ROWID) in both the SQL ORDER BY and the final Swift sort. Concretely, change the query ORDER BY to "ORDER BY m.date DESC, m.ROWID DESC" (in the SQL string built in mappedMessageRows) and adjust the final .sorted closure (the one using ($0.date ?? 0) > ($1.date ?? 0)) to compare date first and if equal compare rowID (e.g., if dates equal, compare ($0.rowID ?? 0) > ($1.rowID ?? 0)), ensuring MappedMessageRow exposes the ROWID field from messageSelectionSQL so the tie-breaker is available.src/IMessage/Sources/IMDatabase/Database/IMDatabase+Attachments.swift (1)
42-55: 💤 Low valueForce unwraps are guarded but could be cleaner with subscript assignment.
The pattern of checking
messages[messageRowID] != nilthen force-unwrapping multiple times is safe but verbose. SwiftLint flags these force unwraps. A minor restructure would eliminate the warnings and improve readability.♻️ Proposed refactor
- guard messages[messageRowID] != nil else { - assertionFailure() - continue - } - - if messages[messageRowID]!.attachments == nil { - messages[messageRowID]!.attachments = [] - } - - guard let attachment = try Attachment(row: row) else { - continue - } - - messages[messageRowID]!.attachments!.append(attachment) + guard var message = messages[messageRowID] else { + assertionFailure() + continue + } + + guard let attachment = try Attachment(row: row) else { + continue + } + + if message.attachments == nil { + message.attachments = [] + } + message.attachments?.append(attachment) + messages[messageRowID] = message🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Attachments.swift around lines 42 - 55, The current loop force-unwraps messages[messageRowID] several times; refactor to a single safe subscript read-modify-write: guard let msg = messages[messageRowID] (or use if var msg = messages[messageRowID]) then create/append the Attachment to msg.attachments without force-unwraps (e.g., if let attachment = try Attachment(row: row) { if msg.attachments == nil { msg.attachments = [attachment] } else { msg.attachments!.append(attachment) } } ) and finally assign messages[messageRowID] = msg; this eliminates repeated force-unwraps and SwiftLint warnings while preserving behavior around Attachment(row:).src/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swift (1)
175-182: 💤 Low valueConsider handling nil reaction key more explicitly.
At line 175, if
platformReactionKey(emoji:)returnsnil, the reaction is created with an emptyreactionKey. Depending on downstream handling, this could produce reactions without meaningful keys.If
nilindicates an unsupported/unknown reaction type that should be skipped, returningnilfrom this function would be more defensive:Suggested approach
func mapMessageReaction( row: any MessageReactionRowFields, reaction: AssociatedReaction, currentUserID: String, accountID: String ) -> PlatformSDK.MessageReaction? { - let reactionKey = reaction.platformReactionKey(emoji: row.associatedMessageEmoji) ?? "" + guard let reactionKey = reaction.platformReactionKey(emoji: row.associatedMessageEmoji) else { + return nil + } let participantID = messageSenderID(for: row, currentUserID: currentUserID) return PlatformSDK.MessageReaction( id: participantID, reactionKey: reactionKey,🤖 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/IMessage/Sources/IMessage/Mappers/MessageMapper`+Associated.swift around lines 175 - 182, The code currently coerces a nil platformReactionKey into an empty string which yields meaningless reactions; change the mapper (the function that builds PlatformSDK.MessageReaction) to guard let reactionKey = reaction.platformReactionKey(emoji: row.associatedMessageEmoji) and if it is nil return nil (i.e., make the mapper return an optional PlatformSDK.MessageReaction), otherwise continue using messageSenderID(for:row,currentUserID:) and reactionStickerAssetURLString(accountID:rowID:) to construct the PlatformSDK.MessageReaction with the non-empty reactionKey; update any call sites to handle the optional result (filter out nils).
🤖 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 `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Attachments.swift:
- Around line 31-38: The SQL in hydrateAttachments(for:) interpolates
messageRowIDs directly into the query (via attachmentQuerySharedPrologue +
"WHERE m.ROWID IN (...)"), which risks SQL injection and bypasses GRDB
parameterization; update hydrateAttachments to build a parameterized IN clause
(create a comma-separated list of "?" placeholders equal to messages.keys.count)
and pass messageRowIDs as StatementArguments (or as the arguments parameter to
Row.fetchAll) so Row.fetchAll(db, sql: ..., arguments: ...) is used instead of
string interpolation; reference hydrateAttachments(for:),
attachmentQuerySharedPrologue, and the Row.fetchAll call when making the change.
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Unreads.swift:
- Around line 57-65: The loop can throw if the DB returns NULL guid or
timestamp; change the decoder to guard against nullable columns instead of
calling requiredString/requiredInt directly: when iterating Row.fetchAll(db,
sql: unreadStatesQuery) use Row.isNull(at:) or optional accessors (e.g.,
optionalString/optionalInt) to skip rows with a nil chat GUID or to provide a
safe default for last_read_message_timestamp, and only call
ChatState(unreadCount:..., lastReadMessageTimestamp:...) and assign into
chatStates[chatGUID] when the GUID is non-nil (or tighten unreadStatesQuery to
include "c.guid IS NOT NULL" and COALESCE(last_read_message_timestamp, 0) to
guarantee non-nulls).
In `@src/IMessage/Sources/IMDatabaseTests/LiveSQLTests.swift`:
- Around line 32-49: Change the live-chat DB guard to be opt-in: in
requireReadable() first check an explicit env var or test switch (e.g.
ProcessInfo.processInfo.environment["ENABLE_LIVE_CHATDB_TESTS"] == "1"); if the
switch is not enabled, call XCTSkip("Live chat.db tests disabled; set
ENABLE_LIVE_CHATDB_TESTS=1 to run") (import XCTest) instead of throwing
FullDiskAccessRequired, otherwise proceed with the existing
fullDiskAccessRequest and throw as before; this keeps imDatabase() and queue()
intact while skipping the suite by default.
In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift:
- Around line 68-82: msgRowsByRowID currently collapses multiple joined rows by
rowID causing wrong thread routing; change the lookup so each
UpdatedMessageChange resolves against the row for its specific chat (use
change.chatGUID or msgRow.threadID). Build a map keyed by a compound key (rowID,
threadID/chatGUID) or a Dictionary<Int, [MessageRow]> and then select the
MessageRow whose threadID or chatGUID matches change.chatGUID when processing
queryResult.updatedMessages (refer to msgRows, msgRowsByRowID,
queryResult.updatedMessages, change.rowID, change.chatGUID, and
msgRow.threadID).
---
Outside diff comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Chats.swift:
- Around line 10-26: chat(withGUID:) can crash if service_name is NULL because
it uses row.requiredString(at: 2); change it to the same defensive pattern used
in chats() by reading service_name with row.optionalString(at: 2) (or
optionalString(at: 2) ?? "NONE") and then initialize serviceName via
Chat.ServiceName(rawValue: serviceNameString) so a NULL DB value won’t throw —
update the service_name handling in chat(withGUID:) accordingly (replace the
requiredString usage and map the optional string to Chat.ServiceName).
---
Nitpick comments:
In `@scripts/imessage-perf.mjs`:
- Around line 222-225: The helper resolveBin is defined but never used; either
remove this dead function or refactor resolveElectron to call resolveBin to
dedupe logic. Locate the resolveBin function and either delete it, or update
resolveElectron to call resolveBin(name) (or path.join(repoRoot, 'node_modules',
'.bin', name') via resolveBin) so the binary resolution logic is centralized and
no duplicate inline logic remains.
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Attachments.swift:
- Around line 42-55: The current loop force-unwraps messages[messageRowID]
several times; refactor to a single safe subscript read-modify-write: guard let
msg = messages[messageRowID] (or use if var msg = messages[messageRowID]) then
create/append the Attachment to msg.attachments without force-unwraps (e.g., if
let attachment = try Attachment(row: row) { if msg.attachments == nil {
msg.attachments = [attachment] } else { msg.attachments!.append(attachment) } }
) and finally assign messages[messageRowID] = msg; this eliminates repeated
force-unwraps and SwiftLint warnings while preserving behavior around
Attachment(row:).
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift:
- Around line 60-73: The code in the closure passed to read uses
Row.fetchAll(db, sql: sql).map { ... }.first which allocates an array for a
single-row query; replace that pattern with Row.fetchOne(db, sql: sql) and
transform the single Row (if present) into the tuple (lastRowID, lastDateRead,
lastDateEdited) directly inside the read closure (keep the same default fallback
tuple used after the read). Update the usage in the same function/closure where
Row.fetchAll was called so the logic still returns the same tuple defaults when
fetchOne returns nil.
- Around line 184-207: The chunked fetch can yield a different order than a
single query when dates tie; update mappedMessageRows to use a stable
tie-breaker (ROWID) in both the SQL ORDER BY and the final Swift sort.
Concretely, change the query ORDER BY to "ORDER BY m.date DESC, m.ROWID DESC"
(in the SQL string built in mappedMessageRows) and adjust the final .sorted
closure (the one using ($0.date ?? 0) > ($1.date ?? 0)) to compare date first
and if equal compare rowID (e.g., if dates equal, compare ($0.rowID ?? 0) >
($1.rowID ?? 0)), ensuring MappedMessageRow exposes the ROWID field from
messageSelectionSQL so the tie-breaker is available.
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Updates.swift:
- Around line 112-128: The WHERE clause in updatedMessagesSinceQuery uses OR
across m.\(MessageTable.Column.rowID.sqlName),
m.\(MessageTable.Column.dateRead.sqlName) and the computed
\(dateEditedExpression), which can prevent SQLite from using indexes
efficiently; either refactor the SQL in updatedMessagesSinceQuery into a UNION
of three SELECTs (one for each predicate) that preserve the same projected
columns and ordering, or add a covering index on the message table that includes
the involved columns (e.g. rowID, date_read, date_edited) and ensure the join to
\(ChatMessageJoinTable.sqlName)/\(ChatTable.sqlName) still works with that index
to improve performance.
In `@src/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Unreads.swift:
- Around line 95-99: The review flags logging of raw chatGUID in
EventWatcher+Unreads.swift (inside the deletedChats.map block that builds
deletedThreadIDs) as potential PII; update the log.info call to avoid emitting
raw identifiers by either logging the hashed token from
Hasher.thread.tokenizeRemembering(pii: chatGUID) or remove the log entirely, and
if raw logging is required make it conditional behind an explicit dev/verbose
flag and document that choice; ensure references to
chatStates.removeValue(forKey: chatGUID), deletedThreadIDs, and
Hasher.thread.tokenizeRemembering are used so you replace the raw chatGUID usage
only within this mapping block.
In `@src/IMessage/Sources/IMessage/Mappers/MessageMapper`+Associated.swift:
- Around line 175-182: The code currently coerces a nil platformReactionKey into
an empty string which yields meaningless reactions; change the mapper (the
function that builds PlatformSDK.MessageReaction) to guard let reactionKey =
reaction.platformReactionKey(emoji: row.associatedMessageEmoji) and if it is nil
return nil (i.e., make the mapper return an optional
PlatformSDK.MessageReaction), otherwise continue using
messageSenderID(for:row,currentUserID:) and
reactionStickerAssetURLString(accountID:rowID:) to construct the
PlatformSDK.MessageReaction with the non-empty reactionKey; update any call
sites to handle the optional result (filter out nils).
In `@src/IMessage/Sources/IMessagePerfBench/main.swift`:
- Around line 289-343: Both measure(_:) and measureAsync(_:) duplicate the
timing loop and BenchmarkResult construction; extract the shared result-building
and (optionally) the timing loop to remove duplication. Add a helper like
makeBenchmarkResult(name: String, resultCount: Int, samples: [Double],
iterations: Int, warmups: Int) -> BenchmarkResult and replace the repeated
return blocks in measure and measureAsync with a call to that helper; optionally
factor the common warmup+sampling loop into a sync runSamples(operation: ()
throws -> Int) and an async runSamplesAsync(operation: () async throws -> Int)
that return (resultCount, samples) so measure/_Async only handle invocation and
call makeBenchmarkResult.
🪄 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: faf5dd75-148a-45b1-be29-3827c5e58d6d
📒 Files selected for processing (45)
.gitignore.parity/check-swift-mapper-parity.mjs.parity/parity-child-processes.mjs.parity/parity-utils.mjsPackage.resolvedPackage.swiftpackage.jsonscripts/imessage-perf.mjssrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Accounts.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Attachments.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Chats.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedThreads.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Messages.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase.swiftsrc/IMessage/Sources/IMDatabase/Models/GUID.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swiftsrc/IMessage/Sources/IMDatabase/Schema/IMDatabaseSchema.swiftsrc/IMessage/Sources/IMDatabase/Schema/IMDatabaseTables.swiftsrc/IMessage/Sources/IMDatabase/Support/ChatRef.swiftsrc/IMessage/Sources/IMDatabase/Support/Column+.swiftsrc/IMessage/Sources/IMDatabaseTestBench/TestBench.swiftsrc/IMessage/Sources/IMDatabaseTests/LiveSQLTests.swiftsrc/IMessage/Sources/IMessage/EventWatcher/ChatRef+Description.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Unreads.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swiftsrc/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swiftsrc/IMessage/Sources/IMessage/PlatformAPI.swiftsrc/IMessage/Sources/IMessageCore/Array+Chunks.swiftsrc/IMessage/Sources/IMessagePerfBench/README.mdsrc/IMessage/Sources/IMessagePerfBench/main.swiftsrc/IMessage/Sources/PlatformSDK/ServerEvent.swiftsrc/api.tssrc/swift-json.test.tssrc/swift-json.tstodos.md
💤 Files with no reviewable changes (2)
- src/IMessage/Sources/IMessage/EventWatcher/ChatRef+Description.swift
- src/IMessage/Sources/IMDatabase/Support/ChatRef.swift
📜 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
🧠 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/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swiftsrc/IMessage/Sources/IMessageCore/Array+Chunks.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Accounts.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swiftsrc/IMessage/Sources/IMDatabaseTestBench/TestBench.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Messages.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swiftsrc/IMessage/Sources/IMessage/Hashing/PlatformAPI+Hashing.swiftsrc/IMessage/Sources/IMDatabase/Schema/IMDatabaseSchema.swiftsrc/IMessage/Sources/IMDatabase/Support/Column+.swiftsrc/IMessage/Sources/IMessagePerfBench/main.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedThreads.swiftsrc/IMessage/Sources/IMDatabase/Models/GUID.swiftsrc/IMessage/Sources/IMDatabaseTests/LiveSQLTests.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swiftsrc/IMessage/Sources/PlatformSDK/ServerEvent.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swiftsrc/IMessage/Sources/IMDatabase/Schema/IMDatabaseTables.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swiftsrc/IMessage/Sources/IMessage/PlatformAPI.swiftsrc/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Attachments.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Chats.swiftsrc/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Unreads.swift
🪛 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/IMDatabase/Database/IMDatabase+Accounts.swift
[Warning] 3-3: Prefer not to use extension access modifiers
(no_extension_access_modifier)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swift
[Warning] 75-75: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Messages.swift
[Warning] 45-45: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 78-78: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 104-104: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 105-105: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 106-106: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 107-107: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 110-110: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 113-113: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 114-114: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 115-115: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 116-116: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 117-117: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMessagePerfBench/main.swift
[Warning] 310-310: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 311-311: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 338-338: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 339-339: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 359-359: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 391-391: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 391-391: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 391-391: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 391-391: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 391-391: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 394-394: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 395-395: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 396-396: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 397-397: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 398-398: Magic numbers should be replaced by named constants
(no_magic_numbers)
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/IMDatabaseTests/LiveSQLTests.swift
[Warning] 143-143: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 170-170: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 180-180: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 202-202: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 222-222: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 275-275: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 289-289: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 339-339: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 357-357: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 379-379: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 399-399: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 468-468: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 488-488: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 518-518: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 552-552: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 643-643: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 682-682: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 707-707: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 624-624: Prefer not to use extension access modifiers
(no_extension_access_modifier)
[Warning] 636-636: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 653-653: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 672-672: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 673-673: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 674-674: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 730-730: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 802-802: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 802-802: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Error] 87-87: Struct body should span 350 lines or less excluding comments and whitespace: currently spans 484 lines
(type_body_length)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swift
[Warning] 61-61: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift
[Warning] 62-62: Function should have complexity 12 or less; currently complexity is 19
(cyclomatic_complexity)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift
[Warning] 67-67: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 78-78: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 86-86: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift
[Warning] 50-50: Prefer not to use extension access modifiers
(no_extension_access_modifier)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift
[Warning] 77-77: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 93-93: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 65-65: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 264-264: Arguments should be either on the same line, or one per line
(multiline_arguments)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Attachments.swift
[Warning] 18-18: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 47-47: Force unwrapping should be avoided
(force_unwrapping)
[Warning] 48-48: Force unwrapping should be avoided
(force_unwrapping)
[Warning] 55-55: Force unwrapping should be avoided
(force_unwrapping)
[Warning] 55-55: Force unwrapping should be avoided
(force_unwrapping)
[Warning] 67-67: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 68-68: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 69-69: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 70-70: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 71-71: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 72-72: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Chats.swift
[Warning] 11-11: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 17-17: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 54-54: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 43-43: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 44-44: Magic numbers should be replaced by named constants
(no_magic_numbers)
🔇 Additional comments (65)
.gitignore (1)
47-47: LGTM!Adding
.swiftpmto ignore the Swift Package Manager generated state directory is the correct and standard practice for Swift projects.todos.md (1)
61-65: No actionable code-review feedback.This is a project TODO status update and looks consistent with the PR scope.
src/IMessage/Sources/IMDatabase/Models/GUID.swift (1)
21-32: GRDB value conversion looks correct.The
DatabaseValueConvertibleimplementation cleanly preserves GUID-as-string storage and safely handles invalid DB values.src/IMessage/Sources/IMessage/Mappers/MessageMapperTypes.swift (2)
11-34: Associated-message target parsing and ID derivation are aligned.The new parser/messageID flow keeps IDs GUID-based and only adds a part suffix when applicable, which preserves expected message identity semantics.
Based on learnings: keepPlatformSDK.MessageIDas raw GUIDs, with optional_<part.index>for multi-part mapping.
36-99: Typed reaction modeling is a solid improvement.Moving from stringly mappings to typed reaction/action structures reduces branching ambiguity and makes downstream handling clearer.
Also applies to: 160-183
src/IMessage/Sources/IMDatabase/Schema/IMDatabaseTables.swift (1)
4-223: Schema modeling is well structured.The explicit
IMDatabaseTable/Columnenums provide a clear, type-safe foundation for GRDB query construction.package.json (1)
25-25: Perf script wiring looks good.The new
perf:imessageentry is straightforward and consistent with the documented benchmark flow.Package.resolved (1)
21-29: GRDB pin addition is clean.The lockfile update is scoped and consistent with the GRDB migration.
src/IMessage/Sources/IMessagePerfBench/README.md (1)
1-23: README is clear and practical.The benchmark intent, wrapper usage, and common command variants are documented succinctly.
src/api.ts (1)
13-13: Event revival hook is integrated correctly.Reviving non-TOAST server events before forwarding should keep downstream event payloads normalized while preserving current TOAST handling.
Also applies to: 110-115
src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows+DictionaryBridges.swift (1)
31-31: LGTM!The
replyToGUIDfield is correctly added to both the dictionary-to-model initialization and the model-to-dictionary serialization, maintaining symmetry with other optional string fields in the bridge.Also applies to: 73-73
Package.swift (1)
81-88: LGTM!The new test target
IMDatabaseTestsand performance benchmarkIMessagePerfBenchare correctly configured with appropriate dependencies. TheIMDatabasetarget now properly depends on GRDB instead of SQLite.Also applies to: 95-95, 111-120
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Accounts.swift (1)
1-12: LGTM!Clean migration to GRDB. The
read { db in ... }pattern withRow.fetchAlland column access by index is idiomatic GRDB usage for simple queries..parity/parity-child-processes.mjs (1)
111-113: LGTM!Good addition of startup error propagation. The
catchblock correctly captures initialization failures (fromcreateAPI, initial IPC, or readline setup), serializes them, and signals the parent. The parent-side handler properly deserializes and rejects thereadypromise.Also applies to: 183-186
src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Unreads.swift (1)
16-16: LGTM!The migration from
ChatRef-keyed toString-keyed dictionaries simplifies the code while preserving the same logic. The direct use ofchatGUIDfor dictionary access and hashing is cleaner.Also applies to: 21-22, 32-32, 34-34
src/swift-json.test.ts (1)
1-62: LGTM!Comprehensive test coverage for the Swift JSON revival utilities. The tests correctly verify:
- Date field conversion during JSON parsing
- Nested structure handling including mixed-type
seenmaps- In-place mutation behavior with reference equality checks
src/IMessage/Sources/IMDatabase/Database/IMDatabase.swift (2)
17-20: LGTM!Clean migration to GRDB's
DatabaseQueue. The read-only configuration for the main connection and thereadhelper method provide a good abstraction. ThemessageIndexesrefactoring to use typed columns improves type safety.Also applies to: 43-43, 46-46, 60-67
79-90: LGTM!The index creation logic correctly:
- Opens a separate writable connection (since the main one is read-only)
- Inspects the schema to check for column existence
- Creates indexes conditionally using
CREATE INDEX IF NOT EXISTSThe string interpolation in line 87 is safe since all values (
indexName,MessageTable.sqlName,column.sqlName) are compile-time constants from controlled sources.src/IMessage/Sources/IMessageCore/Array+Chunks.swift (1)
1-8: ⚡ Quick winThis suggestion cannot be implemented as swift-algorithms is not a project dependency.
The claim that
swift-algorithmsis available as an indirect dependency throughswift-async-algorithmsis incorrect. Whileswift-async-algorithmsis a direct dependency, it only depends onswift-collections, notswift-algorithms. The customchunks(ofCount:)implementation remains necessary.> Likely an incorrect or invalid review comment.src/IMessage/Sources/IMDatabaseTestBench/TestBench.swift (2)
189-194: LGTM!The update to GUID-keyed state lookup (
states[chat.guid.description]) is consistent with the PR's migration to GUID-keyed unread state snapshots fromIMDatabase.chatStates().
251-254: LGTM!The diffing logic correctly builds a
[String: ChatState]dictionary by comparing previous and new GUID-keyed state maps, aligning with the updated watcher behavior inEventWatcher+Unreads.swift..parity/check-swift-mapper-parity.mjs (3)
2-2: LGTM!The
fs/promisesimport supports the new--output-jsonfile write feature, and the reference binary path update toSwiftServer.nodealigns with the newer Swift reference runtime.Also applies to: 35-35
233-260: LGTM!The try/catch blocks around child-role execution and
ensureReferenceAPIprovide consistent error handling with proper stack traces and exit codes.
517-546: LGTM!The summary output refactor cleanly separates the JSON serialization from output destination, enabling flexible
--output-jsonfile writes and--no-stdout-jsonsuppression.src/IMessage/Sources/IMessagePerfBench/main.swift (4)
1-93: LGTM!Clean benchmark infrastructure with well-defined
Encodableresult types and proper error handling viaBenchError.
127-188: LGTM!The
run()method properly validates options before execution and cleanly separates SQL vs API benchmark paths based on flags.
261-287: LGTM!The API benchmarks properly handle
PlatformAPIlifecycle with disposal in both success and error paths usingtry? await api.dispose().
346-407: LGTM!The helper functions for tilde expansion, statistics calculation, and pretty printing are straightforward and correct. The magic numbers for percentiles (0.50, 0.95) and column widths are standard benchmark conventions.
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swift (1)
1-2: LGTM!The GRDB migration correctly uses
Row.fetchAllwithStatementArguments, preserves thefetchLimit = limit * 20over-fetch strategy, and properly decodes attributed body with text fallback for case-insensitive matching.Also applies to: 65-96
.parity/parity-utils.mjs (2)
49-84: LGTM!The new reference preparation helpers provide defensive checks with clear error messages:
ensureReferenceDependencieshandles missingnode_modules,findReferenceNativeModulesupports bothIMessage.nodeandSwiftServer.node, andensureReferenceNativeModulevalidates artifacts post-build.
113-115: LGTM!The integration into
ensureReferenceAPIproperly sequences dependency installation before native module validation.src/IMessage/Sources/IMessage/PlatformAPI.swift (5)
119-125: LGTM!Passing
accountIDandreportErrorMessagetoEventWatcherLifecycle.subscribeToEventsenables proper error reporting context in the event watching flow.
127-139: LGTM!Using
messageUpdateCursorSnapshot()provides the three cursor values (lastRowID,lastDateRead,lastDateEdited) needed for targeted update detection, aligning with the message-centric update pipeline.
920-941: LGTM!The refactor to
mapAndHashMessagesByRowIDcorrectly builds a[Int: [PlatformSDK.Message]]dictionary, then selects the latest message payload bymsgRow.rowIDwhen constructinglatestMessagesByChatGUID.
964-979: LGTM!The new
mapAndHashMessagesByRowIDoverload cleanly encapsulates the pattern of fetching payload rows before calling the core mapping function.
1042-1044: LGTM!Using
parseAssociatedMessageTarget(...).messageGUIDprovides type-safe associated message GUID extraction, replacing the previous regex-based approach. This aligns with the typedAssociatedMessageTargetmodeling inMessageMapperTypes.swift.src/IMessage/Sources/IMDatabase/Database/IMDatabase+Chats.swift (2)
32-48: LGTM!Good defensive handling: logging an error for chats missing a GUID and using
compactMapto filter them out, plus defaulting nullableservice_nameto"NONE".
52-64: LGTM!Clean GRDB migration for handle fetching with proper
requiredInt/requiredStringusage since the JOIN should ensure these columns are present.scripts/imessage-perf.mjs (3)
1-45: LGTM!Clean script structure with proper imports, argument parsing, and conditional execution paths for Swift-only, parity-only, or combined runs.
146-203: LGTM!The parity runner correctly handles timeouts with SIGTERM and exit code 124, uses a temp directory for output, and properly cleans up in the
finallyblock.
241-366: LGTM!The reporting helpers provide clean terminal output with proper TTY-aware coloring, table formatting with numeric column alignment, and a helpful
--helpmessage.src/IMessage/Sources/IMDatabase/Database/IMDatabase+Attachments.swift (1)
61-83: LGTM!The
Attachment.init?(row:)migration to GRDB'sRowaccessors is clean. The index-based access pattern withrequiredInt,optionalString,looseBool, etc. is consistent with the project'sRowextension helpers inColumn+.swift.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swift (2)
4-10: LGTM!The
tableColumnsimplementation correctly extracts column names fromPRAGMA table_info. The table name interpolation is safe here since table names are defined as staticsqlNameproperties fromIMDatabaseTableconformances.
25-30: LGTM!The
sqlArgumentshelper appropriately usespreconditionFailurefor invalid argument types—this is a programmer error that should fail fast during development.src/IMessage/Sources/IMDatabase/Schema/IMDatabaseSchema.swift (2)
17-30: LGTM!Clean schema abstraction. Using
Set<String>for column lookups inhas(_:)provides O(1) performance for schema checks.
54-66: Schema caching may race if called concurrently outside of database transactions.
schema()reads and writesschemaCachewithout synchronization. Although IMDatabase provides aread()method that serializes access via GRDB'sDatabaseQueue, theschema()method bypasses this and accesses the cache directly. If called from multiple threads or concurrent tasks without being wrapped indatabase.read { }blocks, race conditions can occur leading to redundant schema loads or partial reads.src/IMessage/Sources/IMDatabase/Support/Column+.swift (2)
17-23: Required accessors crash on nil/type mismatch.
requiredString(at:)andrequiredInt(at:)use unconditional casts (as String,as Int). These will crash if the column is NULL or contains an incompatible type. This is acceptable for columns known to be NOT NULL in the schema, but callers must ensure the column exists and is non-nullable.
25-52: LGTM!The
imCoreDate(at:)implementation has good defensive checks: handling NULL, zero timestamps (which Apple uses instead of NULL in some cases), and bogus future dates that could cause overflow. ThelooseBoolcorrectly maps only1totrue, matching iMessage's boolean column semantics.src/IMessage/Sources/IMessage/EventWatcher/EventWatcherLifecycle.swift (3)
11-15: LGTM!Clean encapsulation of subscription state into a dedicated struct. This improves code organization by grouping related callback/configuration fields.
65-76: LGTM!The addition of
lastDateEditedto the cursor snapshot enables tracking message edits in the update pipeline, aligning with the newmessageUpdateCursorSnapshot()API.
97-107: LGTM!The
EventWatcherinitialization now correctly passes all three cursor dimensions (lastRowID,lastDateRead,lastDateEdited) to enable comprehensive update detection.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (2)
272-293: LGTM!The
mappedReactionRowsimplementation correctly uses parameterized queries withsqlArgumentsfor mixed-type bindings. AddingORDER BY m.ROWID ASCensures deterministic ordering.
307-330: LGTM!The helper functions
messageSelectionSQL,placeholders, androwValuePlaceholdersare clean utilities for building parameterized SQL queries.src/IMessage/Sources/IMDatabase/Database/IMDatabase+Updates.swift (2)
7-23: LGTM!The new
UpdatedMessageChangeandUpdatedMessagesQueryResultstructures cleanly model per-message update flags with separate maxima for each dimension (rowID, dateRead, dateEdited).
37-101: LGTM!The message-scoped update query implementation is well-structured:
- Correctly computes
isNew,wasRead,wasEditedflags by comparing against provided cursors- Updates maxima only when the corresponding flag is set
- Handles orphaned messages gracefully with rate-limited logging
src/IMessage/Sources/PlatformSDK/ServerEvent.swift (3)
40-55: LGTM!The new
state_syncmessage and reaction event cases provide a clean API for granular message mutations. The documentation comments clearly describe each case's purpose.
83-87: Minor inconsistency: raw string instead of enum rawValue.Line 85 uses the string literal
"thread_messages_refresh"while other cases useServerEventType.x.rawValue. Since this case is deprecated, this is acceptable to avoid compiler warnings about referencing deprecated symbols.
145-166: LGTM!The
messageStateSyncJSONandmessageReactionStateSyncJSONhelpers centralize thestate_syncpayload construction, reducing duplication across the message/reaction cases.src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift (3)
46-54: LGTM on the protocol design.The
MappedDatabaseRowprotocol withFetchableRecordconformance and the convenience initializer that derives column indexes fromrow.columnNamesis a clean pattern for GRDB integration. The separation between the protocol requirement and the convenience extension keeps the API flexible.Regarding the SwiftLint hint about
public extension: this is a style preference and the current approach is readable and idiomatic for Swift.
117-119: Good documentation on the new field.The
replyToGUIDproperty is well-documented, explaining its purpose for reaction removal linkage. The optional type correctly handles databases where this column may not exist or contain null values.
323-365: Clean row accessor pattern.The helper methods correctly separate the column-existence check (via
MappedRowColumnIndexes) from the type conversion. Returningnilfor missing columns handles schema variations across macOS versions gracefully.src/IMessage/Sources/IMessage/Mappers/MessageMapper+Associated.swift (4)
26-58: Clean refactor to typed associated message handling.The switch on the typed
AssociatedMessageTypeenum cases (.sticker,.heading,.reaction) is much cleaner than string-based parsing. The delegation tomapReactionActionwith the strongly-typedAssociatedReactionobject improves type safety.
61-85: LGTM on reaction assignment logic.The pattern matching on
.reaction(parts)is cleaner. The removal by sender ID is correct since iMessage allows at most one reaction per participant per message part—changing a reaction produces an unreact followed by a new react.
102-125: Good defensive handling in reaction action mapping.The
summaryInfo.string("ams").flatMap { $0.isEmpty ? nil : $0 }chain at line 121 correctly handles both missing and empty string cases before falling back to "a message". SettingisHidden = trueappropriately hides the synthetic action message.
146-151: Clear sender resolution logic.The
messageSenderIDfunction correctly handles the edge case wherehandleID == 0andparticipantIDis empty—this typically indicates a self-sent message even whenisFromMemight not be set. The fallback to empty string for missing participant IDs is a reasonable defensive default.
| func hydrateAttachments(for messages: inout OrderedDictionary<Message.ID, Message>) throws { | ||
| let messageRowIDs = messages.keys.map(String.init) | ||
|
|
||
| let statement = try Statement.prepare(escapedSQL: """ | ||
| \(attachmentQuerySharedPrologue) | ||
| WHERE m.ROWID IN (\(messageRowIDs.joined(separator: ","))) | ||
| """, for: database) | ||
| try read { db in | ||
| let rows = try Row.fetchAll(db, sql: """ | ||
| \(attachmentQuerySharedPrologue) | ||
| WHERE m.ROWID IN (\(messageRowIDs.joined(separator: ","))) | ||
| """) |
There was a problem hiding this comment.
SQL injection risk with direct string interpolation of row IDs.
The messageRowIDs are interpolated directly into the SQL string. While these are Int values converted to String, this pattern bypasses GRDB's parameterization. Prefer using placeholders with StatementArguments for consistency and safety.
🔒 Proposed fix using parameterized query
func hydrateAttachments(for messages: inout OrderedDictionary<Message.ID, Message>) throws {
- let messageRowIDs = messages.keys.map(String.init)
+ let messageRowIDs = Array(messages.keys)
try read { db in
let rows = try Row.fetchAll(db, sql: """
\(attachmentQuerySharedPrologue)
- WHERE m.ROWID IN (\(messageRowIDs.joined(separator: ",")))
- """)
+ WHERE m.ROWID IN (\(messageRowIDs.map { _ in "?" }.joined(separator: ", ")))
+ """, arguments: StatementArguments(messageRowIDs))🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Attachments.swift around
lines 31 - 38, The SQL in hydrateAttachments(for:) interpolates messageRowIDs
directly into the query (via attachmentQuerySharedPrologue + "WHERE m.ROWID IN
(...)"), which risks SQL injection and bypasses GRDB parameterization; update
hydrateAttachments to build a parameterized IN clause (create a comma-separated
list of "?" placeholders equal to messages.keys.count) and pass messageRowIDs as
StatementArguments (or as the arguments parameter to Row.fetchAll) so
Row.fetchAll(db, sql: ..., arguments: ...) is used instead of string
interpolation; reference hydrateAttachments(for:),
attachmentQuerySharedPrologue, and the Row.fetchAll call when making the change.
| try read { db in | ||
| for row in try Row.fetchAll(db, sql: unreadStatesQuery) { | ||
| let chatGUID = row.requiredString(at: 0) | ||
|
|
||
| let lastReadMessageTimestamp = try Date(nanosecondsSinceReferenceDate: row[3].expect(Int.self)) | ||
| let lastReadMessageTimestamp = Date(nanosecondsSinceReferenceDate: row.requiredInt(at: 2)) | ||
|
|
||
| let unreadCount: Int = try row[2].expect(Int.self) | ||
| let unreadCount = row.requiredInt(at: 1) | ||
|
|
||
| chatStates[chatRef] = ChatState(unreadCount: unreadCount, lastReadMessageTimestamp: lastReadMessageTimestamp) | ||
| chatStates[chatGUID] = ChatState(unreadCount: unreadCount, lastReadMessageTimestamp: lastReadMessageTimestamp) |
There was a problem hiding this comment.
Handle nullable chat rows before decoding into ChatState.
This loop now uses requiredString/requiredInt, but the query still groups all chats without filtering c.guid IS NOT NULL or COALESCE-ing last_read_message_timestamp. A single null-guid or null-timestamp row will throw here and abort unread-state refresh for the whole watcher. Please either tighten the SQL or skip/default nullable rows during decoding.
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 61-61: Magic numbers should be replaced by named constants
(no_magic_numbers)
🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Unreads.swift around
lines 57 - 65, The loop can throw if the DB returns NULL guid or timestamp;
change the decoder to guard against nullable columns instead of calling
requiredString/requiredInt directly: when iterating Row.fetchAll(db, sql:
unreadStatesQuery) use Row.isNull(at:) or optional accessors (e.g.,
optionalString/optionalInt) to skip rows with a nil chat GUID or to provide a
safe default for last_read_message_timestamp, and only call
ChatState(unreadCount:..., lastReadMessageTimestamp:...) and assign into
chatStates[chatGUID] when the GUID is non-nil (or tighten unreadStatesQuery to
include "c.guid IS NOT NULL" and COALESCE(last_read_message_timestamp, 0) to
guarantee non-nulls).
| static func requireReadable() throws { | ||
| guard isReadable else { | ||
| _ = fullDiskAccessRequest | ||
| throw FullDiskAccessRequired(chatDBURL: chatDBURL) | ||
| } | ||
| } | ||
|
|
||
| static func imDatabase() throws -> IMDatabase { | ||
| try requireReadable() | ||
| return try IMDatabase(messagesDataBaseURL: messagesDirectory) | ||
| } | ||
|
|
||
| static func queue() throws -> DatabaseQueue { | ||
| try requireReadable() | ||
| var configuration = Configuration() | ||
| configuration.readonly = true | ||
| return try DatabaseQueue(path: chatDBURL.path, configuration: configuration) | ||
| } |
There was a problem hiding this comment.
Make this live chat.db suite opt-in instead of failing the default test run.
Every test now depends on a readable local Messages database and Full Disk Access; otherwise requireReadable() throws immediately. That will turn normal CI runs and contributor test runs red on machines without ~/Library/Messages/chat.db access. Please gate this suite behind an explicit env/test-plan switch, or skip it when the DB is unavailable instead of failing.
🤖 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/IMessage/Sources/IMDatabaseTests/LiveSQLTests.swift` around lines 32 -
49, Change the live-chat DB guard to be opt-in: in requireReadable() first check
an explicit env var or test switch (e.g.
ProcessInfo.processInfo.environment["ENABLE_LIVE_CHATDB_TESTS"] == "1"); if the
switch is not enabled, call XCTSkip("Live chat.db tests disabled; set
ENABLE_LIVE_CHATDB_TESTS=1 to run") (import XCTest) instead of throwing
FullDiskAccessRequired, otherwise proceed with the existing
fullDiskAccessRequest and throw as before; this keeps imDatabase() and queue()
intact while skipping the suite by default.
| let msgRows = try db.mappedMessageRows(rowIDs: queryResult.updatedMessages.map(\.rowID)) | ||
| // `messageJoins` LEFT JOINs `chat_message_join`, so a message in multiple | ||
| // chats yields multiple rows with the same ROWID. Keep first. | ||
| let msgRowsByRowID = Dictionary(msgRows.map { ($0.rowID, $0) }, uniquingKeysWith: { first, _ in first }) | ||
|
|
||
| var batchesByThreadID = [PlatformSDK.ThreadID: ThreadBatch]() | ||
| var pendingByThreadID = [PlatformSDK.ThreadID: OrderedDictionary<Int, PendingMessage>]() | ||
|
|
||
| for change in queryResult.updatedMessages { | ||
| guard let msgRow = msgRowsByRowID[change.rowID] else { | ||
| 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.
Don't collapse joined message rows to the first ROWID match.
The comment here already calls out that a single message can produce multiple rows when it belongs to multiple chats, but msgRowsByRowID still keeps only the first one. That makes Line 81 thread-routing data-dependent: the same UpdatedMessageChange can be emitted under the wrong thread, and the later message mapping will use the wrong chat-specific row. Please key this lookup by (rowID, chatGUID/threadID) and resolve each change against its own chat instead of discarding the duplicate rows.
🤖 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/IMessage/Sources/IMessage/EventWatcher/EventWatcher`+Updates.swift around
lines 68 - 82, msgRowsByRowID currently collapses multiple joined rows by rowID
causing wrong thread routing; change the lookup so each UpdatedMessageChange
resolves against the row for its specific chat (use change.chatGUID or
msgRow.threadID). Build a map keyed by a compound key (rowID, threadID/chatGUID)
or a Dictionary<Int, [MessageRow]> and then select the MessageRow whose threadID
or chatGUID matches change.chatGUID when processing queryResult.updatedMessages
(refer to msgRows, msgRowsByRowID, queryResult.updatedMessages, change.rowID,
change.chatGUID, and msgRow.threadID).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift (1)
119-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPage and sort on the same effective timestamp.
For
.afterpages, the filter switches toMAX(m.date, COALESCE(m.date_edited, 0)), but the query still orders and limits oncmj.message_date. An older message edited after the cursor will pass theWHEREand then be sorted by its original send date, which can misorder the page or push newer rows past theLIMIT.Suggested fix
- let dateExpression = comparisonOperator == ">" && messageSchema.has(.dateEdited) + let dateExpression = comparisonOperator == ">" && messageSchema.has(.dateEdited) ? "MAX(m.\(MessageTable.Column.date.sqlName), COALESCE(m.\(MessageTable.Column.dateEdited.sqlName), 0))" : "cmj.\(ChatMessageJoinTable.Column.messageDate.sqlName)" @@ - sql += "\nORDER BY cmj.message_date \(order), cmj.message_id \(order)\nLIMIT \(limit)" + sql += "\nORDER BY \(dateExpression) \(order), cmj.message_id \(order)\nLIMIT \(limit)"🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift around lines 119 - 141, The query uses dateExpression for the WHERE when comparisonOperator is ">" but still orders/limits by cmj.message_date, causing edited messages to be filtered by edited timestamp but sorted by original send date; update the SQL assembly in the block that builds sql (referencing dateExpression, comparisonOperator, messageSchema.has(.dateEdited), and the ORDER BY line that currently uses cmj.message_date) so the ORDER BY and LIMIT use the same effective timestamp expression (dateExpression) instead of cmj.message_date, preserving the existing tie-breaker cmj.message_id and order direction.
♻️ Duplicate comments (1)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swift (1)
57-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle nullable unread-state columns before building
ChatState.This still assumes
c.guidandc.last_read_message_timestampare non-null, but the query does not enforce that. A single null row will throw here and abort the entire unread refresh. Tighten the SQL (c.guid IS NOT NULL,COALESCE(...)) or decode these fields defensively before creatingChatState.🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Unreads.swift around lines 57 - 65, The unread refresh assumes non-null columns and will crash if a row has NULL GUID or timestamp; in the read { db in ... } loop where you iterate fetchAllRowsCached(db: db, sql: unreadStatesQuery) and build ChatState, validate/decode row values defensively (use optional-safe getters or guard-let the guid and timestamp/int values) before constructing ChatState, skipping or logging rows with missing data; alternatively tighten unreadStatesQuery to include "c.guid IS NOT NULL" and COALESCE(...) for timestamp/unread count so requiredString/requiredInt never sees NULL—apply the chosen fix around the loop that assigns chatStates[chatGUID] = ChatState(...).
🧹 Nitpick comments (2)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedThreads.swift (1)
55-58: ⚡ Quick winDon't bucket unexpected participant rows under
-1.This query already filters on
chat_id IN (...), so a nilrow.chatIDis an unexpected decode/data issue. Grouping those rows under-1hides the problem and silently drops participants from the actual chat. Guard-and-skip here, or makechat_idrequired for this fetch path.Suggested fix
return try read { db in try MappedHandleRow.fetchAllMapped(db, sql: sql, arguments: StatementArguments(chatRowIDs)).reduce(into: [:]) { result, row in - result[row.chatID ?? -1, default: []].append(row) + guard let chatID = row.chatID else { return } + result[chatID, default: []].append(row) } }🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedThreads.swift around lines 55 - 58, The code is grouping unexpected nil chatID rows under -1 which hides data issues; update the reduce in the read block that processes MappedHandleRow.fetchAllMapped (the closure that currently uses result[row.chatID ?? -1, default: []].append(row)) to skip any rows where row.chatID is nil (or alternatively make chat_id non-optional at decode time) so nil chat IDs are not bucketed under -1; ensure the logic simply guards on row.chatID and only appends when it is non-nil, optionally logging or asserting on unexpected nils for debugging.src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swift (1)
25-29: ⚡ Quick winAvoid trapping on invalid SQL argument payloads.
This helper is now shared across a lot of the GRDB path, and
preconditionFailureturns one bad bind value into a process crash. Prefer a throwing helper or a type-restricted API here so unsupported values fail as a normal database error instead of aborting the process.🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedShared.swift around lines 25 - 29, The sqlArguments(_:) helper currently uses preconditionFailure which aborts the process on an invalid bind value; change it to a throwing API that returns StatementArguments or throws a descriptive database/bind error instead of crashing. Update the function signature sqlArguments(_ values: [Any]) to be throws and throw a custom error (or a GRDB-compatible error) when StatementArguments(values) fails, and then update its callers to propagate or handle the thrown error so invalid SQL argument payloads surface as recoverable database errors rather than process aborts. Ensure the thrown error includes context about the offending value(s) for easier debugging.
🤖 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 `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Search.swift:
- Around line 61-65: Validate and clamp the public parameter limit before
computing fetchLimit: ensure limit is positive (reject or default to 1 for
non-positive values) and cap it to a reasonable MAX_LIMIT to avoid absurd
requests, then compute fetchLimit using a safe multiplication that prevents
overflow (e.g., if limit > Int.max / 20 then use MAX_LIMIT or compute fetchLimit
= min(limit, MAX_LIMIT) * 20). Update the code around the existing symbols
limit, fetchLimit, and arguments/sqlArguments to use the validated/clamped value
so multiplying by 20 cannot trap and SQL behavior is well-defined.
---
Outside diff comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedMessages.swift:
- Around line 119-141: The query uses dateExpression for the WHERE when
comparisonOperator is ">" but still orders/limits by cmj.message_date, causing
edited messages to be filtered by edited timestamp but sorted by original send
date; update the SQL assembly in the block that builds sql (referencing
dateExpression, comparisonOperator, messageSchema.has(.dateEdited), and the
ORDER BY line that currently uses cmj.message_date) so the ORDER BY and LIMIT
use the same effective timestamp expression (dateExpression) instead of
cmj.message_date, preserving the existing tie-breaker cmj.message_id and order
direction.
---
Duplicate comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+Unreads.swift:
- Around line 57-65: The unread refresh assumes non-null columns and will crash
if a row has NULL GUID or timestamp; in the read { db in ... } loop where you
iterate fetchAllRowsCached(db: db, sql: unreadStatesQuery) and build ChatState,
validate/decode row values defensively (use optional-safe getters or guard-let
the guid and timestamp/int values) before constructing ChatState, skipping or
logging rows with missing data; alternatively tighten unreadStatesQuery to
include "c.guid IS NOT NULL" and COALESCE(...) for timestamp/unread count so
requiredString/requiredInt never sees NULL—apply the chosen fix around the loop
that assigns chatStates[chatGUID] = ChatState(...).
---
Nitpick comments:
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedShared.swift:
- Around line 25-29: The sqlArguments(_:) helper currently uses
preconditionFailure which aborts the process on an invalid bind value; change it
to a throwing API that returns StatementArguments or throws a descriptive
database/bind error instead of crashing. Update the function signature
sqlArguments(_ values: [Any]) to be throws and throw a custom error (or a
GRDB-compatible error) when StatementArguments(values) fails, and then update
its callers to propagate or handle the thrown error so invalid SQL argument
payloads surface as recoverable database errors rather than process aborts.
Ensure the thrown error includes context about the offending value(s) for easier
debugging.
In `@src/IMessage/Sources/IMDatabase/Database/IMDatabase`+MappedThreads.swift:
- Around line 55-58: The code is grouping unexpected nil chatID rows under -1
which hides data issues; update the reduce in the read block that processes
MappedHandleRow.fetchAllMapped (the closure that currently uses
result[row.chatID ?? -1, default: []].append(row)) to skip any rows where
row.chatID is nil (or alternatively make chat_id non-optional at decode time) so
nil chat IDs are not bucketed under -1; ensure the logic simply guards on
row.chatID and only appends when it is non-nil, optionally logging or asserting
on unexpected nils for debugging.
🪄 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: dae8fe86-72e7-4238-8d1e-95c38d2bae23
📒 Files selected for processing (6)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedThreads.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift
📜 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
🧠 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/IMDatabase/Database/IMDatabase+Search.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedThreads.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedShared.swiftsrc/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swiftsrc/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Search.swift
[Warning] 75-75: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+Unreads.swift
[Warning] 61-61: Magic numbers should be replaced by named constants
(no_magic_numbers)
src/IMessage/Sources/IMDatabase/Models/MappedDatabaseRows.swift
[Warning] 69-69: Force unwrapping should be avoided
(force_unwrapping)
[Warning] 50-50: Prefer not to use extension access modifiers
(no_extension_access_modifier)
src/IMessage/Sources/IMDatabase/Database/IMDatabase+MappedMessages.swift
[Warning] 77-77: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 93-93: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 93-93: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 65-65: Magic numbers should be replaced by named constants
(no_magic_numbers)
[Warning] 264-264: Arguments should be either on the same line, or one per line
(multiline_arguments)
[Warning] 264-264: Arguments should be either on the same line, or one per line
(multiline_arguments)
| // Fetch more than limit to account for filtering - we'll filter in Swift after decoding | ||
| let fetchLimit = limit * 20 | ||
|
|
||
| let statement = try cachedStatement(forEscapedSQL: sql).reset() | ||
|
|
||
| // Bind parameters in order | ||
| if let chatGUID { | ||
| try statement.bind(chatGUID, fetchLimit) | ||
| } else { | ||
| try statement.bind(fetchLimit) | ||
| } | ||
|
|
||
| var matchingRowIDs: [Int] = [] | ||
| let arguments = chatGUID.map { sqlArguments([$0, fetchLimit]) } ?? StatementArguments([fetchLimit]) |
There was a problem hiding this comment.
Validate limit before multiplying it.
limit * 20 will trap on large inputs, and non-positive limits produce odd SQL behavior. Since limit is public API here, clamp or reject it before deriving fetchLimit.
Suggested fix
- // Fetch more than limit to account for filtering - we'll filter in Swift after decoding
- let fetchLimit = limit * 20
+ guard limit > 0 else { return [] }
+ let limit = min(limit, Int.max / 20)
+ // Fetch more than limit to account for filtering - we'll filter in Swift after decoding
+ let fetchLimit = limit * 20📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Fetch more than limit to account for filtering - we'll filter in Swift after decoding | |
| let fetchLimit = limit * 20 | |
| let statement = try cachedStatement(forEscapedSQL: sql).reset() | |
| // Bind parameters in order | |
| if let chatGUID { | |
| try statement.bind(chatGUID, fetchLimit) | |
| } else { | |
| try statement.bind(fetchLimit) | |
| } | |
| var matchingRowIDs: [Int] = [] | |
| let arguments = chatGUID.map { sqlArguments([$0, fetchLimit]) } ?? StatementArguments([fetchLimit]) | |
| guard limit > 0 else { return [] } | |
| let limit = min(limit, Int.max / 20) | |
| // Fetch more than limit to account for filtering - we'll filter in Swift after decoding | |
| let fetchLimit = limit * 20 | |
| var matchingRowIDs: [Int] = [] | |
| let arguments = chatGUID.map { sqlArguments([$0, fetchLimit]) } ?? StatementArguments([fetchLimit]) |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 62-62: Magic numbers should be replaced by named constants
(no_magic_numbers)
🤖 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/IMessage/Sources/IMDatabase/Database/IMDatabase`+Search.swift around
lines 61 - 65, Validate and clamp the public parameter limit before computing
fetchLimit: ensure limit is positive (reject or default to 1 for non-positive
values) and cap it to a reasonable MAX_LIMIT to avoid absurd requests, then
compute fetchLimit using a safe multiplication that prevents overflow (e.g., if
limit > Int.max / 20 then use MAX_LIMIT or compute fetchLimit = min(limit,
MAX_LIMIT) * 20). Update the code around the existing symbols limit, fetchLimit,
and arguments/sqlArguments to use the validated/clamped value so multiplying by
20 cannot trap and SQL behavior is well-defined.
2 participants
This consolidates Messages
chat.dbaccess behind GRDB. It models Apple's tables/columns in Swift, keeps runtime guards for OS-specific optional columns, and replaces the custom SQLite statement/cache layer withDatabaseQueue,FetchableRecord, and shared row decoding. The update watcher also does more targeted message/reaction/read-state refreshes, and the hot mapped-message paths batch/dedupe lookups with targeted indexes where available.How it works:
chat.dbthrough a read-onlyDatabaseQueue.TableRecords and columns asColumnExpressionenums.TableSchemachecks before touching Apple columns that vary across OS versions.FetchableRecord/DatabaseValueConvertibleand central row helpers.Validation:
swift build --target IMDatabaseswift test --filter MessageMapperFixtureTestsswift test --filter HashingTestsyarn test:js src/swift-json.test.ts --runInBandswift testbuilds, then the local SwiftPM test runner exits with signal 11 afterEmojiSPITests.swift:31reports.nilResponse(method: "initWithLocale:").