Fix multi-attachment comments rendering as separate messages#82533
Fix multi-attachment comments rendering as separate messages#82533nicepopo86-lang wants to merge 1 commit intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
|
I have read the CLA Document and I hereby sign the CLA nicepopo86-lang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
|
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| addActions({report, notifyReportID, ancestors, timezoneParam: timezone, currentUserAccountID, text: '', file: attachments?.at(i), isInSidePanel}); | ||
| } | ||
| // Send single or multiple attachments in one action | ||
| addActions({report, notifyReportID, ancestors, timezoneParam: timezone, currentUserAccountID, text, file: attachments, isInSidePanel}); |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
Bug: addActions is not updated to handle FileObject[] but now receives it.
The addActions function (lines 612-806) uses truthiness checks on file throughout:
if (text && \!file)(line 622) — an empty array[]is truthy, so this branch is skipped even when there are no filesif (file)(line 628) — truthy for[], so it would try tobuildOptimisticAddCommentReportActionwith an empty arrayif (text && file)(line 636) — same issue, setscommandName = ADD_TEXT_AND_ATTACHMENTeven for empty arraysfile ?ternaries (lines 688, 692) — would select attachment paths even when there are no actual files
Since attachments from addAttachmentWithComment is typed as FileObject | FileObject[], and the caller can pass an array, addActions needs to be updated to handle arrays properly. At minimum, the truthiness checks should use something like:
const hasFile = Array.isArray(file) ? file.length > 0 : \!\!file;And replace all file / \!file checks with hasFile / \!hasFile.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: e61e1aa40b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Array.isArray(value)) { | ||
| value.forEach((singleValue) => { | ||
| validateFormDataParameter(command, key, singleValue); | ||
| formData.append(key, singleValue as string | Blob); | ||
| }); | ||
| continue; |
There was a problem hiding this comment.
The new generic Array.isArray(value) branch changes every array field from a single form value to repeated keys, not just file. That silently alters the payload format for existing *IDList/*List write commands that currently pass arrays through API.write, while other request builders in this repo still explicitly serialize list params as comma-delimited strings (for example exportSearchItemsToCSV and exportReportToCSV). This mismatch can cause bulk operations to run on an incomplete/incorrect ID set when the backend expects the legacy comma-separated form.
Useful? React with 👍 / 👎.
|
@nicepopo86-lang please familiarize yourself with out contribution guidelines found here. You may not raise a PR until a C+ and an internal engineer has assigned you. |
2 participants
Details
$ #76269.
When sending multiple attachments (with or without text), the client currently loops and sends one request per file. That creates multiple report actions, so the conversation shows separate messages even though the composer preview shows a single grouped message.
This PR keeps changes scoped by:
addActions()callfilepayloads to beFileObject | FileObject[]prepareRequestPayloadweb/native) to append array values (includingfile) into FormDataReportTestexpectations for multi-attachment flowsTesting
I could not run Jest in this environment because the repo requires Node
20.19.5and this runner has Node24.13.0(engine mismatch).