Skip to content

Fix multi-attachment comments rendering as separate messages #82533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nicepopo86-lang wants to merge 1 commit into from
Closed

Fix multi-attachment comments rendering as separate messages #82533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/API/parameters/AddCommentOrAttachmentParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ type AddCommentOrAttachmentParams = {
reportActionID?: string;
commentReportActionID?: string | null;
reportComment?: string;
file?: FileObject;
file?: FileObject | FileObject[];
timezone?: string;
clientCreatedTime?: string;
isOldDotConciergeChat?: boolean;
Expand Down
10 changes: 6 additions & 4 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6296,20 +6296,22 @@ function getPolicyDescriptionText(policy: OnyxEntry<Policy>): string {

function buildOptimisticAddCommentReportAction(
text?: string,
file?: FileObject,
file?: FileObject | FileObject[],
actorAccountID?: number,
createdOffset = 0,
reportID?: string,
reportActionID: string = rand64(),
): OptimisticReportAction {
const commentText = getParsedComment(text ?? '', {reportID});
const attachmentHtml = getUploadingAttachmentHtml(file);
const files = Array.isArray(file) ? file : file ? [file] : [];
const attachmentHtml = files.map((singleFile) => getUploadingAttachmentHtml(singleFile)).join('<br /><br />');

const htmlForNewComment = `${commentText}${commentText && attachmentHtml ? '<br /><br />' : ''}${attachmentHtml}`;
const textForNewComment = Parser.htmlToText(htmlForNewComment);

const isAttachmentOnly = file && !text;
const isAttachmentWithText = !!text && file !== undefined;
const hasAttachment = files.length > 0;
const isAttachmentOnly = hasAttachment && !text;
const isAttachmentWithText = !!text && hasAttachment;
const accountID = actorAccountID ?? currentUserAccountID ?? CONST.DEFAULT_NUMBER_ID;
const delegateAccountDetails = getPersonalDetailByEmail(delegateEmail);

Expand Down
18 changes: 3 additions & 15 deletions src/libs/actions/Report/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ type AddActionsParams = {
timezoneParam: Timezone;
currentUserAccountID: number;
text?: string;
file?: FileObject;
file?: FileObject | FileObject[];
isInSidePanel?: boolean;
pregeneratedResponseParams?: PregeneratedResponseParams;
};
Expand Down Expand Up @@ -828,20 +828,8 @@ function addAttachmentWithComment({
playSound(SOUNDS.DONE);
};

// Single attachment
if (!Array.isArray(attachments)) {
addActions({report, notifyReportID, ancestors, timezoneParam: timezone, currentUserAccountID, text, file: attachments, isInSidePanel});
handlePlaySound();
return;
}

// Multiple attachments - first: combine text + first attachment as a single action
addActions({report, notifyReportID, ancestors, timezoneParam: timezone, currentUserAccountID, text, file: attachments?.at(0), isInSidePanel});

// Remaining: attachment-only actions (no text duplication)
for (let i = 1; i < attachments?.length; i += 1) {
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});

@github-actions github-actions Bot Feb 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 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 files
  • if (file) (line 628) — truthy for [], so it would try to buildOptimisticAddCommentReportAction with an empty array
  • if (text && file) (line 636) — same issue, sets commandName = ADD_TEXT_AND_ATTACHMENT even for empty arrays
  • file ? 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.


// Play sound once
handlePlaySound();
Expand Down
42 changes: 28 additions & 14 deletions src/libs/prepareRequestPayload/index.native.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,37 @@ const prepareRequestPayload: PrepareRequestPayload = (command, data, initiatedOf
}

if (key === 'file' && initiatedOffline) {
const {uri: path = '', source, name, type} = value as File;
if (!source) {
validateFormDataParameter(command, key, value);
formData.append(key, value as string | Blob);
const files = Array.isArray(value) ? value : [value];
return files.reduce<Promise<void>>((chain, fileValue) => {
return chain.then(() => {
const {uri: path = '', source, name, type} = fileValue as File;
if (!source) {
validateFormDataParameter(command, key, fileValue);
formData.append(key, fileValue as string | Blob);
return Promise.resolve();
}

return Promise.resolve();
}
// Use the actual file name if available, otherwise fall back to extracting from path/uri
const fileName = name || (path ? (path.split('/').pop() ?? '') : '') || '';
return readFileAsync(source, fileName, () => {}, undefined, type).then((file) => {
if (!file) {
return;
}
// Use the actual file name if available, otherwise fall back to extracting from path/uri
const fileName = name || (path ? (path.split('/').pop() ?? '') : '') || '';
return readFileAsync(source, fileName, () => {}, undefined, type).then((file) => {
if (!file) {
return;
}

validateFormDataParameter(command, key, file);
formData.append(key, file);
validateFormDataParameter(command, key, file);
formData.append(key, file);
});
});
}, Promise.resolve());
}

if (Array.isArray(value)) {
value.forEach((singleValue) => {
validateFormDataParameter(command, key, singleValue);
formData.append(key, singleValue as string | Blob);
});

return Promise.resolve();
}

validateFormDataParameter(command, key, value);
Expand Down
8 changes: 8 additions & 0 deletions src/libs/prepareRequestPayload/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ const prepareRequestPayload: PrepareRequestPayload = (command, data) => {
continue;
}

if (Array.isArray(value)) {
value.forEach((singleValue) => {
validateFormDataParameter(command, key, singleValue);
formData.append(key, singleValue as string | Blob);
});
continue;
Comment on lines +17 to +22

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve list parameter wire format for non-file arrays

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 👍 / 👎.

}

validateFormDataParameter(command, key, value);
formData.append(key, value as string | Blob);
}
Expand Down
21 changes: 12 additions & 9 deletions tests/actions/ReportTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,7 +1493,7 @@ describe('actions/Report', () => {
TestHelper.expectAPICommandToHaveBeenCalled(WRITE_COMMANDS.DELETE_COMMENT, 0);
});

it('should post text + attachment as first action then attachment only for remaining attachments when adding multiple attachments with a comment', async () => {
it('should post multiple attachments with comment in one AddTextAndAttachment request', async () => {
global.fetch = TestHelper.getGlobalFetchMock();
const playSoundMock = playSound as jest.MockedFunction<typeof playSound>;
await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
Expand All @@ -1503,8 +1503,8 @@ describe('actions/Report', () => {
const conn = Onyx.connect({
key: ONYXKEYS.PERSISTED_REQUESTS,
callback: (persisted) => {
const relevant = (persisted ?? []).filter((r) => r?.command === WRITE_COMMANDS.ADD_ATTACHMENT || r?.command === WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT);
if (relevant.length >= 3) {
const relevant = (persisted ?? []).filter((r) => r?.command === WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT);
if (relevant.length >= 1) {
Onyx.disconnect(conn);
resolve(relevant);
}
Expand Down Expand Up @@ -1533,11 +1533,13 @@ describe('actions/Report', () => {

expect(playSoundMock).toHaveBeenCalledTimes(1);
expect(playSoundMock).toHaveBeenCalledWith(SOUNDS.DONE);
expect(relevant).toHaveLength(1);
expect(relevant.at(0)?.command).toBe(WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT);
expect(relevant.slice(1).every((r) => r.command === WRITE_COMMANDS.ADD_ATTACHMENT)).toBe(true);
expect(Array.isArray(relevant.at(0)?.data?.file)).toBe(true);
expect((relevant.at(0)?.data?.file as File[]).length).toBe(3);
});

it('should create attachment only actions when adding multiple attachments without a comment', async () => {
it('should create one attachment-only action when adding multiple attachments without a comment', async () => {
global.fetch = TestHelper.getGlobalFetchMock();
const playSoundMock = playSound as jest.MockedFunction<typeof playSound>;
await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
Expand All @@ -1547,8 +1549,8 @@ describe('actions/Report', () => {
const conn = Onyx.connect({
key: ONYXKEYS.PERSISTED_REQUESTS,
callback: (persisted) => {
const relevant = (persisted ?? []).filter((r) => r?.command === WRITE_COMMANDS.ADD_ATTACHMENT || r?.command === WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT);
if (relevant.length >= 2) {
const relevant = (persisted ?? []).filter((r) => r?.command === WRITE_COMMANDS.ADD_ATTACHMENT);
if (relevant.length >= 1) {
Onyx.disconnect(conn);
resolve(relevant);
}
Expand All @@ -1575,9 +1577,10 @@ describe('actions/Report', () => {

expect(playSoundMock).toHaveBeenCalledTimes(1);
expect(playSoundMock).toHaveBeenCalledWith(SOUNDS.DONE);
expect(relevant).toHaveLength(1);
expect(relevant.at(0)?.command).toBe(WRITE_COMMANDS.ADD_ATTACHMENT);
expect(relevant.slice(1).every((r) => r.command === WRITE_COMMANDS.ADD_ATTACHMENT)).toBe(true);
expect(relevant.some((r) => r.command === WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT)).toBe(false);
expect(Array.isArray(relevant.at(0)?.data?.file)).toBe(true);
expect((relevant.at(0)?.data?.file as File[]).length).toBe(2);
});

it('should create attachment only action & not play sound when adding attachment without a comment & shouldPlaySound not passed', async () => {
Expand Down
Loading