Handle errors and cancellation when loading media - #3369
Open
FrayxRulez wants to merge 1 commit into
Open
Conversation
An Error response fell through silently, and the cancellation token was never checked, so an abandoned load still appended to the collection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four defects in
MediaCollection.LoadMoreItemsAsync, the incremental source behind the profile media tabs and chat search.1. The
countparameter was shadowed.var count = 0u;inside the lambda hid the parameter, so the requested count was silently discarded. I kept the fixed page size of 50 and renamed the local toadded, because honouring the request would break the primary caller:SearchCollection.UpdateImplprimes the list withLoadMoreItemsAsync(0), which would ask TDLib for zero messages. A viewport-derived count would also fragment a media grid into many tiny searches. A comment now records why the parameter is ignored.2. No error path. An
Errorresponse matched neither branch, so the failure was swallowed. It now has its own branch that logs the message and deliberately leaves_hasMoreand the paging offsets alone, so an error is not mistaken for the end of the list and the next attempt resumes from the same position. (SearchCollection.UpdateImplrelies onHasMoreItemsstaying true for its one-shot retry.)3. The cancellation token was ignored. The token is now checked after the
SendAsyncawait, before anything is appended or any offset is advanced, so an abandoned load leaves the collection untouched and returns 0.4. Per-item
Add— left alone deliberately.MediaCollectionderives fromObservableCollection<T>, soMvxObservableCollection's batching methods are not available here, and adopting them would be a regression rather than an optimisation:AddRangeraises oneNotifyCollectionChangedEventArgs(Add, changedItems: <N items>),SearchCollection.OnCollectionChangedforwards it throughInsertRange, and a multi-itemAddhas no representation in the WinRT vector-change protocol (ItemInserted/ItemRemoved/ItemChanged/Resetall carry a single index), so it degrades toResetat the interop boundary. That would make theListViewrebuild every container instead of doing N inserts. No change made.Not built. A UWP/.NET Native build was not available. The file was checked with
CSharpSyntaxTree.ParseText(...).GetDiagnostics(), which reports no diagnostics — that confirms it still parses and nothing more; it is not a type check.🤖 Generated with Claude Code