Open m-natarajan opened 3 months ago
data after the else of if (checkAndFixConflictingRequest) {
I did add after this condition
Seems like it's working
@huult i thought flushQueue
was doing the same thing, could we use that?
@getusha flushQueue
is not the same as clearQueueOnyxUpdates
that is provided. This is because flushQueue
must complete updating all data before setting queuedOnyxUpdates
to an empty array ([]), and the issue still occurs if we use flushQueue
.
clearQueueOnyxUpdates
simply prevents new data from being updated locally when logout is called. With my approach, we ensure that no new data will be updated after logout is triggered.
So, we can't use flushQueue
and should use clearQueueOnyxUpdates
as I provided.
@huult why do we need to add the code other places other than L227? seems unnecessary to fix the issue
@getusha , I added that because I would like to cover all cases related to the Persist request. If we only focus on the test case we reproduced, then we just need to add L227, which is enough for this issue, and remove the others
There is an issue with @huult's solution: it doesn't work when sign-out is triggered by the backend. This can happen when logging in on a second device. In this case, the LogOut request is never made.
My proposal doesn't have this issue
Note: My alternative solution checks the session to update
queuedOnyxUpdates
when we callflushQueue
, so there is no need to check outside to updatequeuedOnyxUpdates
. This approach is good for all cases that may occur and is not the same as other solutions in other proposals.
I think the comment above relates to a different issue because this case involves a different situation and another RCA. Why was the account logged out after logging into a second device? I am still able to use one account on two devices without auto-logout, so we need to find the RCA for why this account was logged out that is the main task we should focus on. And in this issue, I believe we just need to handle the case where cached data remains after logout.
Anyway, if we want to cover the case with no session, we can use a native alert solution in my proposal.
cc: @getusha
@CyberAndrii i can still see the issue after applying your solution
https://github.com/user-attachments/assets/75258191-e675-45c3-acb9-2897a4fb9c0d
@CyberAndrii i can still see the issue after applying your solution
@getusha that's strange. It works every time for me. Did you correctly place QueuedOnyxUpdates.clear();
in the cleanupSession()
function?
My test branch is https://github.com/CyberAndrii/ExpensifyApp/tree/48427-clear-queued-onyx-updates-on-logout
I think the comment above relates to a different issue because this case involves a different situation and another RCA.
It’s the exact same issue, just with different reproduction steps.
Why was the account logged out after logging into a second device? I am still able to use one account on two devices without auto-logout, so we need to find the RCA for why this account was logged out that is the main task we should focus on.
That's the expected behavior when the user is not validated, in other words, never logged in with a 2FA code before.
thank you, @CyberAndrii for your feedback.
There is an issue with @huult's solution: it doesn't work when sign-out is triggered by the backend
I added alternative solutions to cover all the cases related to switching from offline to online and quickly losing the session after coming online while still having existing queuedOnyxUpdates
data.
//.src/libs/actions/QueuedOnyxUpdates.ts#L8
+ let currentAccountID: number | undefined;
+ Onyx.connect({
+ key: ONYXKEYS.SESSION,
+ callback: (session) => {
+ currentAccountID = session?.accountID;
+ },
+ });
//.src/libs/actions/QueuedOnyxUpdates.ts#L24
function flushQueue(): Promise<void> {
+ if (!currentAccountID) {
+ const preservedKeys: OnyxKey[] = [
+ ONYXKEYS.NVP_TRY_FOCUS_MODE,
+ ONYXKEYS.PREFERRED_THEME,
+ ONYXKEYS.NVP_PREFERRED_LOCALE,
+ ONYXKEYS.SESSION,
+ ONYXKEYS.IS_LOADING_APP,
+ ONYXKEYS.CREDENTIALS,
+ ONYXKEYS.IS_SIDEBAR_LOADED,
+ ];
+ queuedOnyxUpdates = queuedOnyxUpdates.filter((update) => preservedKeys.includes(update.key as OnyxKey));
+ }
return Onyx.update(queuedOnyxUpdates).then(() => {
queuedOnyxUpdates = [];
});
}
cc @getusha
@madmax330, @mallenexpensify, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@madmax330, @mallenexpensify, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!
Any updates @getusha?
@madmax330, @mallenexpensify, @getusha Still overdue 6 days?! Let's take care of this!
@CyberAndrii your main solution isn't working
https://github.com/user-attachments/assets/3bbf4908-ddc6-4587-aca3-13762bd53064
@getusha Edit: my theory is that, for some reason, pending requests made until this line are not getting cancelled for you. And those who already completed at this point will be ignored by my clear()
function. If you run the following code in the console, does the request get cancelled?
const abortController = new AbortController();
fetch('https://new.expensify.com/', {signal: abortController.signal});
abortController.abort();
Also thoughts on the alternative solution? I think it’s better as it doesn't require manually calling clear()
.
@getusha My alternative solution resolves all cases related to not having a session. If we don't have a session, then we shouldn't continue saving the response to Onyx. I believe this resolves the issue.
Triggered auto assignment to @sonialiap (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@sonialiap I'm off the next week, can you please keep an eye on this issue til I'm back? Thx
@getusha Have you decided which way to implement to fix this issue?
@madmax330, @sonialiap, @mallenexpensify, @getusha Huh... This is 4 days overdue. Who can take care of this?
@madmax330, @sonialiap, @mallenexpensify, @getusha 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@getusha bumping for proposal review :)
Not overdue
@madmax330, @sonialiap, @mallenexpensify, @getusha 10 days overdue. Is anyone even seeing these? Hello?
Reviewing
@huult's proposal looks good to me and the alternative solution works well, let's go with that. 🎀 👀 🎀 C+ Reviewed!
Current assignee @madmax330 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@madmax330 Waiting for your assignment
📣 @huult 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: Reproducible in staging?: Needs Reproduction Reproducible in production?: Needs Reproduction 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: @mallenexpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725058768402589
Deliverable
Provide reliable reproduction steps and a video for the below bug
Action Performed:
Expected Result:
No report that is not associated with second account should not be shown in search results.
Actual Result:
Old chats from Account A in show in search.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @getusha