Fix copilot bugs#48517
Conversation
|
I know this is a draft but wanted to call out two bugs not yet listed in the list in the PR description: |
|
added them to the list |
melvin-bot
Bot
requested review from
mkhutornyi
and removed request for
a team
|
@mkhutornyi 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] |
|
cc: @dangrous |
|
@dangrous this was fixed in the PR itself. So I've removed it from the list Screen.Recording.2024-09-09.at.02.20.15.mov |
|
All bugs here fixed - #48517 (comment) cc @mkhutornyi |
|
Bug: app crashes while switching account. And blank account info bug.mov
And console error:
Can we fix these here or out of scope? |
| <Text | ||
| numberOfLines={1} | ||
| style={[styles.textBold, styles.textLarge]} | ||
| style={[styles.textBold, styles.textLarge, styles.flexShrink1]} |
There was a problem hiding this comment.
Can you please link fixed issue per each change (github comment) so I can have context?
There was a problem hiding this comment.
@mkhutornyi you can look at the commits for that - https://github.com/Expensify/App/pull/48517/commits
unrelated to copilot actually. and being fixed in another issue |
| <Text | ||
| numberOfLines={1} | ||
| style={[styles.textBold, styles.textLarge]} | ||
| style={[styles.textBold, styles.textLarge, styles.flexShrink1]} |
|
|
||
| if (!subscriptionPlan && isAppLoading) { | ||
| return <FullScreenLoadingIndicator />; | ||
| } |
There was a problem hiding this comment.
@rushatgabhane I still see not here page for a brief amount of time, is that expected?
Screen.Recording.2024-09-16.at.9.19.42.PM.mov
There was a problem hiding this comment.
I don't think it is worth improving this more. The subscription data is simply not in Onyx, so we have to show not found page
There was a problem hiding this comment.
this is the conditon now. the most we can do is to show a loader when the app is loading.
if (!subscriptionPlan && isAppLoading) {
return <FullScreenLoadingIndicator />;
}
if (!subscriptionPlan) {
return <NotFoundPage />;
}
There was a problem hiding this comment.
Yeah I think that's fine for now
|
|
||
| useEffect(() => { | ||
| setError(null, {}); | ||
| }, [source, avatarID]); |
| }} | ||
| ref={buttonRef} | ||
| interactive={canSwitchAccounts} | ||
| pressDimmingValue={canSwitchAccounts ? undefined : 1} |
| /> | ||
| </MenuItemGroup> | ||
| {isEmptyObject(currentUserPersonalDetails) || accountID === -1 || !avatarURL ? ( | ||
| <AvatarSkeleton size={CONST.AVATAR_SIZE.XLARGE} /> |
| disabled={isAvatarCropModalOpen || (disabled && !enablePreview)} | ||
| disabledStyle={disabledStyle} | ||
| style={[styles.pRelative, avatarStyle, type === CONST.ICON_TYPE_AVATAR && styles.alignSelfCenter]} | ||
| style={[styles.pRelative, avatarStyle]} |
There was a problem hiding this comment.
This causes regression when create new group:
There was a problem hiding this comment.
i think i'll just override the avatar style then
There was a problem hiding this comment.
@rushatgabhane please let me know when it's fixed
|
@rushatgabhane are we going to be able to add the new copilot bugs into this PR as well? I believe you're assigned to all of them currently. Thanks! |
|
yes will do |
Happens on |
….tsx Co-authored-by: Gandalf <whogandalf@gmail.com>
….tsx Co-authored-by: Gandalf <whogandalf@gmail.com>
allgandalf
left a comment
allgandalf
left a comment
There was a problem hiding this comment.
Summary of all the bugs attached to this PR ♻️ :
Fixed:
Partially Fixed:
Comments from @rushatgabhane :
I don't think it is worth improving this more. The subscription data is simply not in Onyx, so we have to show not found page
I agree with this conclusion! There isn't a way that we know of to get the subscription data from Onyx
This bug is more of a BE bug, @dangrous you can refer to @rushatgabhane 's comments here.
But the above 2 shouldn't block this PR, this PR looks great and fixes a lot of bugs, had fun reviewing this one 🚀 all yours @dangrous
|
@dangrous can you delete the checklist from @mkhutornyi here, that is why the checklist workflow is not passing |
|
Running through this, it's all looking good so far! Commenting here when I find bugs so you might get a couple messages. I noticed that we don't differentiate between errors on the new validate code form. I tried adding a copilot from an unvalidated account, and got a 
Although now that I'm writing it I'm realizing it might be something we should fix on the backend? Like if the verification code is correct (for all of these new flows), should that count to validate the account? cc @NikkiWines since you wrote the new code on the backend. |
|
For #48246 I think we might want a fallback for if the delegator account doesn't have a subscription but the delegate does, or vice versa - that is, if you're on the subscription page and swap accounts to a user who doesn't have it. Maybe not found is fine here. |
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
Fallback icon works for now for #48966, I will look into the back end fix.
Other than those commented bugs, I think we're almost there! Let me know what you think about those, and I agree that we can skip the ones @allgandalf called out
|
|
||
| if (!subscriptionPlan && isAppLoading) { | ||
| return <FullScreenLoadingIndicator />; | ||
| } |
There was a problem hiding this comment.
Yeah I think that's fine for now
you're right. we should fix on backend. We have to return onyx data and then frontend can show that. |
|
Ready to merge this if you agree with #48517 (comment) - should we leave as is? Or would it be better to navigate from subscription page back to profile page or something? |
we don't have this pattern to fallback to another route. so i think we should do nothing here 👍 |
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
okay great! BE changes needed as noted to fix the remaining bugs but this gets 99% of them and looks great!
|
✋ 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/dangrous in version: 9.0.38-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.38-4 🚀
|
6 participants
Details
Fixed Issues
$ #48261
#48246
#48252
#48253
#48259
#48244
New issues @mkhutornyi -
#48965
#48966
#48967
#48972
PROPOSAL:
Tests
Pre-req: be on copilot beta
Long name
Subscription page
Avatar error
Interactivity
Avatar skeleton
Selected access level
Fallback avatar
Add yourself as copilot
Learn more link
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-09-08.at.21.49.11.mov
Screen.Recording.2024-09-08.at.21.56.05.mov
Screen.Recording.2024-09-08.at.22.32.04.mov
Screen.Recording.2024-09-08.at.22.54.11.mov
#48965 -
#48966 -
#48967 -
#48972 -
MacOS: Desktop