fix: Stale "Review fraudulent charge" task appears on home screen but the fraud flag was already cleared#87717
fix: Stale "Review fraudulent charge" task appears on home screen but the fraud flag was already cleared#87717Uzaifm127 wants to merge 9 commits intoExpensify:mainfrom
Conversation
…e sensitive section
Uzaifm127
changed the title
Uzaifm127
changed the title
…revious possible fraud on API failure on fraud action resolution
…SensitiveCards hook tests so fraud-card expectations align with new unresolved-action guard
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Uzaifm127
changed the title
|
@Krishna2323 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] |
![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: 1a290fedc9
ℹ️ 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 (isCardWithPotentialFraud(card) && card.nameValuePairs?.possibleFraud?.fraudAlertReportID) { | ||
| const fraudAlertReportID = card.nameValuePairs?.possibleFraud?.fraudAlertReportID; | ||
| const hasUnresolvedFraudAction = !!fraudAlertReportID && !!getUnresolvedCardFraudAlertAction(String(fraudAlertReportID)); |
There was a problem hiding this comment.
This filter now depends on getUnresolvedCardFraudAlertAction(...), but the hook only subscribes to ONYXKEYS.CARD_LIST; it does not subscribe to REPORT_ACTIONS. If Home renders before the relevant report actions are loaded, hasUnresolvedFraudAction is false and the fraud task is dropped, and later report-action updates will not trigger a recompute here, so a real unresolved fraud alert can remain hidden until an unrelated card-list change happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done!
I found that it is a real issue, that the useTimeSensitiveCards.ts hook isn't subscribed to report actions but we added the check with report actions.
There was a problem hiding this comment.
@Uzaifm127 I think there's a misunderstanding here. Instead of creating a new selector function and all the extra boilerplate, you could simply subscribe to REPORT_ACTIONS via useOnyx and pass them to getUnresolvedCardFraudAlertAction.
Specifically:
- Add a second optional param (
reportActions) togetUnresolvedCardFraudAlertAction. - Use the passed
reportActionsif available, or fall back to the module-level value. - In
useTimeSensitiveCards, subscribe to the relevant report actions viauseOnyxand pass them in.
This keeps the logic centralized in one function and stays consistent with how the LHN already handles it.
There was a problem hiding this comment.
Krishna, you are correct. Let me clear this further.
If you meant this:
function getUnresolvedCardFraudAlertAction(reportID: string, reportActions?: ReportActions) {
const actions = reportActions ?? getAllReportActions(reportID);
return Object.values(actions).find(
(action) => isActionableCardFraudAlert(action) && !getOriginalMessage(action)?.resolution,
);
}
function useTimeSensitiveCards() {
const [cards] = useOnyx(ONYXKEYS.CARD_LIST);
const [allReportActions] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS);
// In the loop - pass subscribed data to the existing function
const reportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${fraudAlertReportID}`];
const hasUnresolvedFraudAction = !!getUnresolvedCardFraudAlertAction(
String(fraudAlertReportID),
reportActions,
);
}Then, I also thought to subscribe all report actions in the hook but this might decrease performance, because if we subscribe to all report actions via useOnyx as above then the hook will re-render on every report action modification (entire Home time-sensitive section re-renders) but if we go with the current approach then we have a selector in useOnyx which will ensures we only re-render when the fraud resolution actually changes.
Both approaches subscribe to all report actions but the difference is the selector function.
either we can choose DRY or Performance here.
You can correct me if I'm wrong here.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 790f48f80f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return result; | ||
| }; | ||
|
|
||
| const [hasUnresolvedFraudByReport] = useOnyx(ONYXKEYS.COLLECTION.REPORT_ACTIONS, {selector: fraudActionsSelector}, [fraudActionsSelector]); |
There was a problem hiding this comment.
fraudActionsSelector is recreated on every render and then passed as the dependency to useOnyx(...). Since useOnyx invalidates when dependencies change, this subscription will be invalidated on every render; and because the selector builds a fresh result object each time, the hook can keep producing new snapshots and repeatedly re-render the Home section even when the underlying fraud data has not changed. Memoize the selector (e.g., with useCallback) and key dependencies off stable inputs (like report IDs) rather than the selector function reference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Krishna, I don't believe we should use useCallback and useMemo as new React compiler will handle it. WDYT?
Fixed the conflicts.
There was a problem hiding this comment.
Hmm correct, useTimeSensitiveCards is auto-memoized.
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
…bleFraud through FraudAlertContent and preserving resolveFraudAlert rollback support
|
@Uzaifm127 please resolve conflicts. Reviewing now. |
|
@Krishna2323 Fixed the conflicts. |
3 participants
Explanation of Change
This PR updated the fraud card filtering logic in
useTimeSensitiveCardsto only show the "Review fraudulent charge" time-sensitive task when the associated card has an unresolved fraud alert action in the linked workspace chat.Previously, the Home tab fraud task was displayed solely based on card fraud metadata (
card.fraud/possibleFraud) without verifying whether the fraud alert had already been resolved. Because of this, the fraud review task could remain visible even after the fraud flag was cleared and the card was reactivated.This change fixes the issue by validating that the linked fraud alert report still contains an unresolved fraud action before adding the card to the Home screen’s time-sensitive fraud list, ensuring stale fraud review tasks are no longer shown after resolution.
Fixed Issues
$ #85981
PROPOSAL: #85981 (comment)
Tests
Precondition: You must trigger potential fraudulent transaction flag using Expensify card.
Offline tests
Same as Test
QA Steps
Same as Test
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.mov
Android: mWeb Chrome
android-mWeb.mov
iOS: Native
iOS-native.mov
iOS: mWeb Safari
iOS-mWeb.mov
MacOS: Chrome / Safari
macOS.mov