Chore Uber: Add error handling, update empty list - #71241
Conversation
|
@dukenv0307 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] |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@dukenv0307 can you get to this one today? Thanks! |
|
cc @dannymcclain for confirmation on the error message |
| receiptPartners: { | ||
| uber: { | ||
| employees: { | ||
| [email]: null, |
There was a problem hiding this comment.
This would clear all the data, not just the error, no?
There was a problem hiding this comment.
agree, we just need to clear the errors only
There was a problem hiding this comment.
is it really incorrect to remove the whole data? If we just clear error - user row will be with resent button - is it what we expect? @iwiznia

There was a problem hiding this comment.
It needs to go back to the same state it was before the failure, no?
There was a problem hiding this comment.
prev state was empty object - same as we get after doing this [email]: null,
There was a problem hiding this comment.
It was? Don't we use this for resending invitations too?
In any case if this is correct, the name is not since the method is not clearing the error in an uber employee, it's clearing the whole employee
There was a problem hiding this comment.
hmm - ok we should just remove errors object from screenshot i posted above - is it right?
Yes, on it now |
|
The rest looks good |
|
@narefyev91 confirming you updated the padding to match @dukenv0307's suggestion? CC: @dubielzyk-expensify for a quick look on APAC time, so @dukenv0307 can do a final review. |
|
I don't think we have a standardized thing on these pages funnily enough. We use 20px from the edge usually as our default, so both are technically wrong. Sometimes we add a bit more padding to ensure the text looks balanced. And other screens have 16px etc. Long way of saying that we don't have any super consistent treatment here. I'd probably say leave as is for now or update to 20px. Would be curious to hear if @Expensify/design thinks we should standardize here (could end up with some wonky text if we do though). |
|
Yeah, agree with all of your thinking. I think the most consistent thing to do is maybe keep 20px of padding on the outer-most wrapper. And then the empty state item itself might have another 20px of padding to help with the text wrapping if needed? But when in doubt, a simple 20px of padding is likely the most consistent thing to do. The most ideal thing to do would be to have CSS's equivalent of |
|
@shawnborton @dubielzyk-expensify to summarise - for now we just add 20px padding and that's it? |
# Conflicts: # src/pages/workspace/receiptPartners/WorkspaceReceiptPartnersPage.tsx # src/pages/workspace/receiptPartners/utils.tsx
|
I say do that and then show us how it looks. We can adjust after that if needed. |
|
I also add in current PR logic for show broken connection #71195 @iwiznia @trjExpensify |
|
Can you try adding like 8px of margin above the red text so we get some separation from the UI above it? |
Sure - this is for 20px paddings @shawnborton |
|
Cool, I think that's probably fine? Again, would be nice if the two lines were more balanced but I'm not sure if it's worth the hassle. |
|
Okay, let's see what @dannymcclain thinks, but I wonder if 12px or 16px would be better... it still feels a little crammed in there. |
|
Let's go with
20px looks fine by me. I agree it'd be nice to have them more balanced, but it's not worth creating custom styles here. Let's just stick with 20px. One question though - in your screenshot you're showing the |
|
Mhm, I don't think it's possible. Minimally there's one person on the workspace and connected to U4B. |
|
Ok cool. Just wanted to make sure! |
|
Looks good, thanks everyone. Will do the checklist now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-09-26.at.23.07.08.movAndroid: mWeb ChromeScreen.Recording.2025-09-26.at.23.01.14.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2025-09-26.at.22.59.29.movMacOS: Chrome / SafariScreen.Recording.2025-09-26.at.22.55.09.movMacOS: DesktopScreen.Recording.2025-09-26.at.23.07.45.mov |
| acc[email] = null; | ||
| // Build map for resetting employees on failure with individual errors | ||
| const resetEmployeesOnFailure = emails.reduce<Record<string, {status?: string; errors: Errors}>>((acc, email) => { | ||
| acc[email] = { |
There was a problem hiding this comment.
we should reset the pendingFields in failureData as well.
|
@narefyev91 Almost done, just left 1 comment |
|
✋ 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/iwiznia in version: 9.2.20-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.20-4 🚀
|






Details
Fixed Issues
$ #71196
$ #71100
Tests
Test 1:
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop