Skip to content

feat(mobile): Prevent premature image cache eviction when higher image loading is enabled #26208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mertalev merged 2 commits into from
Feb 25, 2026
Merged

feat(mobile): Prevent premature image cache eviction when higher image loading is enabled #26208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d873d43
feat(mobile): enhance image loading with error handling and eviction …
LeLunZ Feb 14, 2026
00e5022
pull request suggestions: remove log and remove useless local impleme…
LeLunZ Feb 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions mobile/lib/presentation/widgets/images/image_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ mixin CancellableImageProviderMixin<T extends Object> on CancellableImageProvide
return null;
}

Stream<ImageInfo> loadRequest(ImageRequest request, ImageDecoderCallback decode) async* {
Stream<ImageInfo> loadRequest(ImageRequest request, ImageDecoderCallback decode, {bool evictOnError = true}) async* {
if (isCancelled) {
this.request = null;
PaintingBinding.instance.imageCache.evict(this);
Expand All @@ -57,14 +57,19 @@ mixin CancellableImageProviderMixin<T extends Object> on CancellableImageProvide

try {
final image = await request.load(decode);
if (image == null || isCancelled) {
if ((image == null && evictOnError) || isCancelled) {
PaintingBinding.instance.imageCache.evict(this);
return;
} else if (image == null) {
return;
Comment on lines +63 to +64
Copy link
Copy Markdown
Member

@mertalev mertalev Feb 17, 2026

Choose a reason for hiding this comment

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

Would this be logged for cancelled images? It seems potentially noisy.

Copy link
Copy Markdown
Collaborator Author

@LeLunZ LeLunZ Feb 21, 2026

Choose a reason for hiding this comment

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

This would only show for images that get loaded when evictOnError is false, and image is null.
This only happens in the FullImageProvider which is used for the asset view.

But you are right, we don't have to log this, as the eviction in the similar case also isn't logged.

}
yield image;
} catch (e) {
PaintingBinding.instance.imageCache.evict(this);
rethrow;
} catch (e, stack) {
if (evictOnError) {
PaintingBinding.instance.imageCache.evict(this);
rethrow;
}
_log.warning('Non-fatal image load error', e, stack);
Comment on lines +70 to +72
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

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

When evictOnError is false, exceptions are caught and logged but not propagated to the stream. This silently swallows errors, preventing subscribers from being notified of the failure. The stream simply ends without yielding an image, leaving subscribers unable to distinguish between a pending load and a failed load. Consider rethrowing the error after logging, or using a different mechanism to handle non-fatal errors.

Suggested change
rethrow;
}
_log.warning('Non-fatal image load error', e, stack);
}
_log.warning('Non-fatal image load error', e, stack);
rethrow;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@LeLunZ LeLunZ Feb 14, 2026

Choose a reason for hiding this comment

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

Thats intended in this case.

} finally {
this.request = null;
}
Expand Down
1 change: 0 additions & 1 deletion mobile/lib/presentation/widgets/images/local_image_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ class LocalFullImageProvider extends CancellableImageProvider<LocalFullImageProv
size: Size(size.width * devicePixelRatio, size.height * devicePixelRatio),
assetType: key.assetType,
);

yield* loadRequest(request, decode);

if (!Store.get(StoreKey.loadOriginal, false)) {
Expand Down
5 changes: 3 additions & 2 deletions mobile/lib/presentation/widgets/images/remote_image_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,10 @@ class RemoteFullImageProvider extends CancellableImageProvider<RemoteFullImagePr
uri: getThumbnailUrlForRemoteId(key.assetId, type: AssetMediaSize.preview, thumbhash: key.thumbhash),
headers: headers,
);
yield* loadRequest(previewRequest, decode);
final loadOriginal = assetType == AssetType.image && AppSetting.get(Setting.loadOriginal);
yield* loadRequest(previewRequest, decode, evictOnError: !loadOriginal);

if (assetType != AssetType.image || !AppSetting.get(Setting.loadOriginal)) {
if (!loadOriginal) {
return;
}

Expand Down
Loading