[PM-14014] Add toast if initial load is taking a while #1280
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1280 +/- ##
==========================================
- Coverage 89.48% 89.47% -0.02%
==========================================
Files 733 734 +1
Lines 46258 46280 +22
==========================================
+ Hits 41393 41407 +14
- Misses 4865 4873 +8 ☔ View full report in Codecov by Sentry. |
| if #available(iOS 16.0, *) { | ||
| try await Task.sleep(for: .seconds(delay), tolerance: .seconds(1)) | ||
| } else { | ||
| try await Task.sleep(nanoseconds: delay * 1_000_000_000) | ||
| } |
There was a problem hiding this comment.
🎨 Could this be extracted to an extension so we don't need to worry about the specific iOS version each time we need to call this?
There was a problem hiding this comment.
Are you thinking something like a Task.sleep(forSeconds:) extension? 🤔 That could work
There was a problem hiding this comment.
Indeed 😄 . Ideally I would generalize it to different units but I think it'd be much over engineered / YAGNI. So for now having the forSeconds extension will work just fine.
| state.toast = nil | ||
| takingTimeTask.cancel() |
There was a problem hiding this comment.
🤔 From what I can see in the demo video the toast is being shown a little bit longer after the data has been displayed to the user. So in order to have a smoother experience should this be also done directly before state.loadingState = .data(sections)? Thus the toast will go away just a moment before the data is displayed which is how I would imagine the flow.
What do you think?
There was a problem hiding this comment.
It was a debate between doing that and replicating it in each of the catch blocks and the guard else block, or doing a defer; unfortunately with the defer it disappears later, but I went for the DRY option for now since ideally this wouldn't show up beyond that initial load. I can change that, though.
There was a problem hiding this comment.
Just in case I wasn't saying to remove the defer block, just repeat it before the state is updated to improve the UI flow. So the defer will still be useful for catches. I get the point on DRY but in this case it's really tiny code and in the same method so I don't see it much as a code smell. However, I get also the point that this is an edge case so I'll leave it up to you as to whether make the change.
There was a problem hiding this comment.
Ah, that makes sense, and that repetition shouldn't matter. Good call, I can do that (though I'll also probably include a comment to that effect)
There was a problem hiding this comment.
After some investigation, it looks like some of this is also being caused because on login, the Fido2CredentialStore.allCredentials() is called by the SDK, which starts a sync before it starts in the vault list. Then when that sync finishes, the display is updated, but the vault list's sync is still going on, and that's what the toast's timing was based on.
To ameliorate this, I have it dismissing the toast whenever we get data from any sync there as well, but I also don't know how much I like us doing a double-sync like that, especially in the sort of situation we're covering for here, where the network is slow relative to the amount of data; grabbing all of it twice seems very inefficient.
There was a problem hiding this comment.
ℹ️ I've checked this and it's because we're updating the ASStore when replacing all identities here which ends up calling the allCredentials method (btw this additional sync is temporary and should not be necessary after some future changes).
However, in these cases of really slow connection yes there could be a race condition where in needsSync lastSyncTime doesn't get updated fast enough and two syncs end up being performed in parallel which is awful.
♻️ I like your temporal solution for now and IMO we should add a new task to deal with parallel syncs in another PR; which can be a little tricky considering the async nature of the calls.
What do you think?
There was a problem hiding this comment.
I think dealing with the parallel syncs is definitely an issue for a different ticket/PR, though if this is also a temporary state we may just want to drive towards resolving the ASStore update additional sync.
There was a problem hiding this comment.
We cannot change the sync there until some updates are done in the SDK/Server regarding passkeys data, I'll check with the team on the status of that.
There was a problem hiding this comment.
Btw @KatherineInCode is the repetition of the code here not needed anymore with the changes you've made?
There was a problem hiding this comment.
Correct; repeating it here wasn't doing anything useful (because it doesn't happen once the ASStore sync comes back, so the toast hangs out there regardless), but dismissing the toast when we load any data from a sync does.
|
Great job, no security vulnerabilities found in this Pull Request |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-14014
📔 Objective
This adds a toast that appears if the initial load of data takes more than five seconds; this toast will not disappear until the data load finishes. This also means updating the toast to be able to either automatically dismiss (as the previous functionality) or require the calling code to manually dismiss it (as for this situation).
📸 Screenshots
ScreenRecording_01-16-2025.12-32-11_1.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes