Skip to content

Handle errors and cancellation when loading media - #3369

Open
FrayxRulez wants to merge 1 commit into
developfrom
media-collection-loading
Open

Handle errors and cancellation when loading media#3369
FrayxRulez wants to merge 1 commit into
developfrom
media-collection-loading

Conversation

@FrayxRulez

Copy link
Copy Markdown
Collaborator

Four defects in MediaCollection.LoadMoreItemsAsync, the incremental source behind the profile media tabs and chat search.

1. The count parameter 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 to added, because honouring the request would break the primary caller: SearchCollection.UpdateImpl primes the list with LoadMoreItemsAsync(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 Error response matched neither branch, so the failure was swallowed. It now has its own branch that logs the message and deliberately leaves _hasMore and 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.UpdateImpl relies on HasMoreItems staying true for its one-shot retry.)

3. The cancellation token was ignored. The token is now checked after the SendAsync await, 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. MediaCollection derives from ObservableCollection<T>, so MvxObservableCollection's batching methods are not available here, and adopting them would be a regression rather than an optimisation: AddRange raises one NotifyCollectionChangedEventArgs(Add, changedItems: <N items>), SearchCollection.OnCollectionChanged forwards it through InsertRange, and a multi-item Add has no representation in the WinRT vector-change protocol (ItemInserted/ItemRemoved/ItemChanged/Reset all carry a single index), so it degrades to Reset at the interop boundary. That would make the ListView rebuild 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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant