Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThreads and message mapping now carry an explicit accountID; sticker reactions are supported end-to-end (new assoc types, reaction handling, asset lookup and imgURL resolution). AppleiMessage requires accountID at construction and sticker reactions are blocked in add/remove reaction calls. Changes
Sequence Diagram(s)sequenceDiagram
participant API as "AppleiMessage (API)"
participant Mapper as "mappers"
participant DB as "Database"
participant Storage as "Attachment FS/URL"
API->>Mapper: mapThreads / mapMessages(accountID)
Mapper->>DB: query messages and reactions (includes ROWID)
DB-->>Mapper: message rows and reaction rows (with ROWID)
alt reaction is sticker
Mapper->>API: request reactionStickerAssetURL(accountID, ROWID)
API->>DB: lookup attachment row for reaction ROWID
DB-->>API: attachment file path / row (or empty -> error)
API->>Storage: resolve file URL from path
Storage-->>API: file URL
API-->>Mapper: imgURL for reaction (or throw if missing/invalid)
end
Mapper-->>API: mapped messages with reactions (imgURL set for stickers)
API-->>Client: messages payload (includes sticker reaction URLs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds support for parsing sticker reactions by mapping them into reaction/action objects and providing an asset resolver path for reaction sticker attachments.
Changes:
- Extend reaction DB row typing/query to include the reaction message
ROWIDso sticker assets can be resolved. - Map sticker reactions to include an
imgURL(asset URL) and allow'sticker'as a supported reaction key. - Add
asset://…/reaction-sticker/<rowid>handling to resolve the sticker attachment file path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Adds ROWID to MappedReactionMessageRow so reactions can be referenced for asset lookup. |
| src/mappers.ts | Generates sticker reaction asset URLs, threads accountID through mapping, and enables sticker reactions in parsing. |
| src/db-api.ts | Includes m.ROWID in getMessageReactions SELECT; passes accountID into mapping calls. |
| src/constants.ts | Adds assoc message types and verb strings for reacted/unreacted sticker. |
| src/api.ts | Makes accountID available on the API instance and adds getAsset support for reaction-sticker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/mappers.ts
Outdated
| type: reactionType, | ||
| messageID: m.linkedMessageID, | ||
| participantID: m.senderID, | ||
| imgURL: assocMsgType === 'reacted_sticker' ? attachmentRows[0]?.filename : undefined, |
There was a problem hiding this comment.
For sticker reaction action messages, imgURL is currently being set from attachmentRows[0]?.filename, which is the raw DB path (may include ~) and is not a file URL/asset URL like other image locations in this mapper. This will likely break rendering of the sticker preview. Consider using attachmentRows[0]?.filePath converted via url.pathToFileURL(...), or reuse the new reactionStickerAssetURL(accountID, msgRow.ROWID) so the renderer can resolve it through getAsset.
| imgURL: assocMsgType === 'reacted_sticker' ? attachmentRows[0]?.filename : undefined, | |
| imgURL: assocMsgType === 'reacted_sticker' && attachmentRows[0]?.filePath | |
| ? url.pathToFileURL(replaceTilde(attachmentRows[0].filePath)).toString() | |
| : undefined, |
| // eslint-disable-next-line @typescript-eslint/default-param-last -- FIXME(skip) | ||
| export function mapMessage(msgRow: MappedMessageRow, attachmentRows: MappedAttachmentRow[] = [], reactionRows: MappedReactionMessageRow[], currentUserID: string): BeeperMessage[] { | ||
| export function mapMessage(msgRow: MappedMessageRow, attachmentRows: MappedAttachmentRow[] = [], reactionRows: MappedReactionMessageRow[], currentUserID: string, accountID: string): BeeperMessage[] { | ||
| const attachments = attachmentRows.map(a => mapAttachment(a, msgRow)).filter(attachment => attachment != null) |
There was a problem hiding this comment.
mapMessage now requires an accountID argument, which breaks existing test fixtures (src/tests/*.json) that serialize Parameters<typeof mapMessage> and currently only provide 4 parameters. To avoid churn and reduce the chance of runtime undefined account IDs, consider making accountID optional with a sensible default (e.g. $accountID) or updating the fixture format + tests in the same PR.
src/mappers.ts
Outdated
| @@ -75,6 +77,7 @@ function assignReactions(currentUserID: string, message: BeeperMessage, _reactio | |||
| id: participantID, | |||
| reactionKey: actionKey === 'emoji' ? reaction.associated_message_emoji : actionKey, | |||
| participantID, | |||
| imgURL: actionKey === 'sticker' ? reactionStickerAssetURL(accountID, reaction.ROWID) : undefined, | |||
| }) | |||
There was a problem hiding this comment.
Sticker reactions are newly supported here (setting reactionKey: 'sticker' and imgURL via reactionStickerAssetURL), but the existing mapper snapshot fixtures only cover numeric/emoji reactions (2000-2006) and don't include 2007/3007. Adding a fixture/snapshot case for reacted/unreacted sticker reactions would help prevent regressions in parsing and asset URL formatting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/api.ts (2)
254-266:⚠️ Potential issue | 🔴 CriticalMissing
accountIDin Context object for Catalina thread creation.Similar to
getThread, this Context object also needsaccountIDto satisfy the updatedContexttype requirements.🐛 Proposed fix
const thread = mapThread( chatRow, { + accountID: this.accountID, handleRowsMap: { [chatRow.guid]: handleRows }, currentUserID: this.currentUser!.id, threadReadStore: this.threadReadStore,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 254 - 266, The Context object passed into mapThread when constructing thread (call site: mapThread(..., { handleRowsMap: ..., currentUserID: this.currentUser!.id, ... })) is missing accountID which the updated Context type requires; add accountID: this.currentUser!.accountId (or the correct account identifier available on this.currentUser) to the second argument object (alongside handleRowsMap, mapMessageArgsMap, reminders, archivalStates) so mapThread receives a valid Context for chatRow.guid.
224-238:⚠️ Potential issue | 🔴 CriticalMissing
accountIDin Context object causes build failure.The pipeline failure indicates this Context object is missing the required
accountIDfield. ThemapThreadfunction expectsaccountIDin the Context type, but it's not provided here.🐛 Proposed fix
return hashThread(mapThread( chatRow, { + accountID: this.accountID, handleRowsMap: { [chatRow.guid]: handleRows }, currentUserID: this.currentUser!.id, threadReadStore: this.threadReadStore,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 224 - 238, The Context object passed to mapThread is missing the required accountID property causing the build error; add accountID to that object (e.g., accountID: this.currentUser!.accountID) alongside currentUserID, so the call to mapThread(chatRow, { ... }) includes accountID and satisfies the Context type expected by mapThread.
🧹 Nitpick comments (1)
src/api.ts (1)
889-896: Verify error message clarity and consider edge cases.The error handling looks reasonable, but consider that
db.getAttachments([reactionRowID])could return an attachment row with anullor emptyfilename/filePatheven if the attachment exists. The current checka.filePathhandles this.One minor observation: the error message says "couldn't resolve sticker attachment for reaction row" but it might be clearer to include the row ID for debugging purposes.
💡 Optional: Include row ID in error message
case 'reaction-sticker': { const reactionRowID = Number.parseInt(methodName, 10) if (!Number.isFinite(reactionRowID)) throw new Error("invalid reaction sticker row ID") const db = await this.ensureDB() const attachment = (await db.getAttachments([reactionRowID])).find(a => a.filePath) - if (!attachment?.filePath) throw new Error("couldn't resolve sticker attachment for reaction row") + if (!attachment?.filePath) throw new Error(`couldn't resolve sticker attachment for reaction row ${reactionRowID}`) return url.pathToFileURL(attachment.filePath).href }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 889 - 896, The thrown error when no valid sticker attachment is found lacks context — update the reaction-sticker branch so that after calling ensureDB() and db.getAttachments([reactionRowID]) you include the reactionRowID (and if available the attachment.filename/filePath) in the Error message; locate the logic in the case 'reaction-sticker' block that computes reactionRowID from methodName and that queries getAttachments, and change the throw to include the row ID (and optional attachment details) to make the error message explicit for debugging.
🤖 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/mappers.ts`:
- Line 621: The imgURL for reacted_sticker is using the raw DB column
attachmentRows[0]?.filename (which may contain ~) instead of the resolved
path/URL used elsewhere; change the branch that sets imgURL for assocMsgType ===
'reacted_sticker' to use the same resolved value pattern as other attachments
(use attachmentRows[0]?.filePath converted to a file:// URL or the project's
asset URL helper used by assignReactions) so the value is displayable and
consistent with other attachment handling.
---
Outside diff comments:
In `@src/api.ts`:
- Around line 254-266: The Context object passed into mapThread when
constructing thread (call site: mapThread(..., { handleRowsMap: ...,
currentUserID: this.currentUser!.id, ... })) is missing accountID which the
updated Context type requires; add accountID: this.currentUser!.accountId (or
the correct account identifier available on this.currentUser) to the second
argument object (alongside handleRowsMap, mapMessageArgsMap, reminders,
archivalStates) so mapThread receives a valid Context for chatRow.guid.
- Around line 224-238: The Context object passed to mapThread is missing the
required accountID property causing the build error; add accountID to that
object (e.g., accountID: this.currentUser!.accountID) alongside currentUserID,
so the call to mapThread(chatRow, { ... }) includes accountID and satisfies the
Context type expected by mapThread.
---
Nitpick comments:
In `@src/api.ts`:
- Around line 889-896: The thrown error when no valid sticker attachment is
found lacks context — update the reaction-sticker branch so that after calling
ensureDB() and db.getAttachments([reactionRowID]) you include the reactionRowID
(and if available the attachment.filename/filePath) in the Error message; locate
the logic in the case 'reaction-sticker' block that computes reactionRowID from
methodName and that queries getAttachments, and change the throw to include the
row ID (and optional attachment details) to make the error message explicit 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: 0efa99fa-c1dd-418f-98a8-4407af9ccb2a
📒 Files selected for processing (5)
src/api.tssrc/constants.tssrc/db-api.tssrc/mappers.tssrc/types.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: Agent
🧰 Additional context used
🪛 GitHub Actions: CI/CD
src/api.ts
[error] 226-226: TypeScript TS2345: Argument of type '{ handleRowsMap: { [x: string]: MappedHandleRow[]; }; currentUserID: string; threadReadStore: ThreadReadStore | undefined; mapMessageArgsMap: { [x: string]: [...]; }; ... }' is not assignable to parameter of type 'Context'.
🔇 Additional comments (10)
src/constants.ts (1)
22-22: LGTM!The new sticker reaction type codes (2007/3007) and verb mappings follow the established patterns consistently.
Also applies to: 31-31, 42-42, 51-51
src/db-api.ts (2)
83-83: LGTM!Adding
m.ROWIDto the SELECT clause is necessary for the sticker reaction asset URL generation that uses the reaction row ID.
414-414: LGTM!Correctly passes
this.papi.accountIDto match the updatedmapMessagessignature.src/types.ts (1)
176-185: LGTM!The expanded
MappedReactionMessageRowtype now correctly reflects all fields retrieved by the SQL query and used by the reaction mapping logic.src/api.ts (2)
50-52: LGTM!The constructor now correctly accepts and stores
accountIDas a readonly property, enabling it to be threaded through mapping calls.
361-361: LGTM!The
accountIDis correctly threaded through tomapThreadsandmapMessagescalls ingetThreads,getMessages,getMessage, andsearchMessages.Also applies to: 396-396, 414-414, 443-443
src/mappers.ts (4)
62-63: LGTM!The
reactionStickerAssetURLhelper correctly constructs an asset URL that will be resolved by thegetAssethandler's'reaction-sticker'case.
64-89: LGTM!The
assignReactionsfunction is correctly updated to acceptaccountIDand use it to generate sticker reaction image URLs viareactionStickerAssetURL.
624-628: LGTM!Adding
actionKey === 'sticker'to the template eligibility condition correctly enables templated text for sticker reactions.
247-247: LGTM!The
accountIDparameter is consistently threaded through the mapping pipeline:mapMessage,assignReactions,mapMessages,Contexttype, andmapThread.Also applies to: 636-636, 679-679, 696-700, 705-705, 715-715
![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: f14ad5e00e
ℹ️ 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".
src/mappers.ts
Outdated
| type: reactionType, | ||
| messageID: m.linkedMessageID, | ||
| participantID: m.senderID, | ||
| imgURL: assocMsgType === 'reacted_sticker' ? attachmentRows[0]?.filename : undefined, |
There was a problem hiding this comment.
For reacted_sticker, m.action.imgURL is assigned from attachmentRows[0]?.filename, which is the raw DB value (commonly ~/...) instead of a normalized URL/path. In reaction-created action messages, clients can receive an imgURL they cannot fetch, so sticker reactions may render without their image until a later full message reload rebuilds reactions from rows.
Useful? React with 👍 / 👎.
| const db = await this.ensureDB() | ||
| const attachment = (await db.getAttachments([reactionRowID])).find(a => a.filePath) | ||
| if (!attachment?.filePath) throw new Error("couldn't resolve sticker attachment for reaction row") | ||
| return url.pathToFileURL(attachment.filePath).href |
There was a problem hiding this comment.
The new reaction-sticker branch returns file://... directly and skips the existing convertCGBI fallback used by the default asset path. If the sticker attachment is an Apple CGBI PNG (a format this codebase already special-cases), the reaction image can be unreadable/corrupted when rendered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api.ts (1)
891-898: Consider renamingreactionRowIDfor clarity.Since
getAttachmentsaccepts message ROWIDs and reactions are stored as messages, consider renaming toreactionMessageRowIDto clarify that this is the message table ROWID for the reaction message, not an attachment-specific ID.♻️ Optional rename for clarity
case 'reaction-sticker': { - const reactionRowID = Number.parseInt(methodName, 10) - if (!Number.isFinite(reactionRowID)) throw new Error("invalid reaction sticker row ID") + const reactionMessageRowID = Number.parseInt(methodName, 10) + if (!Number.isFinite(reactionMessageRowID)) throw new Error("invalid reaction sticker message row ID") const db = await this.ensureDB() - const attachment = (await db.getAttachments([reactionRowID])).find(a => a.filePath) + const attachment = (await db.getAttachments([reactionMessageRowID])).find(a => a.filePath) if (!attachment?.filePath) throw new Error("couldn't resolve sticker attachment for reaction row") return url.pathToFileURL(attachment.filePath).href }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 891 - 898, Rename the local variable reactionRowID to reactionMessageRowID everywhere in this block to make clear it represents the message table ROWID for the reaction, e.g. change the declaration const reactionRowID = Number.parseInt(methodName, 10) to const reactionMessageRowID = Number.parseInt(methodName, 10), update the Number.isFinite check and error text, and update subsequent uses (the call to this.ensureDB(), db.getAttachments([reactionRowID]) and attachment lookup) to use reactionMessageRowID; also update the thrown error messages to reference "reaction message row ID" or similar so logs reflect the new name.
🤖 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/api.ts`:
- Around line 891-898: Rename the local variable reactionRowID to
reactionMessageRowID everywhere in this block to make clear it represents the
message table ROWID for the reaction, e.g. change the declaration const
reactionRowID = Number.parseInt(methodName, 10) to const reactionMessageRowID =
Number.parseInt(methodName, 10), update the Number.isFinite check and error
text, and update subsequent uses (the call to this.ensureDB(),
db.getAttachments([reactionRowID]) and attachment lookup) to use
reactionMessageRowID; also update the thrown error messages to reference
"reaction message row ID" or similar so logs reflect the new name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4780ae6e-6373-443c-a642-2a87cc25b93e
📒 Files selected for processing (2)
src/api.tssrc/mappers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mappers.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 comments (5)
src/api.ts (5)
361-361: LGTM!Correctly threads
accountIDtomapThreadsfor sticker reaction URL resolution.
396-396: LGTM!All
mapMessagescall sites consistently passthis.accountID.
685-685: LGTM!Appropriately blocks adding sticker reactions since this PR only adds parsing support.
691-691: LGTM!Consistent with the
addReactionguard.
50-50: Remove the instantiation verification request—this is a library class exported for external consumers.The constructor change to require
accountIDis a breaking change for external code that instantiatesAppleiMessagevia the Texts Platform SDK. However, since this repository contains no internal instantiations ofAppleiMessage(the class is exported as a library reference for external consumers to instantiate), the verification of internal call sites is not applicable. External consumers of this plugin will need to update their instantiation calls to pass theaccountIDparameter.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mappers.ts`:
- Line 680: The Context objects passed into mapThread are missing the required
accountID property; update both direct mapThread calls to include accountID:
this.accountID (use the same pattern as the bulk mapThreads call) so the Context
passed to mapThread contains accountID, e.g., add accountID: this.accountID to
the objects fed into mapThread to match the Context type and the existing
mapThreads usage.
🪄 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: 61245871-69e7-4eff-8188-8b753f74a3d4
📒 Files selected for processing (1)
src/mappers.ts
📜 Review details
🔇 Additional comments (1)
src/mappers.ts (1)
62-63: Nice consistency on sticker reaction image URL mapping.Using
reactionStickerAssetURL(...)in bothassignReactionsand associated-message reaction mapping keeps sticker reaction media resolution uniform across both code paths (Line 62, Line 81, Line 622, Line 637).Also applies to: 65-65, 81-81, 622-625, 637-637
| export const mapAccountLogin = (al: string) => al?.replace(/^(E|P):/, '') | ||
|
|
||
| type Context = { | ||
| accountID: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify every mapThread/mapMessages callsite threads accountID.
rg -n -C3 '\bmapThread\s*\(|\bmapMessages\s*\(' --type tsRepository: beeper/platform-imessage
Length of output: 4617
🏁 Script executed:
# First, check what Context is being passed at the mapThread calls
cat -n src/api.ts | sed -n '220,240p'Repository: beeper/platform-imessage
Length of output: 1200
🏁 Script executed:
# Also check the second mapThread call
cat -n src/api.ts | sed -n '250,270p'Repository: beeper/platform-imessage
Length of output: 983
🏁 Script executed:
# Check the Context type definition
fd -t f -e ts | xargs rg -l 'type Context|interface Context' | head -5Repository: beeper/platform-imessage
Length of output: 82
🏁 Script executed:
# Search for Context type/interface definition
rg -n 'type Context|interface Context' src/mappers.ts -A 15Repository: beeper/platform-imessage
Length of output: 848
🏁 Script executed:
# Also check the mapThread function signature more completely
sed -n '705,720p' src/mappers.tsRepository: beeper/platform-imessage
Length of output: 1145
🏁 Script executed:
# Check mapThreads function and its callers
rg -n 'mapThreads' src/ -B 2 -A 5Repository: beeper/platform-imessage
Length of output: 2258
🏁 Script executed:
# Also verify TypeScript compilation would catch this type error
# by checking how accountID is passed in the correct calls
rg -n 'accountID:' src/ -B 2 -A 2 | head -40Repository: beeper/platform-imessage
Length of output: 2518
Add accountID: this.accountID to Context objects in mapThread calls.
accountID is a required field in the Context type (line 680) and is used at lines 706 and 716 in mapThread. The calls at src/api.ts:224 and src/api.ts:254 do not provide it, causing a type error. The bulk call via mapThreads at src/api.ts:360 shows the correct pattern with accountID: this.accountID. Update both direct mapThread calls to include this field before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mappers.ts` at line 680, The Context objects passed into mapThread are
missing the required accountID property; update both direct mapThread calls to
include accountID: this.accountID (use the same pattern as the bulk mapThreads
call) so the Context passed to mapThread contains accountID, e.g., add
accountID: this.accountID to the objects fed into mapThread to match the Context
type and the existing mapThreads usage.
No description provided.