Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 36 additions & 5 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@
let currentUserAccountID = -1;
let currentUserEmail: string | undefined;

Onyx.connect({

Check warning on line 285 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.SESSION,
callback: (value) => {
// When signed out, val is undefined
Expand All @@ -295,13 +295,13 @@
},
});

Onyx.connect({

Check warning on line 298 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.CONCIERGE_REPORT_ID,
callback: (value) => (conciergeReportID = value),
});

let preferredSkinTone: number = CONST.EMOJI_DEFAULT_SKIN_TONE;
Onyx.connect({

Check warning on line 304 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PREFERRED_EMOJI_SKIN_TONE,
callback: (value) => {
preferredSkinTone = EmojiUtils.getPreferredSkinToneIndex(value);
Expand All @@ -311,7 +311,7 @@
// map of reportID to all reportActions for that report
const allReportActions: OnyxCollection<ReportActions> = {};

Onyx.connect({

Check warning on line 314 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
callback: (actions, key) => {
if (!key || !actions) {
Expand All @@ -323,14 +323,14 @@
});

let allTransactionViolations: OnyxCollection<TransactionViolations> = {};
Onyx.connect({

Check warning on line 326 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
waitForCollectionCallback: true,
callback: (value) => (allTransactionViolations = value),
});

let allReports: OnyxCollection<Report>;
Onyx.connect({

Check warning on line 333 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -340,7 +340,7 @@

let isNetworkOffline = false;
let networkStatus: NetworkStatus;
Onyx.connect({

Check warning on line 343 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.NETWORK,
callback: (value) => {
isNetworkOffline = value?.isOffline ?? false;
Expand All @@ -349,7 +349,7 @@
});

let allPersonalDetails: OnyxEntry<PersonalDetailsList> = {};
Onyx.connect({

Check warning on line 352 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (value) => {
allPersonalDetails = value ?? {};
Expand All @@ -357,7 +357,7 @@
});

let account: OnyxEntry<Account> = {};
Onyx.connect({

Check warning on line 360 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.ACCOUNT,
callback: (value) => {
account = value ?? {};
Expand All @@ -365,7 +365,7 @@
});

const draftNoteMap: OnyxCollection<string> = {};
Onyx.connect({

Check warning on line 368 in src/libs/actions/Report.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.PRIVATE_NOTES_DRAFT,
callback: (value, key) => {
if (!key) {
Expand Down Expand Up @@ -842,12 +842,43 @@
notifyNewAction(notifyReportID, lastAction?.actorAccountID, lastAction);
}

/** Add an attachment and optional comment. */
function addAttachment(reportID: string, notifyReportID: string, file: FileObject, timezoneParam: Timezone, text = '', shouldPlaySound?: boolean) {

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.

Can you clarify why we removed shouldPlaySound? Is it safe to do so? I think we should avoid changing behavior that isn’t part of the issue’s scope.

@ikevin127 ikevin127 Sep 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sobitneupane That makes sense, just added back shouldPlaySound and applied it as it was applied before (ReportActionCompose.tsx only, otherwise false by default).

I can’t reproduce it consistently, but sometimes when I send an attachment, the sound doesn’t play. Have you noticed this on your side?

I did not encounter this while testing, but could have been related to the removal of the shouldPlaySound, meaning that the sound was always played when the function was called as there was no guard, which could create browser sound-queuing issues. I added tests for the sound play / no play now with the new commit.

if (shouldPlaySound) {
/** Add an attachment with an optional comment to a report */
function addAttachmentWithComment(
reportID: string,
notifyReportID: string,
attachments: FileObject | FileObject[],
text = '',
timezone: Timezone = CONST.DEFAULT_TIME_ZONE,
shouldPlaySound = false,
) {
if (!reportID) {
return;
}

const handlePlaySound = () => {
if (!shouldPlaySound) {
return;
}
playSound(SOUNDS.DONE);
};

// Single attachment
if (!Array.isArray(attachments)) {
addActions(reportID, notifyReportID, timezone, text, attachments);
handlePlaySound();
return;
}
addActions(reportID, notifyReportID, timezoneParam, text, file);

// Multiple attachments - first: combine text + first attachment as a single action
addActions(reportID, notifyReportID, timezone, text, attachments?.at(0));

// Remaining: attachment-only actions (no text duplication)
for (let i = 1; i < attachments?.length; i += 1) {
addActions(reportID, notifyReportID, timezone, '', attachments?.at(i));
}

// Play sound once
handlePlaySound();
}

/** Add a single comment to a report */
Expand Down Expand Up @@ -6106,7 +6137,7 @@
export type {Video, GuidedSetupData, TaskForParameters, IntroSelected};

export {
addAttachment,
addAttachmentWithComment,
addComment,
addPolicyReport,
broadcastUserIsLeavingRoom,
Expand Down
4 changes: 2 additions & 2 deletions src/pages/Share/ShareDetailsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'
import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import useThemeStyles from '@hooks/useThemeStyles';
import {addAttachment, addComment, getCurrentUserAccountID, openReport} from '@libs/actions/Report';
import {addAttachmentWithComment, addComment, getCurrentUserAccountID, openReport} from '@libs/actions/Report';
import {canUseTouchScreen} from '@libs/DeviceCapabilities';
import {getFileName, readFileAsync} from '@libs/fileDownload/FileUtils';
import Navigation from '@libs/Navigation/Navigation';
Expand Down Expand Up @@ -128,7 +128,7 @@ function ShareDetailsPage({
);
}
if (report.reportID) {
addAttachment(report.reportID, report.reportID, file, personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE, message);
addAttachmentWithComment(report.reportID, report.reportID, file, message, personalDetail.timezone);
}

const routeToNavigate = ROUTES.REPORT_WITH_ID.getRoute(reportOrAccountID);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ import ReportTypingIndicator from '@pages/home/report/ReportTypingIndicator';
import type {FileObject} from '@pages/media/AttachmentModalScreen/types';
import {hideEmojiPicker, isActive as isActiveEmojiPickerAction} from '@userActions/EmojiPickerAction';
import {initMoneyRequest, replaceReceipt, setMoneyRequestParticipantsFromReport, setMoneyRequestReceipt} from '@userActions/IOU';
import {addAttachment as addAttachmentReportActions, setIsComposerFullSize} from '@userActions/Report';
import {addAttachmentWithComment, setIsComposerFullSize} from '@userActions/Report';
import Timing from '@userActions/Timing';
import {buildOptimisticTransactionAndCreateDraft} from '@userActions/TransactionEdit';
import {isBlockedFromConcierge as isBlockedFromConciergeUserAction} from '@userActions/User';
Expand Down Expand Up @@ -326,22 +326,7 @@ function ReportActionCompose({
const newCommentTrimmed = newComment.trim();

if (attachmentFileRef.current) {
if (Array.isArray(attachmentFileRef.current)) {
// Handle multiple files
attachmentFileRef.current.forEach((file) => {
addAttachmentReportActions(transactionThreadReportID ?? reportID, reportID, file, personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE, newCommentTrimmed, true);
});
} else {
// Handle single file
addAttachmentReportActions(
transactionThreadReportID ?? reportID,
reportID,
attachmentFileRef.current,
personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE,
newCommentTrimmed,
true,
);
}
addAttachmentWithComment(transactionThreadReportID ?? reportID, reportID, attachmentFileRef.current, newCommentTrimmed, personalDetail.timezone, true);
attachmentFileRef.current = null;
} else {
Performance.markStart(CONST.TIMING.SEND_MESSAGE, {message: newCommentTrimmed});
Expand Down
5 changes: 2 additions & 3 deletions src/pages/settings/AboutPage/ShareLogList/index.native.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React from 'react';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import Navigation from '@libs/Navigation/Navigation';
import {addAttachment} from '@userActions/Report';
import CONST from '@src/CONST';
import {addAttachmentWithComment} from '@userActions/Report';
import ROUTES from '@src/ROUTES';
import BaseShareLogList from './BaseShareLogList';
import type {ShareLogListProps} from './types';
Expand All @@ -14,7 +13,7 @@ function ShareLogList({logSource}: ShareLogListProps) {
return;
}
const src = `file://${logSource}`;
addAttachment(reportID, reportID, {name: filename, source: src, uri: src, type: 'text/plain'} as File, personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE);
addAttachmentWithComment(reportID, reportID, {name: filename, source: src, uri: src, type: 'text/plain'} as File, undefined, personalDetail.timezone);

const routeToNavigate = ROUTES.REPORT_WITH_ID.getRoute(reportID);
Navigation.navigate(routeToNavigate);
Expand Down
5 changes: 2 additions & 3 deletions src/pages/settings/AboutPage/ShareLogList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import React from 'react';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import {readFileAsync} from '@libs/fileDownload/FileUtils';
import Navigation from '@libs/Navigation/Navigation';
import {addAttachment} from '@userActions/Report';
import CONST from '@src/CONST';
import {addAttachmentWithComment} from '@userActions/Report';
import ROUTES from '@src/ROUTES';
import BaseShareLogList from './BaseShareLogList';
import type {ShareLogListProps} from './types';
Expand All @@ -15,7 +14,7 @@ function ShareLogList({logSource}: ShareLogListProps) {
logSource,
filename,
(file) => {
addAttachment(reportID, reportID, file, personalDetail?.timezone ?? CONST.DEFAULT_TIME_ZONE);
addAttachmentWithComment(reportID, reportID, file, undefined, personalDetail?.timezone);
const routeToNavigate = ROUTES.REPORT_WITH_ID.getRoute(reportID);
Navigation.navigate(routeToNavigate);
},
Expand Down
108 changes: 106 additions & 2 deletions tests/actions/ReportTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {WRITE_COMMANDS} from '@libs/API/types';
import HttpUtils from '@libs/HttpUtils';
import {buildNextStep} from '@libs/NextStepUtils';
import {getOriginalMessage} from '@libs/ReportActionsUtils';
import playSound, {SOUNDS} from '@libs/Sound';
import CONST from '@src/CONST';
import OnyxUpdateManager from '@src/libs/actions/OnyxUpdateManager';
import * as PersistedRequests from '@src/libs/actions/PersistedRequests';
Expand Down Expand Up @@ -74,6 +75,12 @@ jest.mock('@hooks/useScreenWrapperTransitionStatus', () => ({
}),
}));

jest.mock('@libs/Sound', () => ({
__esModule: true,
default: jest.fn(),
SOUNDS: {DONE: 'DONE'},
}));

const originalXHR = HttpUtils.xhr;
OnyxUpdateManager();
describe('actions/Report', () => {
Expand Down Expand Up @@ -1078,7 +1085,7 @@ describe('actions/Report', () => {
await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});

const file = new File([''], 'test.txt', {type: 'text/plain'});
Report.addAttachment(REPORT_ID, REPORT_ID, file, CONST.DEFAULT_TIME_ZONE);
Report.addAttachmentWithComment(REPORT_ID, REPORT_ID, file);

// Need the reportActionID to delete the comments
const newComment = PersistedRequests.getAll().at(0);
Expand Down Expand Up @@ -1147,7 +1154,7 @@ describe('actions/Report', () => {

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});

Report.addAttachment(REPORT_ID, REPORT_ID, file, CONST.DEFAULT_TIME_ZONE, 'Attachment with comment');
Report.addAttachmentWithComment(REPORT_ID, REPORT_ID, file, 'Attachment with comment');

// Need the reportActionID to delete the comments
const newComment = PersistedRequests.getAll().at(0);
Expand Down Expand Up @@ -1205,6 +1212,103 @@ 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 () => {
global.fetch = TestHelper.getGlobalFetchMock();
const playSoundMock = playSound as jest.MockedFunction<typeof playSound>;
await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
await waitForBatchedUpdates();

const relevantPromise = new Promise((resolve) => {
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) {
Onyx.disconnect(conn);
resolve(relevant);
}
},
});
});

const REPORT_ID = '1';
const shouldPlaySound = true;
const fileA = new File(['a'], 'a.txt', {type: 'text/plain'});
const fileB = new File(['b'], 'b.txt', {type: 'text/plain'});
const fileC = new File(['c'], 'c.txt', {type: 'text/plain'});

Report.addAttachmentWithComment(REPORT_ID, REPORT_ID, [fileA, fileB, fileC], 'Hello world', CONST.DEFAULT_TIME_ZONE, shouldPlaySound);
const relevant = (await relevantPromise) as OnyxTypes.Request[];

expect(playSoundMock).toHaveBeenCalledTimes(1);
expect(playSoundMock).toHaveBeenCalledWith(SOUNDS.DONE);
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);
});

it('should create attachment only actions 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});
await waitForBatchedUpdates();

const relevantPromise = new Promise((resolve) => {
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) {
Onyx.disconnect(conn);
resolve(relevant);
}
},
});
});

const REPORT_ID = '1';
const shouldPlaySound = true;
const fileA = new File(['a'], 'a.txt', {type: 'text/plain'});
const fileB = new File(['b'], 'b.txt', {type: 'text/plain'});

Report.addAttachmentWithComment(REPORT_ID, REPORT_ID, [fileA, fileB], undefined, CONST.DEFAULT_TIME_ZONE, shouldPlaySound);
const relevant = (await relevantPromise) as OnyxTypes.Request[];

expect(playSoundMock).toHaveBeenCalledTimes(1);
expect(playSoundMock).toHaveBeenCalledWith(SOUNDS.DONE);
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);
});

it('should create attachment only action & not play sound when adding attachment without a comment & shouldPlaySound not passed', async () => {
global.fetch = TestHelper.getGlobalFetchMock();
const playSoundMock = playSound as jest.MockedFunction<typeof playSound>;
await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
await waitForBatchedUpdates();

const relevantPromise = new Promise((resolve) => {
const conn = Onyx.connect({
key: ONYXKEYS.PERSISTED_REQUESTS,
callback: (persisted) => {
const relevant = (persisted ?? []).filter((r) => r?.command === WRITE_COMMANDS.ADD_ATTACHMENT);
if (relevant.length >= 1) {
Onyx.disconnect(conn);
resolve(relevant);
}
},
});
});

const REPORT_ID = '1';
const file = new File(['a'], 'a.txt', {type: 'text/plain'});

Report.addAttachmentWithComment(REPORT_ID, REPORT_ID, file);
const relevant = (await relevantPromise) as OnyxTypes.Request[];

expect(playSoundMock).toHaveBeenCalledTimes(0);
expect(relevant.at(0)?.command).toBe(WRITE_COMMANDS.ADD_ATTACHMENT);
});

it('should not send DeleteComment request and remove any Reactions accordingly', async () => {
global.fetch = TestHelper.getGlobalFetchMock();
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
Expand Down
Loading