refactor: handle delay submissions message help site URL with explain feature#83802
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: ad34bcbf4d
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR implements the handling of the "delay submissions" message for reports submitted via harvesting (delayed submission workflow). When Concierge sends an automated submit message that also has AI reasoning attached, it now shows a restructured UI with the text "submitted via [delay submissions link]. [Explain] ✨" instead of the previous HTML-rendered automaticallySubmitted string.
Changes:
- New
wasSubmittedViaHarvestingprop added toReportActionItemMessageWithExplainto conditionally render a custom "submitted via delay submissions" layout with a clickable link and the Explain feature. - New
submittedViaanddelaySubmissionstranslation keys added across all 10 supported languages. PureReportActionItem.tsxnow passeswasSubmittedViaHarvestingto theReportActionItemMessageWithExplaincomponent when the submitted action was via harvesting.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ReportActionItemMessageWithExplain.tsx |
Adds wasSubmittedViaHarvesting prop and the new "submitted via delay submissions + Explain" rendering branch |
PureReportActionItem.tsx |
Passes wasSubmittedViaHarvesting to the component when the SUBMITTED/SUBMITTED_AND_CLOSED action used harvesting |
en.ts |
Adds submittedVia and delaySubmissions translation keys |
de.ts, es.ts, fr.ts, it.ts, nl.ts, pl.ts, pt-BR.ts, zh-hans.ts |
Same two keys added in each respective language |
ja.ts |
Same keys added, but with incorrect word order (see comments) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
samranahm
changed the title
|
@codex review |
|
@ikevin127 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] |
|
|
||
| /** Original report from which the given reportAction is first created */ | ||
| originalReport: OnyxEntry<Report>; | ||
|
|
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
The wasSubmittedViaHarvesting boolean prop is a configuration flag that causes if/else branching inside the component (line 80), controlling two entirely different rendering strategies. The parent (PureReportActionItem.tsx line 1334) already gates this component behind if (wasSubmittedViaHarvesting), so the prop is always true at the only call site that passes it -- the child re-checks a condition the parent already resolved.
Instead of adding a boolean flag to switch rendering paths, consider extracting the harvesting-specific rendering into its own component (e.g., ReportActionItemHarvestingMessageWithExplain) that handles anchor parsing and the explain link. The parent already knows which variant to render, so it can choose the right component directly:
// In PureReportActionItem.tsx, line 1334:
if (wasSubmittedViaHarvesting) {
children = (
<ReportActionItemHarvestingMessageWithExplain
message={translate('iou.automaticallySubmitted')}
action={action}
childReport={childReport}
originalReport={originalReport}
/>
);
}This keeps each component focused on a single rendering concern and avoids the boolean flag configuration pattern.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e94be02e8
ℹ️ 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".
|
@FitseTLT This one is for you to review 🙌 |
|
@samranahm this approach make things more complicated. Why can't we use |
@FitseTLT |
But what you are doing is not a comprehensive fix. U are almost doing a rebuild for RenderHTML by identifying only anchor tags and rendering it separately. It is not wholistic we don't know what we are expecting on html now or even for the future. U have some options
IN the mean time I will revert ur previous pr to avoid regression issues. |
|
PR is now reverted |
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@FitseTLT PR ready for review. |
Is the test failures not related to this? |
|
Ahh, yes this is related to previous implementation. Let me fix this |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@FitseTLT Please take a look. |
|
sparkle icon to be link Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
@shawnborton can u verify the design in the snapshots here? And also to confirm, in the current implementation, now the sparkle icon is not part of the link, do you agree with that? |
This comment was marked as resolved.
This comment was marked as resolved.
|
Are we talking about this screenshot here? I would think the icon is part of the link personally, but cc @Expensify/design to confirm. |
|
Agree with that |
|
Updated. macOS.Chrome.mp4@FitseTLT Pease take a look. |
|
Looks good to me 👍 |
|
Agree - that looks good to me too |
|
🚧 @marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.3.37-0 🚀
|
|
Hi @samranahm. Our tester submitted the expense more than 12 hours ago, and she is still waiting for it to be auto-submitted. What is the estimated time for the expense to be auto-submitted? Recording.1790.mp4 |
@IuliiaHerets I think it submits at the end of the day but instead you can use this step, it tests the same thing Tests
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.37-10 🚀
|
Explanation of Change
Fixed Issues
$ #82171
PROPOSAL:
Tests
Offline tests
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android.Native.mp4
Android: mWeb Chrome
Android.mWeb.Chrome.mp4
iOS: Native
IOS.Native.mp4
iOS: mWeb Safari
IOS.mWeb.Safari.mp4
MacOS: Chrome / Safari
macOS.Chrome.5.52.25.AM.mp4