fix: show floating action badge for errored expense not above viewpor… - #97180
fix: show floating action badge for errored expense not above viewpor…#97180Abdulloh0109 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4292a99193
ℹ️ 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".
| // The FIX badge points at the errored child (the just-failed expense), which is the newest action at the | ||
| // bottom of the chat — never above the viewport — so it must surface regardless of scroll position. Green | ||
| // badges stay a "scroll up to your queue" affordance and remain gated to the above-viewport case. | ||
| const shouldShowActionBadge = !isProduction && (isActionBadgeAboveViewport || reportAttributes?.actionBadge === CONST.REPORT.ACTION_BADGE.FIX); |
There was a problem hiding this comment.
Require a target before showing Fix badge
When a report gets a FIX badge from an RBR source that does not provide a reportAction (for example receipt-upload errors leave actionTargetReportActionID undefined in reportAttributes.ts), actionBadgeTargetIndex is -1; this new condition still renders the red Fix pill, but pressing it only reaches the early return in scrollToActionBadgeTarget, leaving a persistent no-op badge. Please keep the unconditional FIX path behind a valid action target.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
4292a99 to
aab7999
Compare
Expensify#96132) The FIX action badge targets the oldest errored child's report-preview action (the just-failed expense), which is the newest action at the bottom of the chat and therefore never above the viewport. The render gate only showed the badge when the target was above the viewport (isActionBadgeAboveViewport), so after a failed submission the badge disappeared instead of showing Fix. Gate the badge on the error status too so it surfaces regardless of scroll position; green Submit/Approve/Pay badges keep the above-viewport behavior.
aab7999 to
63d46b1
Compare
trjExpensify
left a comment
There was a problem hiding this comment.
The floating action badge (FloatingMessageCounter) is only rendered when its target report action is above the viewport (isActionBadgeAboveViewport)
@Expensify/design I'm not sure I quite understand this. Isn't it intentional that the Fix pill should only show for previews in the chat history that are not in the viewport?
|
Yeah I guess I'm not following really either because I thought the whole point of the pill was to draw your attention to things that weren't visible in the viewport? So I'm not sure I totally understand the goal of this fix. |
|
@rojiphil do you agree? |
|
That's my understanding too, Danny. |
Thanks @trjExpensify for raising this. Yeah, that seems intentional as per #86064 where the above-viewport rule was implemented. And the staging version seems to work fine for this as demonstrated below. Looks like the confusion arises from OP test steps where the just-created failing expense’s preview action is the newest action i.e. at the bottom of the chat and the expectation is to see the 96132-001.mp4 |
|
Okay cool, thanks for confirming. Yeah, I think we should do nothing. It doesn't make sense to always show Fix when the preview in question is in the scrollview 👍 |
|
@trjExpensify @rojiphil bumping this — no reply for 4 days. What change do we want on this PR? I just need the call so I can move:
Either works for me. Separately: #96132 was assigned to me, the PR was raised, reviewed and is fully green — the work was done in good faith on an assigned job before we concluded no code change may be needed. Could you confirm how payment is handled in this case? |
|
@Abdulloh0109 I think closing the PR makes sense as the current behaviour is intended. Also, agree with you that the work was done in good faith. @mallenexpensify This is a case where we have to close the job before it is completed. Also, the contributor has drafted the PR and requested a review; I reviewed and tested it after considering the product/design team's input. A 100% compensation seems fair here. |
|
yes, closing this PR. @mallenexpensify can help with the payment side of things on the parent issue. |
Explanation of Change
The floating action badge (
FloatingMessageCounter) is only rendered when its target report action is above the viewport (isActionBadgeAboveViewport). PR #94762 retargeted the FIX badge'sactionTargetReportActionIDto the oldest errored child's report-preview action — the just-failed expense, which is the newest action at the bottom of the chat. That target is never above the viewport, so after a failed submission the badge stopped appearing instead of changing to "Fix".This gates the badge on the error status too, so the red FIX badge surfaces regardless of scroll position. The green SUBMIT/APPROVE/PAY badges keep their existing above-viewport ("scroll up to your queue") behavior.
Fixed Issues
$ #96132
PROPOSAL: #96132 (comment)
Tests
Prerequisite: a workspace chat.
Offline tests
With failing network simulated, the failed expense shows its error state and the floating Fix badge appears; clicking it scrolls to the failed expense.
QA Steps
Prerequisite: a workspace chat.
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
MacOS: Chrome / Safari
622006477-538592c4-e1a9-4729-aa4c-e707f7986b3e.mp4