Reverted new Onyx Derived Value VISIBLE_REPORT_ACTIONS, blockers fix - #80343
Conversation
…sible utility function, improving code clarity and maintainability.
…e timing check, ensuring accurate rendering of the ReportActionsSkeletonView.
…handling null values for specific updates and improving code clarity.
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.
|
… and associated errors, improving the accuracy of visibility checks.
… error handling for pending actions, enhancing clarity in the visibility determination process.
…isible-report-actions-derived
…for improved formatting
… retrieval for archived and preview reports
…essage text retrieval
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60fdf98c46
ℹ️ 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".
| const reportID = reportAction.reportID; | ||
| if (!reportID) { | ||
| return false; |
There was a problem hiding this comment.
This new early return makes any action lacking reportID invisible as a “last action.” That’s a regression because several optimistic actions are created without reportID (e.g., buildOptimisticTaskReportAction in ReportUtils), yet they are stored under reportActions_<reportID> and should still count as visible. With this change, doesReportHaveVisibleActions() can incorrectly return false and trigger report deletion/navigation for task flows even though visible actions exist. Consider deriving the reportID from the caller or allowing a fallback similar to getLastVisibleAction’s injection.
Useful? React with 👍 / 👎.
…ack for actions without it, improving the accuracy of visibility determinations.
heyjennahay
left a comment
There was a problem hiding this comment.
Product review note required
|
I'm a little confused here… This PR looks near identical to the PR you say it is reverting. Is this a revert of a revert? Going to add @chuckdries since he reviewed the original and is the internal engineer on the original GH. |
|
Hi @deetergp. Yes, this is a revert of a revert, plus an additional fix for the deployment blockers I listed in the QA header. I have also attached videos in the web section demonstrating that it works on my end, also need to fix those conflicts |
…isible-report-actions-derived
…s and imports across multiple components
…er across various components and hooks for improved report action visibility handling.
…ensuring transactionThreadReportID is safely handled with nullish coalescing.
|
Thanks @mkhutornyi , addressed the bot comments. I also take a look on this skeleton case, but could not reproduce on my side |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743f46250e
ℹ️ 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".
| const reportID = reportAction.reportID; | ||
| if ( | ||
| !shouldReportActionBeVisible(reportAction, reportAction.reportActionID, canUserPerformWriteAction(report, isReportArchived)) || | ||
| !reportID || | ||
| !isReportActionVisible(reportAction, reportID, canUserPerformWriteAction(report, isReportArchived), visibleReportActionsData) || | ||
| isDeletedAction(reportAction) || |
There was a problem hiding this comment.
This new guard skips any report action whose reportID field is absent. ReportAction.reportID is optional (and there are known optimistic/legacy actions without it), so this will silently drop those actions from search results even though the report ID can be derived from the collection key in this loop. Previously these actions were still evaluated for visibility. Consider falling back to key.replace(ONYXKEYS.COLLECTION.REPORT_ACTIONS, '') (or similar) instead of skipping when reportAction.reportID is missing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I thin it's fine.
there are known optimistic/legacy actions without it
Not sure what this case is.
There was a problem hiding this comment.
Fixed by extracting reportID from the collection key once per report (outside the inner loop) and using it as a fallback when reportAction.reportID is missing, ensuring optimistic actions appear immediately in search results instead of being dropped until server confirmation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743f46250e
ℹ️ 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".
| // Reports changed - recompute actions that depend on report existence | ||
| if (reportUpdates) { | ||
| const result: VisibleReportActionsDerivedValue = currentValue ? {...currentValue} : {}; | ||
|
|
There was a problem hiding this comment.
This if (reportUpdates) branch returns early after only recomputing UNREPORTED/MOVED actions, so when a report update and reportAction update land in the same Onyx batch (common when new actions arrive or an action is deleted and the report’s lastMessage fields update), the cache is left stale for all other actions. Because isReportActionVisible trusts cached false/true for non‑pending actions, a server‑deleted action (no pendingAction) can remain visible until another reportActions‑only update occurs. Consider handling reportActionsUpdates even when reportUpdates is present, or merging the branches so visibility is recalculated for updated actions in the same batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by removing the early return and continuing to process reportActionsUpdates after handling report dependent actions, so deleted actions are correctly removed from the cache when both report and reportAction updates arrive in the same batch
…sure correct report retrieval based on reportAction data.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab17cbfad
ℹ️ 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".
| const reportCache = visibleReportActions[reportID]; | ||
| if (!reportCache) { | ||
| return shouldReportActionBeVisible(reportAction, reportAction.reportActionID, canUserPerformWriteAction); | ||
| } | ||
| const staticVisibility = reportCache[reportAction.reportActionID]; |
There was a problem hiding this comment.
This visibility lookup only indexes the derived cache by reportAction.reportActionID. For reports that still have legacy actions keyed by sequenceNumber (the migration case handled by isReportActionDeprecated), the cache entry is stored under the sequenceNumber key, so this lookup misses it and falls back to shouldReportActionBeVisible with the reportActionID. That bypasses the sequenceNumber deprecation check and can re‑surface deprecated actions in the UI. This happens specifically when Onyx still contains sequenceNumber-keyed actions during the migration window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
imo this change is unnecessary
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab17cbfad
ℹ️ 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".
| filteredReportActions = Object.entries(reportActions) | ||
| .filter(([key, reportAction]) => shouldReportActionBeVisible(reportAction, key, canUserPerformWriteAction)) | ||
| .filter(([, reportAction]) => { | ||
| const actionReportID = reportAction?.reportID ?? reportID; | ||
| if (!actionReportID) { | ||
| return false; |
There was a problem hiding this comment.
The previous implementation passed the collection key into shouldReportActionBeVisible, which is required for isReportActionDeprecated() to filter out actions still stored under sequenceNumber during the migration. This new filter path calls isReportActionVisible(reportAction, actionReportID, …) without the key, so shouldReportActionBeVisible (and its deprecation check) only see reportAction.reportActionID. If Onyx still contains actions keyed by sequenceNumber, they will no longer be filtered and can show up as duplicates or out‑of‑order actions in the report view/LHN. Consider threading the entry key through isReportActionVisible or otherwise preserving the sequenceNumber key check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
small edge case fix, added it
…ated report actions based on collectionKey.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7498ee4312
ℹ️ 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".
| const reportCache = visibleReportActions[reportID]; | ||
| if (!reportCache) { | ||
| return shouldReportActionBeVisible(reportAction, reportAction.reportActionID, canUserPerformWriteAction); | ||
| } | ||
| const staticVisibility = reportCache[reportAction.reportActionID]; |
There was a problem hiding this comment.
When reportActions are still keyed by sequenceNumber (the migration path this file explicitly filters), isReportActionVisible looks up and falls back using reportAction.reportActionID instead of the collection key. That means the derived cache won’t match (stored under the sequenceNumber), and the fallback call to shouldReportActionBeVisible won’t trip isReportActionDeprecated, so those deprecated actions can reappear in the UI. This will happen whenever a report still contains sequenceNumber-keyed actions (or any key mismatch between the map key and reportAction.reportActionID).
Useful? React with 👍 / 👎.
|
Could this AI codex reviewer give feedback once, instead giving one suggestion each time? |
I think so. I don't see any PR where codex reviewed multiple suggestions at one time. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
Conflicts |
…isible-report-actions-derived
…ensuring proper deletion of stale entries.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/deetergp in version: 9.3.11-16 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
This reverts commit df8d273, reversing changes made to a5e4a32.
This is a revert of a PR that was rolled back due to deploy blockers. I’m re-opening it with fixes for those blockers.
Explanation of Change
Created a new Onyx Derived Value VISIBLE_REPORT_ACTIONS that pre-computes visibility when data changes:
Use shouldReportActionBeVisible in the derived value computation to pre-calculate visibility
Replace direct calls with derived value lookups across ~13 call sites
Use sourceValues for incremental updates
Static checks move to the derived value. Runtime checks remain in the component.
The optimization applies to ReportActionsView, MoneyRequestReportActionsList, LHN, and other call sites.
Fixed Issues
$ #79500
PROPOSAL: #79500
Tests
Offline tests
QA Steps
Scenario in each ticket in steps to reproduce:
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
80281
Screen.Recording.2026-01-26.at.08.37.55.mov
80293
Screen.Recording.2026-01-26.at.08.48.36.mov
80292
539672031-32c199d1-b0e8-4d9f-8157-d2f7555265d2.mov
80300
Screen.Recording.2026-01-26.at.08.52.32.mov
80302
Screen.Recording.2026-01-26.at.09.50.10.mov
80305
539672031-32c199d1-b0e8-4d9f-8157-d2f7555265d2.mov
80307
Screen.Recording.2026-01-26.at.09.01.03.mov
80308
Screen.Recording.2026-01-26.at.09.06.08.mov
80310
Screen.Recording.2026-01-26.at.10.45.24.mov
80312
Screen.Recording.2026-01-26.at.09.10.08.mov