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
57 changes: 57 additions & 0 deletions src/libs/ReportActionsFollowupUtils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import CONST from '@src/CONST';
import type {OnyxInputOrEntry, ReportAction} from '@src/types/onyx';
import type {Followup} from './ReportActionsUtils';
import {getReportActionMessage, isActionOfType} from './ReportActionsUtils';

/**
* Checks if a report action contains actionable (unresolved) followup suggestions.
* @param reportAction - The report action to check
* @returns true if the action is an ADD_COMMENT with unresolved followups, false otherwise
*/
function containsActionableFollowUps(reportAction: OnyxInputOrEntry<ReportAction>): boolean {
const isActionAComment = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB:

Suggested change
const isActionAComment = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT);
const isAddCommentAction = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT);

if (!isActionAComment) {
return false;
}
const messageHtml = getReportActionMessage(reportAction)?.html;
if (!messageHtml) {
return false;
}
const followups = parseFollowupsFromHtml(messageHtml);

return !!followups && followups.length > 0;
}

// Matches a <followup-list> HTML element and its entire contents. (<followup-list><followup><followup-text>Question?</followup-text></followup></followup-list>)
const followUpListRegex = /<followup-list(\s[^>]*)?>[\s\S]*?<\/followup-list>/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

My initial reaction is always to grimace when I see regexes used for HTML parsing. This repo is already configured with htmlparser2, so I suggest using that to parse your HTML.

Vibe-coded an example implementation:

diff --git a/src/libs/ReportActionsFollowupUtils.ts b/src/libs/ReportActionsFollowupUtils.ts
index e068a3935da..1eb735c0f61 100644
--- a/src/libs/ReportActionsFollowupUtils.ts
+++ b/src/libs/ReportActionsFollowupUtils.ts
@@ -1,3 +1,4 @@
+import {DomUtils, parseDocument} from 'htmlparser2';
 import CONST from '@src/CONST';
 import type {OnyxInputOrEntry, ReportAction} from '@src/types/onyx';
 import type {Followup} from './ReportActionsUtils';
@@ -22,36 +23,36 @@ function containsActionableFollowUps(reportAction: OnyxInputOrEntry<ReportAction
     return !!followups && followups.length > 0;
 }
 
-// Matches a <followup-list> HTML element and its entire contents. (<followup-list><followup><followup-text>Question?</followup-text></followup></followup-list>)
-const followUpListRegex = /<followup-list(\s[^>]*)?>[\s\S]*?<\/followup-list>/i;
 /**
  * Parses followup data from a <followup-list> HTML element.
  * @param html - The HTML string to parse for <followup-list> elements
  * @returns null if no <followup-list> exists, empty array [] if the followup-list has the 'selected' attribute (resolved state), or an array of followup objects if unresolved
  */
 function parseFollowupsFromHtml(html: string): Followup[] | null {
-    const followupListMatch = html.match(followUpListRegex);
-    if (!followupListMatch) {
+    const doc = parseDocument(html);
+    const followupListElements = DomUtils.getElementsByTagName('followup-list', doc, true);
+
+    if (followupListElements.length === 0) {
         return null;
     }
 
     // There will be only one follow up list
-    const followupListHtml = followupListMatch[0];
-    // Matches a <followup-list> element that has the "selected" attribute (<followup-list selected>...</followup-list>).
-    const followUpSelectedListRegex = /<followup-list[^>]*\sselected[\s>]/i;
-    const hasSelectedAttribute = followUpSelectedListRegex.test(followupListHtml);
-    if (hasSelectedAttribute) {
-        return [];
+    const followupList = followupListElements.at(0);
+    if (!followupList) {
+        return null;
     }
 
-    const followups: Followup[] = [];
-    // Matches individual <followup><followup-text>...</followup-text></followup> elements
-    const followUpTextRegex = /<followup><followup-text>([^<]*)<\/followup-text><\/followup>/gi;
-    let match = followUpTextRegex.exec(followupListHtml);
-    while (match !== null) {
-        followups.push({text: match[1]});
-        match = followUpTextRegex.exec(followupListHtml);
+    // Check if the followup-list has the "selected" attribute (resolved state)
+    if (followupList.attribs && 'selected' in followupList.attribs) {
+        return [];
     }
+
+    // Find all <followup-text> elements within the followup-list
+    const followupTextElements = DomUtils.getElementsByTagName('followup-text', followupList, true);
+    const followups: Followup[] = followupTextElements.map((element) => ({
+        text: DomUtils.textContent(element),
+    }));
+
     return followups;
 }
 export {containsActionableFollowUps, parseFollowupsFromHtml};

Copy link
Contributor

Choose a reason for hiding this comment

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

That would likely also resolve codex's concern. Should be less brittle. I also think it's a bit easier to read and think about. It's unlikely it really matters much but it will probably be faster too.

/**
* Parses followup data from a <followup-list> HTML element.
* @param html - The HTML string to parse for <followup-list> elements
* @returns null if no <followup-list> exists, empty array [] if the followup-list has the 'selected' attribute (resolved state), or an array of followup objects if unresolved
*/
function parseFollowupsFromHtml(html: string): Followup[] | null {
const followupListMatch = html.match(followUpListRegex);
if (!followupListMatch) {
return null;
}

// There will be only one follow up list
const followupListHtml = followupListMatch[0];
// Matches a <followup-list> element that has the "selected" attribute (<followup-list selected>...</followup-list>).
const followUpSelectedListRegex = /<followup-list[^>]*\sselected[\s>]/i;
const hasSelectedAttribute = followUpSelectedListRegex.test(followupListHtml);
if (hasSelectedAttribute) {
return [];
}

const followups: Followup[] = [];
// Matches individual <followup><followup-text>...</followup-text></followup> elements
const followUpTextRegex = /<followup><followup-text>([^<]*)<\/followup-text><\/followup>/gi;
let match = followUpTextRegex.exec(followupListHtml);
Comment on lines +48 to +50

Choose a reason for hiding this comment

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

P2 Badge Accept whitespace/attributes in followup tag parsing

The followup parser only matches the exact pattern <followup><followup-text>…</followup-text></followup> with no whitespace or attributes. If concierge outputs valid HTML with indentation/newlines (e.g., <followup>\n <followup-text>…) or adds attributes on <followup>, the regex will never match, parseFollowupsFromHtml() returns an empty list, and no follow‑up buttons are rendered (plus resolution logic will treat it as “no followups”). This breaks the feature for common formatting variants; consider allowing optional whitespace/attributes or using an HTML parser.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that should be ok, given that our BE generates the pattern and we expect it as is

while (match !== null) {
followups.push({text: match[1]});
match = followUpTextRegex.exec(followupListHtml);
}
return followups;
}
export {containsActionableFollowUps, parseFollowupsFromHtml};
24 changes: 22 additions & 2 deletions src/libs/ReportActionsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@

type MemberChangeMessageElement = MessageTextElement | MemberChangeMessageUserMentionElement | MemberChangeMessageRoomReferenceElement;

type Followup = {
text: string;
};

function isPolicyExpenseChat(report: OnyxInputOrEntry<Report>): boolean {
return report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT || !!(report && typeof report === 'object' && 'isPolicyExpenseChat' in report && report.isPolicyExpenseChat);
}
Expand All @@ -73,7 +77,7 @@
}

let allReportActions: OnyxCollection<ReportActions>;
Onyx.connect({

Check warning on line 80 in src/libs/ReportActionsUtils.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,
waitForCollectionCallback: true,
callback: (actions) => {
Expand All @@ -85,7 +89,7 @@
});

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

Check warning on line 92 in src/libs/ReportActionsUtils.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 @@ -94,13 +98,13 @@
});

let isNetworkOffline = false;
Onyx.connect({

Check warning on line 101 in src/libs/ReportActionsUtils.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: (val) => (isNetworkOffline = val?.isOffline ?? false),
});

let deprecatedCurrentUserAccountID: number | undefined;
Onyx.connect({

Check warning on line 107 in src/libs/ReportActionsUtils.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, value is undefined
Expand Down Expand Up @@ -1729,6 +1733,21 @@
];
}

/**
* Used for generating preview text in LHN and other places where followups should not be displayed.
* Implemented here instead of ReportActionFollowupUtils due to circular ref
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: put it in its own file src/libs/ReportActionFollowupUtils/stripFollowupListFromHtml.ts

* @param html message.html from the report COMMENT actions
* @returns html with the <followup-list> element and its contents stripped out or undefined if html is undefined
*/
function stripFollowupListFromHtml(html?: string): string | undefined {
if (!html) {
return;
}
// Matches a <followup-list> HTML element and its entire contents. (<followup-list><followup><followup-text>Question?</followup-text></followup></followup-list>)
const followUpListRegex = /<followup-list(\s[^>]*)?>[\s\S]*?<\/followup-list>/i;
return html.replace(followUpListRegex, '').trim();
}

function getReportActionHtml(reportAction: PartialReportAction): string {
return getReportActionMessage(reportAction)?.html ?? '';
}
Expand All @@ -1737,7 +1756,7 @@
const message = getReportActionMessage(reportAction);
// Sometime html can be an empty string
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const text = (message?.html || message?.text) ?? '';
const text = stripFollowupListFromHtml(message?.html) || (message?.text ?? '');
return text ? Parser.htmlToText(text) : '';
}

Expand Down Expand Up @@ -3972,6 +3991,7 @@
withDEWRoutedActionsArray,
withDEWRoutedActionsObject,
getReportActionActorAccountID,
stripFollowupListFromHtml,
};

export type {LastVisibleMessage};
export type {LastVisibleMessage, Followup};
98 changes: 94 additions & 4 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@
import processReportIDDeeplink from '@libs/processReportIDDeeplink';
import Pusher from '@libs/Pusher';
import type {UserIsLeavingRoomEvent, UserIsTypingEvent} from '@libs/Pusher/types';
import * as ReportActionsFollowupUtils from '@libs/ReportActionsFollowupUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import {getLastVisibleAction} from '@libs/ReportActionsUtils';
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit odd that we have a destructured named import here and also a globbed named import.

import {updateTitleFieldToMatchPolicy} from '@libs/ReportTitleUtils';
import type {Ancestor, OptimisticAddCommentReportAction, OptimisticChatReport, SelfDMParameters} from '@libs/ReportUtils';
import {
Expand Down Expand Up @@ -280,7 +282,7 @@
/** @deprecated This value is deprecated and will be removed soon after migration. Use the email from useCurrentUserPersonalDetails hook instead. */
let deprecatedCurrentUserLogin: 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 @@ -294,7 +296,7 @@
},
});

Onyx.connect({

Check warning on line 299 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) => (conciergeReportIDOnyxConnect = value),
});
Expand All @@ -302,7 +304,7 @@
// map of reportID to all reportActions for that report
const allReportActions: OnyxCollection<ReportActions> = {};

Onyx.connect({

Check warning on line 307 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 @@ -314,7 +316,7 @@
});

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

Check warning on line 319 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 @@ -323,7 +325,7 @@
});

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

Check warning on line 328 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 @@ -338,7 +340,7 @@
});

let onboarding: OnyxEntry<Onboarding>;
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.NVP_ONBOARDING,
callback: (val) => {
if (Array.isArray(val)) {
Expand Down Expand Up @@ -532,6 +534,39 @@
actionSubscriber.callback(isFromCurrentUser, reportAction);
}

/**
* Builds an optimistic report action with resolved followups (followup-list marked as selected).
* @param reportAction - The report action to check and potentially resolve
* @returns Null if the action doesn't have unresolved followups or the updated report action with resolved followups.
*/
function buildOptimisticResolvedFollowups(reportAction: OnyxEntry<ReportAction>): ReportAction | null {
if (!reportAction) {
return null;
}

const message = ReportActionsUtils.getReportActionMessage(reportAction);
if (!message) {
return null;
}
const html = message?.html ?? '';
const followups = ReportActionsFollowupUtils.parseFollowupsFromHtml(html);

if (!followups || followups.length === 0) {
return null;
}

const updatedHtml = html.replace(/<followup-list(\s[^>]*)?>/, '<followup-list selected>');
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: a comment on what this line is doing might be helpful

return {
...reportAction,
message: [
{
...message,
html: updatedHtml,
},
],
};
}

/**
* Add up to two report actions to a report. This method can be called for the following situations:
*
Expand Down Expand Up @@ -597,7 +632,7 @@
}

// Optimistically add the new actions to the store before waiting to save them to the server
const optimisticReportActions: OnyxCollection<OptimisticAddCommentReportAction> = {};
const optimisticReportActions: OnyxCollection<OptimisticAddCommentReportAction | ReportAction> = {};

// Only add the reportCommentAction when there is no file attachment. If there is both a file attachment and text, that will all be contained in the attachmentAction.
if (text && reportCommentAction?.reportActionID && !file) {
Expand All @@ -606,6 +641,17 @@
if (file && attachmentAction?.reportActionID) {
optimisticReportActions[attachmentAction.reportActionID] = attachmentAction;
}

// Check if the last visible action is from Concierge with unresolved followups
// If so, optimistically resolve them by adding the updated action to optimisticReportActions
const lastVisibleAction = getLastVisibleAction(reportID);
const lastActorAccountID = lastVisibleAction?.actorAccountID;
const lastActionReportActionID = lastVisibleAction?.reportActionID;
const resolvedAction = buildOptimisticResolvedFollowups(lastVisibleAction);
if (lastActorAccountID === CONST.ACCOUNT_ID.CONCIERGE && lastActionReportActionID && resolvedAction) {
optimisticReportActions[lastActionReportActionID] = resolvedAction;
}

const parameters: AddCommentOrAttachmentParams = {
reportID,
reportActionID: file ? attachmentAction?.reportActionID : reportCommentAction?.reportActionID,
Expand Down Expand Up @@ -662,17 +708,15 @@
};
const {lastMessageText = ''} = ReportActionsUtils.getLastVisibleMessage(reportID);
if (lastMessageText) {
const lastVisibleAction = ReportActionsUtils.getLastVisibleAction(reportID);
const lastVisibleActionCreated = lastVisibleAction?.created;
const lastActorAccountID = lastVisibleAction?.actorAccountID;
failureReport = {
lastMessageText,
lastVisibleActionCreated,
lastActorAccountID,
};
}

const failureReportActions: Record<string, OptimisticAddCommentReportAction> = {};
const failureReportActions: Record<string, OptimisticAddCommentReportAction | ReportAction> = {};

for (const [actionKey, action] of Object.entries(optimisticReportActions)) {
failureReportActions[actionKey] = {
Expand All @@ -681,6 +725,10 @@
errors: getMicroSecondOnyxErrorWithTranslationKey('report.genericAddCommentFailureMessage'),
};
}
// In case of error bring back the follow up buttons to the cast comment
if (lastActorAccountID === CONST.ACCOUNT_ID.CONCIERGE && lastActionReportActionID) {
failureReportActions[lastActionReportActionID] = lastVisibleAction;
}

const failureData: Array<OnyxUpdate<typeof ONYXKEYS.COLLECTION.REPORT | typeof ONYXKEYS.COLLECTION.REPORT_ACTIONS>> = [
{
Expand Down Expand Up @@ -6495,6 +6543,46 @@
resolveConciergeOptions(report, notifyReportID, reportActionID, selectedDescription, timezoneParam, 'selectedDescription', ancestors);
}

/**
* Resolves a suggested followup by posting the selected question as a comment
* and optimistically updating the HTML to mark the followup-list as resolved.
* @param report - The report where the action exists
* @param notifyReportID - The report ID to notify for new actions
* @param reportAction - The report action containing the followup-list
* @param selectedFollowup - The followup question selected by the user
* @param timezoneParam - The user's timezone
* @param ancestors - Array of ancestor reports for proper threading
*/
function resolveSuggestedFollowup(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Put the new SuggestedFollowup-related functions in src/libs/actions/Report/SuggestedFollowup.ts. Report.ts is already too big, we should start breaking it up before git blame and syntax highlighting stops working in here 😅

We're trying to enforce an (arbitrary) 4K line limit.

report: OnyxEntry<Report>,
notifyReportID: string | undefined,
reportAction: OnyxEntry<ReportAction>,
selectedFollowup: string,
timezoneParam: Timezone,
ancestors: Ancestor[] = [],
) {
const reportID = report?.reportID;
const reportActionID = reportAction?.reportActionID;

if (!reportID || !reportActionID) {
return;
}

const resolvedAction = buildOptimisticResolvedFollowups(reportAction);

if (!resolvedAction) {
return;
}

// Optimistically update the HTML to mark followup-list as resolved
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {
[reportActionID]: resolvedAction,
});
Comment on lines +6577 to +6580

Choose a reason for hiding this comment

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

P2 Badge Restore followups when addComment fails

resolveSuggestedFollowup() merges the “selected” version of the action into Onyx before the comment write. If the subsequent addComment fails (offline/API error), there is no rollback path in this function to restore the original unresolved <followup-list> state, so the follow‑up buttons disappear even though the follow‑up was never posted. This is user‑visible when the write fails and leaves no way to reselect from the list without reloading. Consider deferring the merge until the optimistic comment succeeds or storing the original action so failure handling can restore it.

Useful? React with 👍 / 👎.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rollback added in src/libs/actions/Report.ts#728 and working as shown here


// Post the selected followup question as a comment
addComment(report, notifyReportID ?? reportID, ancestors, selectedFollowup, timezoneParam);
}

/**
* Enhances existing transaction thread reports with additional context for navigation
*
Expand Down Expand Up @@ -6534,6 +6622,7 @@
broadcastUserIsLeavingRoom,
broadcastUserIsTyping,
buildOptimisticChangePolicyData,
buildOptimisticResolvedFollowups,
clearAddRoomMemberError,
clearAvatarErrors,
clearDeleteTransactionNavigateBackUrl,
Expand Down Expand Up @@ -6596,6 +6685,7 @@
resolveActionableReportMentionWhisper,
resolveConciergeCategoryOptions,
resolveConciergeDescriptionOptions,
resolveSuggestedFollowup,
savePrivateNotesDraft,
saveReportActionDraft,
saveReportDraftComment,
Expand Down
33 changes: 29 additions & 4 deletions src/pages/home/report/PureReportActionItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import Parser from '@libs/Parser';
import Permissions from '@libs/Permissions';
import {getDisplayNameOrDefault} from '@libs/PersonalDetailsUtils';
import {getCleanedTagName, hasDynamicExternalWorkflow, isPolicyAdmin, isPolicyMember, isPolicyOwner} from '@libs/PolicyUtils';
import {containsActionableFollowUps, parseFollowupsFromHtml} from '@libs/ReportActionsFollowupUtils';
import {
extractLinksFromMessageHtml,
getActionableCardFraudAlertMessage,
Expand Down Expand Up @@ -202,6 +203,7 @@ import {
resolveActionableMentionConfirmWhisper,
resolveConciergeCategoryOptions,
resolveConciergeDescriptionOptions,
resolveSuggestedFollowup,
} from '@userActions/Report';
import type {IgnoreDirection} from '@userActions/ReportActions';
import {isAnonymousUser, signOutAndRedirectToSignIn} from '@userActions/Session';
Expand Down Expand Up @@ -875,6 +877,20 @@ function PureReportActionItem({
},
}));
}
const messageHtml = getReportActionMessage(action)?.html;
if (messageHtml && reportActionReport) {
const followups = parseFollowupsFromHtml(messageHtml);
if (followups && followups.length > 0) {
return followups.map((followup) => ({
text: followup.text,
shouldUseLocalization: false,
key: `${action.reportActionID}-followup-${followup.text}`,
onPress: () => {
resolveSuggestedFollowup(reportActionReport, reportID, action, followup.text, personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE);
},
}));
}
}

if (!isActionableWhisper && !isActionableCardFraudAlert(action) && (!isActionableJoinRequest(action) || getOriginalMessage(action)?.choice !== ('' as JoinWorkspaceResolution))) {
return [];
Expand Down Expand Up @@ -1645,6 +1661,14 @@ function PureReportActionItem({
![CONST.MODERATION.MODERATOR_DECISION_APPROVED, CONST.MODERATION.MODERATOR_DECISION_PENDING].some((item) => item === moderationDecision) && !isPendingRemove(action);

const isConciergeOptions = isConciergeCategoryOptions(action) || isConciergeDescriptionOptions(action);
const actionContainsFollowUps = containsActionableFollowUps(action);
Copy link
Contributor

Choose a reason for hiding this comment

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

❌ PERF-13 (docs)

The function parseFollowupsFromHtml(messageHtml) is called twice for the same action: once inside the actionableItemButtons useMemo (line 881) and again in containsActionableFollowUps(action) (line 1665). This causes redundant HTML parsing.

Optimize by computing the followups once and reusing the result:

const actionableItemButtons = useMemo(() => {
    // ... existing code ...
    
    const messageHtml = getReportActionMessage(action)?.html;
    if (messageHtml && reportActionReport) {
        const followups = parseFollowupsFromHtml(messageHtml);
        if (followups && followups.length > 0) {
            return followups.map((followup) => ({
                text: followup.text,
                shouldUseLocalization: false,
                key: `${action.reportActionID}-followup-${followup.text}`,
                onPress: () => {
                    resolveSuggestedFollowup(reportActionReport, reportID, action, followup.text, personalDetail.timezone ?? CONST.DEFAULT_TIME_ZONE);
                },
            }));
        }
    }
    // ... rest of code
}, [...]);

// Later in the component, reuse the result:
const actionContainsFollowUps = actionableItemButtons.length > 0 && getReportActionMessage(action)?.html?.includes("<followup-list");

Or alternatively, compute followups once outside the useMemo and use it in both places.


Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think despite duplicating some logic, the current version is more readable and maintainable.

let actionableButtonsNoLines = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

❌ CONSISTENCY-2 (docs)

The magic numbers 1, 2, and 0 assigned to actionableButtonsNoLines lack clear documentation about their meaning and purpose.

Add a comment explaining what these values represent:

// Controls the number of lines to display in button text:
// - 1: default single line for most buttons
// - 2: multi-line for concierge options
// - 0: no limit for followup buttons (allow full text wrapping)
let actionableButtonsNoLines = 1;
if (isConciergeOptions) {
    actionableButtonsNoLines = 2;
}
if (actionContainsFollowUps) {
    actionableButtonsNoLines = 0;
}

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a basic Text prop. The dosc explain it

if (isConciergeOptions) {
actionableButtonsNoLines = 2;
}
if (actionContainsFollowUps) {
actionableButtonsNoLines = 0;
}
children = (
<MentionReportContext.Provider value={mentionReportContextValue}>
<ShowContextMenuContext.Provider value={contextValue}>
Expand Down Expand Up @@ -1684,13 +1708,14 @@ function PureReportActionItem({
isActionableTrackExpense(action) ||
isConciergeCategoryOptions(action) ||
isConciergeDescriptionOptions(action) ||
isActionableMentionWhisper(action)
isActionableMentionWhisper(action) ||
actionContainsFollowUps
? 'vertical'
: 'horizontal'
}
shouldUseLocalization={!isConciergeOptions}
primaryTextNumberOfLines={isConciergeOptions ? 2 : 1}
textStyles={isConciergeOptions ? styles.textAlignLeft : undefined}
shouldUseLocalization={!isConciergeOptions && !actionContainsFollowUps}
primaryTextNumberOfLines={actionableButtonsNoLines}
textStyles={isConciergeOptions || actionContainsFollowUps ? styles.textAlignLeft : undefined}
/>
)}
</View>
Expand Down
Loading
Loading