-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CP Staging] Revert "Remove withNavigationFallback.js
HOC"
#29844
[CP Staging] Revert "Remove withNavigationFallback.js
HOC"
#29844
Conversation
withNavigationFallback.js
HOC"
@thesahindia 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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're restoring |
Alright, thanks let me find someone for the C+ review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert looks clean to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tienifr i can't delete the message on web
- tap and hold
- delete
Uncaught TypeError: Cannot read properties of undefined (reading '3708590258034326363')
at eval (ReportActionsUtils.js:387:85)
at Array.forEach (<anonymous>)
at Module.getLastVisibleAction (ReportActionsUtils.js:386:1)
at Module.getLastVisibleMessage (ReportUtils.js:1211:27)
at Module.deleteReportComment (Report.js:990:33)
at Object.eval [as onConfirm] (PopoverReportActionContextMenu.js:249:7)
at Object.onConfirm [as onPress] (ConfirmModal.js:85:1)
Screen.Recording.2023-10-18.at.13.58.05.mov
Hmm I think it's another issue because I can still delete it 🤔 . Can you try other messages? |
still no luck, could you retest after merging main? |
Tested again, still can't delete any messages |
…49-ts-migration/withNavigationFallback-hoc
@rushatgabhane since it's late for you @situchan will be completing the checklist |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
I tried what @rushatgabhane told above but can't really reproduce the issue but faced another one instead. Note that he was deleting comment using popover context menu (not the mini one). It did not throw any error or crashed the app but the comment was not deleted until I switched to another report and came back. I think this is out of scope. delete-web-compressed.mov |
@tienifr what do you think about this bug @situchan encountered here? also cc @kacper-mikolajczak I see you encountered this bug before here |
This seems like another deploy blocker with different root cause |
@youssef-lr Yes, I was able to reproduce the resizing issue. Having looked at the errors, I thought it's not related to this PR. |
Yeah my guess as well, I think we have another PR causing this issue. So I'm gonna go ahead and merge this one, thanks everyone! |
withNavigationFallback.js
HOC"withNavigationFallback.js
HOC"
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@youssef-lr Can you please create a GH issue for this? Seems critical. I'd like to investigate. WIP: I'm writing a draft following bug report template. |
…NavigationFallback-hoc Revert "Remove `withNavigationFallback.js` HOC" (cherry picked from commit fbab24b)
sounds good @tienifr, also, this PR just got deployed yesterday, @kacper-mikolajczak ran into this issue while working on it, could it be what's causing that bug on staging? |
Yeah I know. I think we also need to figure out the resizing issue. |
Hmm so strange. I can't reproduce it (i.e. the delete by popover menu issue) anymore on the latest |
I suspect #29784 (merged 15 min ago) might have fixed this as it's the only one related to popover merged within 1 hr. |
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.3.86-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
@situchan @youssef-lr why did we merge this PR? we still can't delete messages on staging with the same error as here #29844 (review) Screen.Recording.2023-10-19.at.03.35.37.mov |
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀
|
@rushatgabhane I can delete message successfully on staging (both from context menu and from mini context menu) Screen.Recording.2023-10-19.at.11.00.57.AM.movScreen.Recording.2023-10-19.at.11.01.31.AM.mov |
OK I can confirm that reverting that PR fixes the issue for me locally |
Looking into it now 🔧 |
Thanks @kacper-mikolajczak! |
🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.3.88-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.88-11 🚀
|
Details
Restore the
withNavigationFallback
to avoid app crashing. Straight revert of PR #29449.Fixed Issues
$ #29839
PROPOSAL: #29839 (comment)
Tests
Offline tests
NA
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
hoc-android-compressed.mov
Android: mWeb Chrome
hoc-chrome-compressed.mov
iOS: Native
hoc-ios-compressed.mov
iOS: mWeb Safari
hoc-safari-compressed.mov
MacOS: Chrome / Safari
hoc-web-compressed.mov
MacOS: Desktop
hoc-desktop-compressed.mov