Update optimistic IDs in sequential queue #27996
Conversation
roryabraham
changed the title
|
Opening this up for review while I work on screenshots. |
|
@s77rt fyi we're looking to get this one reviewed by early tomorrow. If you're able to review today, that would be amazing! If not, no worries, we'll assign an internal engineer tomorrow. |
|
This test is also broken on main atm |
Reviewer Checklist
Screenshots/VideosWebweb-ios.movMobile Web - ChromeMobile Web - SafariDesktopiOSios-web.movAndroid |
|
Test is still failing Screen.Recording.2023-09-28.at.2.38.05.AM-1.mov |
|
Thanks @s77rt – updated and ready for another review. |
|
@roryabraham Still looking broken Screen.Recording.2023-09-28.at.3.11.42.AM-1.mov |
|
Thanks – I was having difficulties with my dev environment but managed to get them sorted so now I'm seeing the same thing, and can see why this isn't working quite yet. Working on a fix now |
|
Exploring a new version of function replaceReportIDInNavigationStack(oldReportID, newReportID) {
const updatedState = deepReplaceKeysAndValues(navigationRef.getRootState(), oldReportID, newReportID);
navigationRef.resetRoot(updatedState);
}Edit: i think it's putting the navigator in the correct state, but it doesn't appear to be actually performing a navigation action. Going to keep noodling with this approach because I think it will end up being more robust. Edit 2: Got a bit further by using |
|
Asked for help in slack: https://expensify.slack.com/archives/C03UK30EA1Z/p1696205332369089 |
|
Going to split this up and move the navigation piece to a separate PR. We can address it in a separate issue. |
|
Created #28572 to address the navigation issues separately. |
|
I have tested on Web <- iOS and iOS <- Web. Let me know if you feel we need more tests here. |
There was a problem hiding this comment.
Feel free to merge once the last review comment is resolved.
| .slice(offset) | ||
| .forEach((persistedRequest, index) => { | ||
| const persistedRequestClone = _.clone(persistedRequest); | ||
| persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); |
There was a problem hiding this comment.
Idea for future improvement: return a boolean if deepReplaceKeysAndValues made any change to the clone, only use PersistedRequests.update if something changed.
There was a problem hiding this comment.
I think Onyx will also check this, but it incurs an extra object comparison so - maybe it can make some difference.
| .forEach((persistedRequest, index) => { | ||
| const persistedRequestClone = _.clone(persistedRequest); | ||
| persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); | ||
| PersistedRequests.update(index + offset, persistedRequestClone); |
There was a problem hiding this comment.
Idea for future improvement: don't run PersistedRequests.update in a loop, because it will result in n calls to Onyx.set instead of potentially just 1 call.
There was a problem hiding this comment.
Probably depends on the number of pending requests. I would keep this how it is but keep an eye on it. Alternatively test it by going offline and triggering a bunch of different action. Then go online and observe how quickly the UI updates (non-scientific approach).
There was a problem hiding this comment.
Another school of thought is that this is gonna be pretty edge case so maybe we can get away with what we've got here..
|
LMK if those sound like good improvements. I imagine they'll be better for performance, so maybe worth including in the V1... |
|
Calling |
| return; | ||
| } | ||
| const oldReportID = request.data?.reportID; | ||
| const offset = isFromSequentialQueue ? 1 : 0; |
There was a problem hiding this comment.
I would explain this with a code comment just a little bit. Something like:
If we are in the sequential queue then we want to ignore the first request in the queue as we know that it's values are correct. If we are not in the sequential queue then all of the requests queued to run after this are safe to modify.
| .forEach((persistedRequest, index) => { | ||
| const persistedRequestClone = _.clone(persistedRequest); | ||
| persistedRequestClone.data = deepReplaceKeysAndValues(persistedRequest.data, oldReportID as string, preexistingReportID); | ||
| PersistedRequests.update(index + offset, persistedRequestClone); |
There was a problem hiding this comment.
Another school of thought is that this is gonna be pretty edge case so maybe we can get away with what we've got here..
|
I'm gonna proceed with merging this as there really aren't any major blocking comments from me. |
|
✋ 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/marcaaron in version: 1.3.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀
|
| const responseOnyxData = response?.onyxData ?? []; | ||
| responseOnyxData.forEach((onyxData) => { | ||
| const key = onyxData.key; | ||
| if (!key.startsWith(ONYXKEYS.COLLECTION.REPORT)) { |
There was a problem hiding this comment.
key could be undefined which caused this issue - #28748 (comment)
Details
This PR adds a new middleware that will:
preexistingReportIDin an API responseFixed Issues
$ https://github.com/Expensify/Expensify/issues/319031
Tests / Offline Tests / QA Steps
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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
iOS.mov
Android