Sort report actions by created date to link LHN badge to oldest report#90553
Sort report actions by created date to link LHN badge to oldest report#90553MelvinBot wants to merge 10 commits into
Conversation
When pressing an LHN row with an action badge, navigate to the oldest report action with that badge instead of an arbitrary one. Co-authored-by: Aimane Chnaif <aimane-chnaif@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.
|
The ESLint rule prefer-locale-compare-from-context disallows direct localeCompare calls. Since these sort ISO date strings, plain string comparison is equivalent and avoids the lint error. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Fixed the failing ESLint check: replaced |
| .sort((a, b) => { | ||
| const createdA = a?.created ?? ''; | ||
| const createdB = b?.created ?? ''; | ||
| if (createdA < createdB) { | ||
| return -1; | ||
| } | ||
| if (createdA > createdB) { | ||
| return 1; | ||
| } | ||
| return 0; | ||
| }) |
There was a problem hiding this comment.
There's already a well-established sorting function: getSortedReportActions in ReportActionsUtils.ts.
It handles more edge cases than the inline sort (e.g., CREATED actions always first, REPORT_PREVIEW actions last on tie, fallback to reportActionID for stable ordering). Both inline sorts in this PR should be replaced with it:
import {getSortedReportActions} from '@libs/ReportActionsUtils';
// Replace the inline .sort(...).find(...) with:
getSortedReportActions(Object.values(reportActions)).find(...)This keeps the sort ascending (oldest first) which matches the current inline behavior.
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
@MelvinBot add unit tests, Tests / QA Steps, pull main |
…Report Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
Replace inline sorting logic in getIOUReportActionWithBadge and getReasonAndReportActionThatRequiresAttention with the existing getSortedReportActions utility from ReportActionsUtils. Add a unit test verifying that getIOUReportActionWithBadge returns the oldest matching report action when multiple actions have badges. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Done! Here's what was pushed:
AI Tests:
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
1 similar comment
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot revert last commit except tests. You're regressing performance which was already optimized. |
Replace getSortedReportActions (O(n log n)) with a single-pass for-loop that tracks the earliest matching action in both getIOUReportActionWithBadge and getReasonAndReportActionThatRequiresAttention. Keeps the unit test. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Reverted the |
| let actionBadge: ValueOf<typeof CONST.REPORT.ACTION_BADGE> | undefined; | ||
| const reportAction = Object.values(chatReportActions).find((action) => { | ||
| let earliestAction: ReportAction | undefined; | ||
| let earliestBadge: ValueOf<typeof CONST.REPORT.ACTION_BADGE> | undefined; |
There was a problem hiding this comment.
@MelvinBot earliestBadge is redundant variable. actionBadge should be enough.
There was a problem hiding this comment.
Done — removed earliestBadge and assigned directly to actionBadge in the loop. Pushed in 7f35bc5.
Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
![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: 7f35bc5c24
ℹ️ 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 (!earliestAction?.created || (action.created && action.created < earliestAction.created)) { | ||
| earliestAction = action; | ||
| actionBadge = badge; |
There was a problem hiding this comment.
created as oldest in badge action selection
The new earliest-action check in getIOUReportActionWithBadge replaces the current candidate whenever earliestAction.created is missing, because !earliestAction?.created stays true. That means a REPORT_PREVIEW with no created timestamp can be overwritten by later actions and no longer be treated as the oldest candidate. This conflicts with the repo’s ordering convention (getSortedReportActions / isNewerReportAction treat undefined created as oldest), so badge navigation can target a different report action when partially populated/optimistic actions are present.
Useful? React with 👍 / 👎.
| if (!earliestAction?.created || (action.created && action.created < earliestAction.created)) { | ||
| earliestAction = action; |
There was a problem hiding this comment.
created previews as oldest in invoice-room scan
The invoice-room loop uses the same earliest-action condition and has the same ordering regression: once earliestAction.created is undefined, every later matching preview can replace it. Because the app’s shared report-action ordering treats missing created as oldest, this can make the LHN attention link jump to a newer invoice preview instead of the intended oldest one when timestamps are incomplete.
Useful? React with 👍 / 👎.
ReviewThe single-pass loop approach is correct and efficient. Two items to consider: 1. Duplicate earliest-action condition if (!earliestAction?.created || (action.created && action.created < earliestAction.created))Consider extracting a small helper (e.g., 2. Missing test for the invoice room path Otherwise LGTM — the logic is sound and the test setup is thorough. |
- Add isOlderReportAction to ReportActionsUtils as the inverse of isNewerReportAction - Replace inline earliest-action comparisons in ReportWorkflow.ts and ReportUtils.ts - Add test for getReasonAndReportActionThatRequiresAttention invoice room path Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Addressed both review items from the review comment:
|
|
@MelvinBot pull main and fix failing tests |
…Report Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
The createRandomReport function signature requires two arguments (index, chatType). Pass undefined as the chatType for the invoice report test data. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Merged main and fixed the failing typecheck. The |
2 participants
Explanation of Change
When pressing an LHN row with an action badge (e.g., APPROVE, PAY, SUBMIT, FIX), the app navigates to a specific report action. Previously,
getIOUReportActionWithBadgeand the invoice room badge logic usedObject.values(...).find(...)which returned whichever matching action happened to come first in the object — not necessarily the oldest one.This PR uses
getSortedReportActionsfromReportActionsUtilsto sort report actions bycreatedtimestamp (ascending) before calling.find(), so the app always navigates to the oldest report action with the relevant badge.Two places were fixed:
getIOUReportActionWithBadgeinReportWorkflow.ts— handles PAY/APPROVE/SUBMIT badgesReportUtils.ts— handles missing invoice bank account badgesFixed Issues
$ #90275
Tests
Offline tests
QA Steps
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))Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari