[No QA] add two-level gate pattern to ReportNotFoundGuard#87559
Conversation
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.
|
BartekObudzinski
changed the title
BartekObudzinski
changed the title
|
@DylanDylann 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] |
melvin-bot
bot
requested review from
flaviadefaria
and removed request for
a team
|
@codex review |
![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: 863629e452
ℹ️ 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".
| // - deleteTransactionNavigateBackUrl is set (always suppresses not-found), OR | ||
| // - path is valid AND report is not a transaction thread AND (still loading OR report exists) | ||
| const reportClearlyExists = !!reportID || isOptimisticDelete || userLeavingStatus; | ||
| const canSkipInnerGuard = !!deleteTransactionNavigateBackUrl || (!isInvalidReportPath && !mayBeTransactionThread && (isLoading || reportClearlyExists)); |
There was a problem hiding this comment.
canSkipInnerGuard relies on mayBeTransactionThread, but isReportTransactionThread(report) depends on parent report actions that may not be in memory on first render (e.g., deep-linking directly into a transaction thread). In that case this evaluates to false, the fast path returns children, and this component no longer subscribes to parent action/metadata updates that would later reveal the thread is deleted; the Not Found view can therefore be skipped for deleted/inaccessible transaction threads until another unrelated rerender happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This partially duplicates the outer gate, let’s clean it up.
There was a problem hiding this comment.
@BartekObudzinski This is my suggestion
outerGateCondition
const reportExists = !!reportID || isOptimisticDelete || userLeavingStatus;
const shouldShowNotFoundPage= !deleteTransactionNavigateBackUrl && (isInvalidReportPath || (!isLoading && !reportExists));
innerGateCondition
const shouldShowNotFoundPage = !deleteTransactionNavigateBackUrl && isDeletedTransactionThread
There was a problem hiding this comment.
Done. Outer guard now handles invalid path and missing report cases directly with FullPageNotFoundView. Inner guard only mounts for transaction threads and only checks isDeletedTransactionThread. Removed 6 duplicate Onyx subscriptions from the inner guard.
| * The inner gate mounts only when the "not found" path is plausible: | ||
| * the report is missing, the path is invalid, or the report is a transaction | ||
| * thread whose parent action may have been deleted. |
There was a problem hiding this comment.
Also, please update the comment if you agree with my suggestion
There was a problem hiding this comment.
Updated the comment to reflect the new structure.
…ecks deleted transaction thread
|
No new product considerations - removing my assignment and unsubscribing. |
| /** | ||
| * Two-level gate: the outer guard subscribes to lightweight keys and handles | ||
| * all "obvious" not-found cases (invalid path, report missing after load). | ||
| * | ||
| * The inner gate mounts only for transaction threads, where the expensive | ||
| * parentReportMetadata / useParentReportAction subscriptions are needed to | ||
| * detect whether the parent action was deleted. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Two-level gate: the outer guard subscribes to lightweight keys and handles | |
| * all "obvious" not-found cases (invalid path, report missing after load). | |
| * | |
| * The inner gate mounts only for transaction threads, where the expensive | |
| * parentReportMetadata / useParentReportAction subscriptions are needed to | |
| * detect whether the parent action was deleted. | |
| */ | |
| /** | |
| * The outer guard subscribes to lightweight keys and handles | |
| * all "obvious" not-found cases (invalid path, report missing after load). | |
| * | |
| */ |
There was a problem hiding this comment.
The comment for the inner gate has already been commented on below. Don't need to mention it twice
There was a problem hiding this comment.
Done. Removed the inner gate description from the outer guard comment.
| parentReportMetadata, | ||
| isOffline, | ||
| }); | ||
| const isDeletedTransactionThread = isReportTransactionThread(report) && (isParentActionDeleted || isParentActionMissingAfterLoad); |
There was a problem hiding this comment.
| const isDeletedTransactionThread = isReportTransactionThread(report) && (isParentActionDeleted || isParentActionMissingAfterLoad); | |
| const isDeletedTransactionThread = isParentActionDeleted || isParentActionMissingAfterLoad; |
There was a problem hiding this comment.
The inner gate only renders if isReportTransactionThread is true, so the check here is unnecessary
There was a problem hiding this comment.
Removed the redundant check.
| ); | ||
| } | ||
|
|
||
| if (isReportTransactionThread(report)) { |
There was a problem hiding this comment.
We only should render ReportNotFoundInnerGuard if deleteTransactionNavigateBackUrl is false. Then in ReportNotFoundInnerGuard, we should remove deleteTransactionNavigateBackUrl condition
There was a problem hiding this comment.
Done. Moved the deleteTransactionNavigateBackUrl check to the outer guard condition for rendering the inner guard. Removed the subscription and condition from ReportNotFoundInnerGuard.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-04-13.at.16.03.16.movAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
| const isDeletedTransactionThread = isParentActionDeleted || isParentActionMissingAfterLoad; | ||
|
|
||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||
| const shouldShowNotFoundPage = isDeletedTransactionThread; |
There was a problem hiding this comment.
| const isDeletedTransactionThread = isParentActionDeleted || isParentActionMissingAfterLoad; | |
| // eslint-disable-next-line rulesdir/no-negated-variables | |
| const shouldShowNotFoundPage = isDeletedTransactionThread; | |
| // eslint-disable-next-line rulesdir/no-negated-variables | |
| const shouldShowNotFoundPage = isParentActionDeleted || isParentActionMissingAfterLoad; |
|
All good 💯 |
|
🚧 @JS00001 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
5 participants
Explanation of Change
ReportNotFoundGuardpreviously subscribed to expensive Onyx keys (parentReportMetadataviauseOnyxand the full parent report actions collection viauseParentReportAction) on every render, even when the report clearly exists and the "not found" page will never be shown.This change introduces a two-level gate pattern, following the same approach already used by
LinkedActionNotFoundGuardin the codebase. The outer guard subscribes only to lightweight Onyx keys and determines whether the report clearly exists. When it does (and the report is not a transaction thread), children render directly without mounting the expensive subscriptions. The inner guard is a self-contained component that only mounts when the "not found" path is plausible: the report is missing, the path is invalid, or the report is a transaction thread whose parent action may have been deleted. The inner guard re-derives all its state from its own hooks, keeping the interface minimal (onlyreportIDFromPathandchildrenprops).Fixed Issues
$ #87632
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 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari