-
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
[$500] Thread - App crashes when leaving thread #33009
Comments
👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
|
Triggered auto assignment to @puneetlath ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.App crashes when leaving thread What is the root cause of that problem?in the following line App/src/pages/home/HeaderView.js Line 145 in e7ef4bd
What changes do you think we should make in order to solve the problem?this issue orginates because of the optimistic value we set when leaving a room: App/src/libs/actions/Report.ts Lines 2049 to 2058 in 41471e9
here we dont add the so we have two solutions either to add notificationPreference as hidden: : {
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
reportID,
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS.CLOSED,
chatType: report.chatType,
parentReportID: report.parentReportID,
parentReportActionID: report.parentReportActionID,
policyID: report.policyID,
type: report.type,
notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
}, or we add another condition that checks if the #POC Screen.Recording.2023-12-14.at.1.15.36.AM.mov |
I confirmed that this also happens on production. Not deploy blocker but it would be good to fix asap as it's "crash" In addition, we can add fix some more optimistic data to be consistent with server. Screen.Recording.2023-12-14.at.3.39.42.AM.mov |
@situchan sorry for my late, could you help to check my proposal? ProposalPlease re-state the problem that we are trying to solve in this issue.App crashes when leaving thread What is the root cause of that problem?When leaving the room we don't set notificationPreference in optimistic Data and we using SET method to update thread report to ONYX. It caused notificationPreference is lost in the chat report. What changes do you think we should make in order to solve the problem?we need to add fully optimistic data to match the data from BE. I think we should redirect to the parent report after the user leaving room. In here App/src/libs/actions/Report.ts Line 2117 in b950c1e
we should update like this if (isWorkspaceMemberLeavingWorkspaceRoom) {
const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]);
const chat = ReportUtils.getChatByParticipants(participantAccountIDs);
if (chat?.reportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID));
}
+ return
}
+ if (report?.parentReportID) {
+ Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.parentReportID));
+ }
What alternative solutions did you explore? (Optional)NA |
I was unable to reproduce this on |
@Julesssss constantly reproducible on production Screen.Recording.2023-12-14.at.4.29.46.PM.mov |
Please test on public room |
I can't reproduce this in a 1:1 chat on web or Android. But yeah I can reproduce in a public room. The problem is that a new regression has been introduced for the 1:1 chats, so as much as I'd like to drop this from being a blocker, we'll have to fix it asap. |
Turns out this PR also fixes crashes when leaving threads. cc @Julesssss |
yeah, confirmed PR fixes both issues. |
This was CP'd to staging and released. Removing the label. |
Triggered auto assignment to @dylanexpensify ( |
This comment was marked as off-topic.
This comment was marked as off-topic.
Btw, I already accepted your invite |
@Julesssss @dylanexpensify @situchan this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks! |
Current assignee @situchan is eligible for the Internal assigner, not assigning anyone new. |
Issue not reproducible during KI retests. (Third week) |
Current assignee @situchan is eligible for the External assigner, not assigning anyone new. |
Hey @situchan did that work? |
Issue made internal after my comment so didn't work. |
📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Accepted offer. Thanks |
Not overdue |
paying now |
done |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.12-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Expected Result:
App does not crash
Actual Result:
App crashes
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6312071_1702491924994.crash.mp4
logs.txt
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dylanexpensifyThe text was updated successfully, but these errors were encountered: