Show inline Explain link on integration sync failure messages#96424
Show inline Explain link on integration sync failure messages#96424MelvinBot wants to merge 1 commit into
Conversation
Co-authored-by: Marc Glasser <marcaaron@users.noreply.github.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
All of the code checks are now green — no code fix was needed, so I didn't push anything. When you filed this request, the JS checks (ESLint, typecheck, tests, perf) were still queued/pending rather than failed. They've since all completed and passed:
The only remaining ❌ is |
|
@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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
ReviewLooks good. The change faithfully mirrors the export-success path ( Details I verified
Two small things worth confirming before merge:
No blocking concerns on the code itself. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 972b02d80c
ℹ️ 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".
| if (hasReasoning(action)) { | ||
| return ( | ||
| <ReportActionItemMessageWithExplain | ||
| message={message} |
There was a problem hiding this comment.
Strip the sync failure period before appending Explain
When an INTEGRATION_SYNC_FAILED action includes reasoning, this passes the sync-failure HTML directly into ReportActionItemMessageWithExplain. That translation already ends with a period (...workspace settings</a>., and recurrence adds another sentence), while iou.AskToExplain begins with . , so the rendered message becomes settings.. Explain or (Repeated 2 times.). Explain; strip the trailing period in this branch before appending the Explain link, as the receipt-scan-failed path does.
Useful? React with 👍 / 👎.
|
The only failing checks are PR Author Checklist and PR Reviewer Checklist — there's no code fix to push here. Every real CI check (tests, typecheck, ESLint, Oxfmt, build, perf, storybook, etc.) is passing. Both checklist checks fail simply because their checkboxes aren't completed yet:
Nothing to commit — these clear once the checklists are filled out. |
Explanation of Change
Export/sync failure system messages (e.g.
failed to export this report to NetSuite ("NS0196 Sync Error: …")) exposed the right-click "Explain" option but not the inline "Explain" link on hover. The right-click entry is gated onhasReasoning(reportAction), while the inline link only renders when the action's render component wraps its message inReportActionItemMessageWithExplain. TheINTEGRATION_SYNC_FAILEDfailure path routed throughIntegrationSyncFailedMessage.tsx, which rendered plain muted text viaReportActionItemBasicMessage+RenderHTMLand was never wired up for the inline affordance (only the success pathExportIntegrationwas, in commit281da4f7).This change makes
IntegrationSyncFailedMessagemirrorExportIntegration: whenhasReasoning(action)is true it wraps the message inReportActionItemMessageWithExplain(which appends the inline "Explain" link and wires up theexplainhandler), and otherwise falls back to the existingReportActionItemBasicMessage/RenderHTMLrendering.ActionContentRouternow passesoriginalReportthrough so the explain handler has the report context it needs. Net effect: NetSuite/QBO sync-failure messages that carry a reasoning payload now show the inline "Explain" link on hover as a first-class action, matching the export success path.Fixed Issues
$ #96422
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari