Skip to content

fix(images): cancel orphaned thumbnail downloads#4695

Open
NotThatKindOfDrLiz wants to merge 4 commits into
mainfrom
fix/profile-thumbnail-cancellation
Open

fix(images): cancel orphaned thumbnail downloads#4695
NotThatKindOfDrLiz wants to merge 4 commits into
mainfrom
fix/profile-thumbnail-cancellation

Conversation

@NotThatKindOfDrLiz
Copy link
Copy Markdown
Member

@NotThatKindOfDrLiz NotThatKindOfDrLiz commented May 24, 2026

Closes #4692
Closes #4696

Summary

  • replace the last cached_network_image usage with a repo-owned MediaCacheImageProvider backed by MediaCacheManager.cacheFileCancellable
  • cancel in-flight thumbnail downloads when the last image listener is removed while preserving VineCachedImage's public API
  • drop the cached_network_image dependency and refresh regression coverage around avatars, thumbnails, and concurrent image loading

Verification

  • flutter test test/src/media_cache_image_provider_test.dart (from mobile/packages/media_cache)
  • flutter test --no-pub test/widgets/vine_cached_image_test.dart
  • flutter test --no-pub test/widgets/video_thumbnail_widget_test.dart
  • flutter test --no-pub test/widgets/comprehensive_user_avatar_test.dart
  • flutter test --no-pub test/notifications/widgets/notification_avatar_stack_test.dart
  • flutter test --no-pub test/widgets/ios_network_image_deadlock_test.dart
  • flutter test --no-pub test/widgets/profile/profile_tab_thumbnail_test.dart
  • flutter test --no-pub test/widgets/user_avatar_svg_test.dart
  • flutter analyze --no-pub lib/widgets/vine_cached_image.dart packages/media_cache/lib/src/media_cache_image_provider.dart packages/media_cache/test/src/media_cache_image_provider_test.dart test/widgets/ios_network_image_deadlock_test.dart test/widgets/video_thumbnail_widget_test.dart test/widgets/comprehensive_user_avatar_test.dart test/notifications/widgets/notification_avatar_stack_test.dart test/widgets/vine_cached_image_test.dart test/widgets/profile/profile_tab_thumbnail_test.dart

Notes

  • full-repo flutter analyze still reports pre-existing infos in unrelated manual/integration scripts outside this diff; the changed files above analyze cleanly
  • flutter test / flutter analyze without --no-pub can crash in this worktree because Flutter tries to delete ios/Flutter/ephemeral/Packages/.packages; verification used the already-resolved dependency graph to avoid that tooling bug

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@NotThatKindOfDrLiz NotThatKindOfDrLiz marked this pull request as ready for review May 24, 2026 22:32
@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Member

@rabble rabble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one issue that I think should be fixed before merge.

  • mobile/packages/media_cache/lib/src/media_cache_image_provider.dart:149: operator == compares authHeaders by contents with mapEquals, but hashCode hashes the MapEntry objects from authHeaders.entries. MapEntry equality/hash is identity-based, so two providers with equal header maps can compare equal while producing different hash codes. Since this class is an ImageProvider key, that violates the key contract used by Flutter's ImageCache and can lead to duplicate/missed cache entries when callers pass equivalent header maps. Please hash headers by stable key/value pairs, for example sorted entries or Object.hashAllUnordered(entries.map((e) => Object.hash(e.key, e.value))), and add a regression test for equal providers with separate but equal header maps.

Verification I ran locally from mobile/:

  • flutter test packages/media_cache/test/src/media_cache_image_provider_test.dart test/widgets/vine_cached_image_test.dart

Comment thread mobile/lib/widgets/vine_cached_image.dart Outdated
Copy link
Copy Markdown
Contributor

@hm21 hm21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left only one minor nit comment but nothing blocking. I also tested it on android and it works perfectly fine for me.

@NotThatKindOfDrLiz NotThatKindOfDrLiz force-pushed the fix/profile-thumbnail-cancellation branch from b217da0 to 5a6bb6e Compare May 25, 2026 15:18
@NotThatKindOfDrLiz NotThatKindOfDrLiz requested review from hm21 and rabble May 25, 2026 15:19
@github-actions
Copy link
Copy Markdown

Mobile PR Preview

Preview refreshed for 5a6bb6e

Last refresh: 5a6bb6e at 2026-05-25 15:22:53 UTC (preview run)

Property Value
Preview URL https://f3a82eb5.openvine-app.pages.dev
Pages project openvine-app
Preview branch pr-4695
PR branch fix/profile-thumbnail-cancellation
Commit 5a6bb6e

@NotThatKindOfDrLiz
Copy link
Copy Markdown
Member Author

Addressed the review feedback in commit 5a6bb6e.

Changes in this update:

  • fixed MediaCacheImageProvider.hashCode so authHeaders hashes by stable key/value pairs and now matches the existing mapEquals equality semantics
  • added regression coverage for equal providers built from separate-but-equal header maps, plus assertions that cacheKey and scale still distinguish keys
  • extracted the new placeholder and error helpers in VineCachedImage into private widget classes to match the repo widget-composition rule

Local verification on this revision:

  • flutter analyze lib test integration_test packages/media_cache
  • flutter test packages/media_cache/test/src/media_cache_image_provider_test.dart
  • flutter test test/widgets/vine_cached_image_test.dart test/widgets/video_thumbnail_widget_test.dart test/widgets/ios_network_image_deadlock_test.dart
  • the repo pre-push hook also passed its changed-file analyzer/test gate before push

@NotThatKindOfDrLiz
Copy link
Copy Markdown
Member Author

@rabble Can you please re-review and clear your requested changes? Thanks!

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.

fix(images): cancel orphaned thumbnail downloads fix: profile video thumbnails not loading

3 participants