(2/2) ChatListItem to ReportActionItem V2#57819
Conversation
… - use ReportActionItem in ChatListItem" This reverts commit 9ed2a28.
…x/51296-chat-list-item-v2
…x/51296-chat-list-item-v2
…x/51296-chat-list-item-v2
…x/51296-chat-list-item-v2
|
@luacmartins @allgandalf Regarding this issue #57343 We have this function to determine if an action deleted src/libs/ReportActionsUtils.ts function isDeletedAction(reportAction: OnyxInputOrEntry<ReportAction | OptimisticIOUReportAction>): boolean {
const message = reportAction?.message ?? [];
if (!Array.isArray(message)) {
return message?.html === '' || !!message?.deleted;
}
// A legacy deleted comment has either an empty array or an object with html field with empty string as value
const isLegacyDeletedComment = message.length === 0 || message.at(0)?.html === '';
return isLegacyDeletedComment || !!message.at(0)?.deleted;
}That function is being used in But it will return true when the onyx is not ready, do you have any suggestion to make it more accurate? The inaccurate result will show not found page for a brief moment especially when we clear onyx data / reset first edit: @blazejkustra I've noticed that we have function isDeletedAction(reportAction: OnyxInputOrEntry<ReportAction | OptimisticIOUReportAction>): boolean {
const message = reportAction?.message ?? [];
if (!Array.isArray(message)) {
return (message?.html === '' || !!message?.deleted) && !reportAction?.shouldShow;
}
// A legacy deleted comment has either an empty array or an object with html field with empty string as value
const isLegacyDeletedComment = message.length === 0 || message.at(0)?.html === '';
return (isLegacyDeletedComment || !!message.at(0)?.deleted) && !reportAction?.shouldShow;
} |
I see myself in the git blame, but it was because I created this Onyx type when we were migrating the app to Typescript. As per comment: /** TODO: Not enough context */
shouldShow?: boolean;I don't have context what this property does 😅 |
|
@wildan-m I think that function is fine and I'm not sure that the function itself should be aware of data being available in Onyx or not. Can we prevent calling it outside of the function if Onyx is not ready? |
…x/51296-chat-list-item-v2
|
@luacmartins this property missing from report which will cause #57344 "invoiceReceiver": {
"accountID": 11646283,
"type": "individual"
},If possible, could we align the snapshot's report/reportAction/transaction property with the live data to guarantee consistency and avoid future regressions? |
|
For #57357 I think close action shouldn't be shown in the serch results, since the action will not acually visible, it's meta action like create action So I solve it by if (isDeletedAction(reportAction) || isResolvedActionableWhisper(reportAction) || reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED || isCreatedAction(reportAction)) {
// eslint-disable-next-line no-continue
continue;
}Please let me know if you know other invisible reportaction types |
|
#57363 probably BE, I couldn't reproduce it while offline Kapture.2025-03-06.at.15.51.23.mp4 |
|
Update: BE Pending BE issue |
I got a PR in review to fix this. The idea with the snapshot data is that we send as little as possible back for performance reasons. I don't think we should blindly add all keys to it. |
This seems fine. |
|
I got a PR for this one too #57819 (comment) |
|
Update: WIP BE Pending BE issue |
|
Regarding #57343 isLoadingInitialReportActions is being set to false earlier when open the chatListItem, so it will show |
|
Nice! Just waiting on conflict resolution now |
…x/51296-chat-list-item-v2
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
This was a large PR and the ESLint changes were unrelated to the changes here so we decided to not address those in this PR. |
|
✋ 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/luacmartins in version: 9.1.26-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.26-10 🚀
|
There was a problem hiding this comment.
Having some early returns, which returns null, caused #59971 (comment), because, we have a wrapper around this component which take fixed space, if these items should not be shown in searches, we should filter them from search options.
allgandalf
left a comment
allgandalf
left a comment
There was a problem hiding this comment.
when we clear cache and restart then iouReport will not be returned in Onyx report_ but will be returned in search data
this leads to in Inbox we will not have iouReport => and return false here. this caused #59973
| const {translate} = useLocalize(); | ||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const reportID = report?.reportID; | ||
| const reportID = report?.reportID ?? action?.reportID; |
There was a problem hiding this comment.
@wildan-m wondering why we need action?.reportID here?
|
At the time this pull request was created, the report object is undefined
on the search result page, therefore we fallback to retrieve the reportID
from reportAction instead.
…On Sat, Jun 14, 2025 at 8:58 PM Pujan Shah ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pages/home/report/PureReportActionItem.tsx
<#57819 (comment)>:
> @@ -395,7 +400,7 @@ function PureReportActionItem({
}: PureReportActionItemProps) {
const {translate} = useLocalize();
const {shouldUseNarrowLayout} = useResponsiveLayout();
- const reportID = report?.reportID;
+ const reportID = report?.reportID ?? action?.reportID;
@wildan-m <https://github.com/wildan-m> wondering why we need
action?.reportID here?
—
Reply to this email directly, view it on GitHub
<#57819 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC2MM32UU776OKVIMHEZLB33DQTATAVCNFSM6AAAAABYLJFNXSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSMRYGM3DANBUHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
13 participants
Explanation of Change
Fixed Issues
$ #51296
$ #57340
$ #57344
$ #57347
$ #57352
$ #57357
$ #57363
PROPOSAL: #51296 (comment)
Tests
Pre-condition: Login to account with various chat types
Also Verify these issues resolved:
$ #51296
$ #57340
$ #57344
$ #57347
$ #57352
$ #57357
$ #57363
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))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
Kapture.2025-04-01.at.16.33.39.mp4
Android: mWeb Chrome
Kapture.2025-04-01.at.16.41.06.mp4
iOS: Native
Kapture.2025-04-01.at.16.07.08.mp4
iOS: mWeb Safari
Kapture.2025-04-01.at.15.58.04.mp4
MacOS: Chrome / Safari
Web.Kapture.2025-04-01.at.11.00.29.mp4
MacOS: Desktop
Kapture.2025-04-01.at.16.24.36.mp4