[Suggested Follow-ups][R1] Render follow‑up buttons + optimistic resolution in App#80539
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Question: how should we handle really long questions ? |
melvin-bot
bot
requested review from
joekaufmanexpensify and
mkhutornyi
and removed request for
a team
|
@mkhutornyi 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}
|
|
||
| const isConciergeOptions = isConciergeCategoryOptions(action) || isConciergeDescriptionOptions(action); | ||
| const actionContainsFollowUps = containsActionableFollowUps(action); | ||
| let actionableButtonsNoLines = 1; |
There was a problem hiding this comment.
❌ 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.
There was a problem hiding this comment.
This is just a basic Text prop. The dosc explain it
|
@Expensify/design thoughts about this, this is an edge that we didn't consider for small screens and long followups |
| ![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); |
There was a problem hiding this comment.
❌ 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.
There was a problem hiding this comment.
I think despite duplicating some logic, the current version is more readable and maintainable.
![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: c9eddebc88
ℹ️ 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".
src/libs/actions/Report.ts
Outdated
| if (lastActorAccountID === CONST.ACCOUNT_ID.CONCIERGE && lastActionReportActionID) { | ||
| const resolvedAction = buildOptimisticResolvedFollowups(lastVisibleAction); | ||
| if (resolvedAction) { | ||
| optimisticReportActions[lastActionReportActionID] = resolvedAction; |
There was a problem hiding this comment.
The resolved concierge follow‑up action is now inserted into optimisticReportActions. That same collection is later used to build failureReportActions, which attaches errors to every entry. If the ADD_COMMENT/ADD_ATTACHMENT request fails (offline/timeout) while a follow‑up list is present, the concierge message itself will be marked with a generic “add comment failed” error even though it wasn’t the failed action. Consider excluding the resolved concierge action from the failure error set or handling it separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
0088.failed.to.post.a.message.mov
Added a revert for stripping the pills, should be all g now
|
Hmm why is the label repeated twice in that button example? I think the reality is that we might need to allow these buttons to go multiple lines, but it feels like we should be able to avoid three lines? |
Hey @shawnborton the duplication was just me forcing longer text to showcase the issue, don't mind the content. Right now there is no limit on the number of lines the button can take up. |
This is fine for now, since Concierge is being forced to display only the followups without a prepended message, but in practice this scenario shouldn't happen for real user questions |
|
🚧 @inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
Web works as expected but on native buttons are broken, showing raw html text. I used this prompt: 
|
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Thanks for catching this @mkhutornyi This is fine as well, since the request is very specific about requesting follow-up content, and it’s not something a real user would normally ask. |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #80512 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be04dd8f45
ℹ️ 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".
| // Matches individual <followup><followup-text>...</followup-text></followup> elements | ||
| const followUpTextRegex = /<followup><followup-text>([^<]*)<\/followup-text><\/followup>/gi; | ||
| let match = followUpTextRegex.exec(followupListHtml); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
I think that should be ok, given that our BE generates the pattern and we expect it as is
| // Optimistically update the HTML to mark followup-list as resolved | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, { | ||
| [reportActionID]: resolvedAction, | ||
| }); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Rollback added in src/libs/actions/Report.ts#728 and working as shown here
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
General comment: we should capitalize HTML as that's more consistent with our common convention to use capitalize ID and other acronyms in variable names like reportID.
| 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'; |
There was a problem hiding this comment.
It's a bit odd that we have a destructured named import here and also a globbed named import.
| return null; | ||
| } | ||
|
|
||
| const updatedHtml = html.replace(/<followup-list(\s[^>]*)?>/, '<followup-list selected>'); |
There was a problem hiding this comment.
NAB: a comment on what this line is doing might be helpful
| * @param timezoneParam - The user's timezone | ||
| * @param ancestors - Array of ancestor reports for proper threading | ||
| */ | ||
| function resolveSuggestedFollowup( |
There was a problem hiding this comment.
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 😅
| * @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); |
There was a problem hiding this comment.
NAB:
| const isActionAComment = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT); | |
| const isAddCommentAction = isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT); |
| } | ||
|
|
||
| // 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; |
There was a problem hiding this comment.
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};There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * 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 |
There was a problem hiding this comment.
suggestion: put it in its own file src/libs/ReportActionFollowupUtils/stripFollowupListFromHtml.ts
Beamanator
left a comment
Beamanator
left a comment
There was a problem hiding this comment.
Love the comments @roryabraham 👍
I think we should merge and CP for now and def handle followups in a followup 👍
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@dannymcclain I can get down with something like that! Sounds like we'll do all of that in a follow up? |
hey @dannymcclain do you think this styling should be for the follow up pills or all actionable buttons in concierge chat ? |
|
I kinda think we'd do it for all actions in chat in general. |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 9.3.11-0 🚀
|
|
@jmusial @Beamanator @dubielzyk-expensify Could you please confirm what account should we use for checking this PR. We have such result when using account without + |
|
@jmusial @roryabraham @Beamanator QA team isn’t able to use an Expensifail account for Concierge testing, and unfortunately, testing with a Gmail account didn’t work for us. |
|
@jponikarchuk @IuliiaHerets |
|
@jmusial We still can't check this. I used ttyy@swmansion.com account 
|
|
✅ QA Passed Part 1 SS.-.P1.mp4Part 2 SS.-.P2.mp4 |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
12 participants
Explanation of Change
This PR is a part of
Suggested Follow-upsproject. It allows rendering follow up buttons in a concierge chat and optimistically removes follow up buttons from the message if the button has been clicked or comment posted by a user.Fixed Issues
$ #80512
PROPOSAL:
Tests
Pre requisites:
Have expensifail or swmansion account
Use staging server
Open the app
Enter concierge chat
Find a way to prompt concierge-ai to give you follow up list. Prompts that usually work
Offline tests
N / A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
0088.android.native.mov
Android: mWeb Chrome
0088.android.web.mov
iOS: Native
0088.ios.native.mov
iOS: mWeb Safari
0088.ios.safari.mov
MacOS: Chrome / Safari
0088.web.chrome.mov