Show comment API errors with `OfflineWithFeedback` #10317

marcaaron · 2022-08-09T01:54:58Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/218572

Tests

Test together with https://github.com/Expensify/Web-Expensify/pull/34538
Trigger an error by throwing an ExpError somewhere in the try/catch here
Verify that the error message appears below the chat message
Verify that the sidebar has a red dot (red brick road indicator)
Dismiss the error
Verify the red brick road indicator is removed.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…mponent

marcaaron · 2022-08-09T02:05:57Z

Style on this is looking a bit rough but the opacity and errors are showing up...

To Do:

Fix the styles
Work on getting the red brick road indicators to show up in the sidebar

marcaaron · 2022-08-09T19:19:58Z

src/libs/actions/Report.js

-    Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
-        optimisticReportActionIDs: [],
-    });
-}


This is kind of an MVP solution that I'm removing now. If we leave it in each time the report actions are fetched we will remove any "optimistic actions". That includes any pending actions with errors which breaks the new design. It was always the plan to remove this code so if we end up with a problem related to "stuck actions" then we'll just need to approach it another way. Though, I'm not sure if "stuck comments" are really a thing anymore.

marcaaron · 2022-08-09T21:15:04Z

I think this is almost ready. This is what the errors look like atm:

cc @shawnborton to see if there are any notes!

shawnborton · 2022-08-09T21:31:32Z

Looks pretty good! Make sure the right side of the error block also has 20px padding, so that the right icon aligns well with the content above and below it. Also, is this nested within the message above it? As in, if you hover over the message row with the error, does the row highlight color show behind the error as well?

neil-marcellini

Looks good overall and it works well. I have some questions and a couple things that I think should be changed. As Shawn said the error needs some right margin.

The error row is separate from the comment when highlighted. Personally I think it looks fine.

Also, the tooltip doesn't look quite right

neil-marcellini · 2022-08-09T21:53:40Z

src/components/OfflineWithFeedback.js

+
+    /** Additional style object for the error row */
+    // eslint-disable-next-line react/forbid-prop-types
+    errorStyle: PropTypes.object,


NAB: How about errorRowStyle?

src/libs/OptionsListUtils.js

neil-marcellini · 2022-08-09T22:32:23Z

src/libs/OptionsListUtils.js

@@ -581,7 +600,7 @@ function getOptions(reports, personalDetails, activeReportID, {
        const login = (Str.isValidPhone(searchValue) && !searchValue.includes('+'))
            ? `+${countryCodeByIP}${searchValue}`
            : searchValue;
-        userToInvite = createOption([login], personalDetails, null, reportsWithDraft, {
+        userToInvite = createOption([login], personalDetails, null, reportsWithDraft, reportActions, {


This looks like the case for creating an option when searching for a user to chat with. Are we planning to add a brickRoadIndicator here?

No, but I don't have any specific concerns with passing the reportActions here. We could create a more specific createReportOption() alternatively, but unsure whether it's necessary.

neil-marcellini · 2022-08-09T22:52:27Z

src/libs/actions/Report.js

@@ -59,10 +59,6 @@ const allReports = {};
 let conciergeChatReportID;
 const typingWatchTimers = {};

-// Map of optimistic report action IDs. These should be cleared when replaced by a recent fetch of report history
-// since we will then be up to date and any optimistic actions that are still waiting to be replaced can be removed.
-const optimisticReportActionIDs = {};


NAB: It looks like we weren't really using optimisticReportActionIDs so removing this is fine. How were we using these before?

We were:

adding all optimistic actions clientID to an array on each report object

removing them later when one of these things happens (whichever one happens first):

The real action created in the server shows up in Pusher

The latest actions for the report are fetched

Clearing out the actions was done to take care of an edge case where:

A report comment would fail to be created in the server

No error message would be displayed and there would be no way to dismiss the message

These comments would get "stuck" in a kind of limbo of never being created and unable to be removed

So to counteract that we would clear these out when fetching new actions

Ultimately, I think this is not as much of a problem any more since we are handling the failure cases and giving the user a way to dismiss a message that could not be created for some reason.

Makes sense. Thanks for the explanation.

src/pages/home/report/ReportActionsList.js

marcaaron · 2022-08-10T00:02:46Z

Yeah so I'm not too sure what exactly is happening with the tooltip tbh. Tried switching from margin to padding to see if that helped.

Here's what this looks like with improved spacing on the right side and everything under the "hover"

Unsure if this is a blocker or something we might improve later, but here's some feedback...

It's not entirely clear that the "Close" button will remove the message. Should it say something like "Delete Comment" instead? Do we care?
The x feels a bit far from the error message on desktop and having an x in the bottom right corner sort of betrays my expectations (x usually in the top right or left to close or dismiss something).

marcaaron · 2022-08-10T00:05:53Z

This effect is not as noticeable on mobile sizes ->

neil-marcellini

It's not entirely clear that the "Close" button will remove the message. Should it say something like "Delete Comment" instead? Do we care?

I think it's fine. If this command fails I suspect the error message will give some context that will make it clear that the message was not sent / saved.

The x feels a bit far from the error message on desktop and having an x in the bottom right corner sort of betrays my expectations (x usually in the top right or left to close or dismiss something).

Yeah, it doesn't look super great, but that's only the case for very large screen sizes. I think it looks pretty good on a laptop sized screen or smaller. The hover actions like "copy to clipboard" are also pushed to the far right side and we seem to be okay with that.

Also, the tooltip looks different now and I think it's satisfactory at this point. We can always make small tweaks in the future.

I'll approve but I'm curious to hear what @shawnborton thinks before we merge.

src/components/OptionRow.js

shawnborton · 2022-08-10T15:19:34Z

Both the tooltip and the "x" being far off to the right are totally cool with me, so I'm good with merging this.

marcaaron · 2022-08-10T18:41:35Z

I'm gonna merge this one to unblock @neil-marcellini and @PauloGasparSv. I tested against the staging Web-Expensify and don't think this needs to be on HOLD.

melvin-bot · 2022-08-10T18:41:46Z

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-08-10T18:48:06Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

marcaaron · 2022-08-10T18:48:33Z

Tests passed

Add initial code to render comment errors with OfflineWithFeedback co…

4504555

…mponent

marcaaron self-assigned this Aug 9, 2022

add success data

5d10385

marcaaron added 2 commits August 9, 2022 08:03

Merge remote-tracking branch 'origin' into marcaaron-addCommentPatternB

d10e4c9

Add logic for RBR indicator

05fccfa

neil-marcellini self-requested a review August 9, 2022 19:05

neil-marcellini mentioned this pull request Aug 9, 2022

Use new UpdatePolicyRoomName api command #10237

Merged

93 tasks

Fix styles

0ac1547

marcaaron changed the title ~~[WIP] Add initial code to render comment errors with OfflineWithFeedback~~ Add initial code to render comment errors with OfflineWithFeedback Aug 9, 2022

marcaaron changed the title ~~Add initial code to render comment errors with OfflineWithFeedback~~ [HOLD Web-Expensify 34538] Add initial code to render comment errors with OfflineWithFeedback Aug 9, 2022

marcaaron changed the title ~~[HOLD Web-Expensify 34538] Add initial code to render comment errors with OfflineWithFeedback~~ [HOLD Web-Expensify 34538] Render comment errors with OfflineWithFeedback Aug 9, 2022

marcaaron changed the title ~~[HOLD Web-Expensify 34538] Render comment errors with OfflineWithFeedback~~ [HOLD Web-Expensify 34538] Show comment API errors with OfflineWithFeedback Aug 9, 2022

marcaaron commented Aug 9, 2022

View reviewed changes

marcaaron added 2 commits August 9, 2022 11:08

Remove more optimisticReportActionIDs logic. Fix tests

ee1b25c

only one action to take when tapping the x for now

13e71e2

marcaaron marked this pull request as ready for review August 9, 2022 21:15

marcaaron requested a review from a team as a code owner August 9, 2022 21:15

melvin-bot bot requested review from youssef-lr and removed request for a team August 9, 2022 21:15

shawnborton self-requested a review August 9, 2022 21:31

PauloGasparSv self-requested a review August 9, 2022 21:52

neil-marcellini requested changes Aug 9, 2022

View reviewed changes

Make the error inside the hoverable area

01565c9

neil-marcellini previously approved these changes Aug 10, 2022

View reviewed changes

PauloGasparSv reviewed Aug 10, 2022

View reviewed changes

src/components/OptionRow.js Outdated Show resolved Hide resolved

PauloGasparSv previously approved these changes Aug 10, 2022

View reviewed changes

shawnborton previously approved these changes Aug 10, 2022

View reviewed changes

remove double brackets

0858736

marcaaron dismissed stale reviews from shawnborton, PauloGasparSv, and neil-marcellini via 0858736 August 10, 2022 16:58

marcaaron changed the title ~~[HOLD Web-Expensify 34538] Show comment API errors with OfflineWithFeedback~~ Show comment API errors with OfflineWithFeedback Aug 10, 2022

PauloGasparSv approved these changes Aug 10, 2022

View reviewed changes

neil-marcellini approved these changes Aug 10, 2022

View reviewed changes

marcaaron merged commit e57e113 into main Aug 10, 2022

marcaaron deleted the marcaaron-addCommentPatternB branch August 10, 2022 18:41

melvin-bot bot added the Emergency label Aug 10, 2022

marcaaron removed the Emergency label Aug 10, 2022

OSBotify mentioned this pull request Aug 15, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

marcaaron mentioned this pull request Aug 22, 2023

Remove unused attachment placeholder code #25557

Merged

62 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show comment API errors with `OfflineWithFeedback` #10317

Show comment API errors with `OfflineWithFeedback` #10317

marcaaron commented Aug 9, 2022 •

edited

Loading

marcaaron commented Aug 9, 2022

marcaaron Aug 9, 2022

marcaaron commented Aug 9, 2022

shawnborton commented Aug 9, 2022

neil-marcellini left a comment

neil-marcellini Aug 9, 2022

neil-marcellini Aug 9, 2022

marcaaron Aug 9, 2022

neil-marcellini Aug 9, 2022

marcaaron Aug 9, 2022 •

edited

Loading

neil-marcellini Aug 9, 2022

marcaaron commented Aug 10, 2022

marcaaron commented Aug 10, 2022

neil-marcellini left a comment

shawnborton commented Aug 10, 2022

marcaaron commented Aug 10, 2022

melvin-bot bot commented Aug 10, 2022

OSBotify commented Aug 10, 2022

marcaaron commented Aug 10, 2022

Show comment API errors with OfflineWithFeedback #10317

Show comment API errors with OfflineWithFeedback #10317

Conversation

marcaaron commented Aug 9, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Aug 9, 2022

marcaaron Aug 9, 2022

Choose a reason for hiding this comment

marcaaron commented Aug 9, 2022

shawnborton commented Aug 9, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Aug 9, 2022

Choose a reason for hiding this comment

neil-marcellini Aug 9, 2022

Choose a reason for hiding this comment

marcaaron Aug 9, 2022

Choose a reason for hiding this comment

neil-marcellini Aug 9, 2022

Choose a reason for hiding this comment

marcaaron Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

neil-marcellini Aug 9, 2022

Choose a reason for hiding this comment

marcaaron commented Aug 10, 2022

marcaaron commented Aug 10, 2022

neil-marcellini left a comment

Choose a reason for hiding this comment

shawnborton commented Aug 10, 2022

marcaaron commented Aug 10, 2022

melvin-bot bot commented Aug 10, 2022

OSBotify commented Aug 10, 2022

marcaaron commented Aug 10, 2022

Show comment API errors with `OfflineWithFeedback` #10317

Show comment API errors with `OfflineWithFeedback` #10317

marcaaron commented Aug 9, 2022 •

edited

Loading

marcaaron Aug 9, 2022 •

edited

Loading