Skip to content

Conversation

@xsahil03x
Copy link
Member

@xsahil03x xsahil03x commented Sep 19, 2025

Submit a pull request

Fixes: #2357

Description of the pull request

This PR fixes an issue where videos downloaded from attachments were not correctly saved to the device's gallery on mobile platforms.

The saveFile method in StreamAttachmentHandler now uses Gal.putVideo for video files and Gal.putImage for image files. If the file type is neither image nor video, it's saved to the app's directory without being copied to the gallery.

Additionally, the temporary file is now only deleted if it was successfully copied to the gallery.

Summary by CodeRabbit

  • Bug Fixes

    • Videos now save reliably to the device gallery on mobile (iOS and Android). Only image/video files are copied to the gallery; other file types are left unchanged.
  • Documentation

    • Changelog updated to document the video gallery save fix.

This commit fixes an issue where videos downloaded from attachments were not correctly saved to the device's gallery on mobile platforms.

The `saveFile` method in `StreamAttachmentHandler` now uses `Gal.putVideo` for video files and `Gal.putImage` for image files. If the file type is neither image nor video, it's saved to the app's directory without being copied to the gallery.

Additionally, the temporary file is now only deleted if it was successfully copied to the gallery.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Introduces mime-type based handling for saving attachments: images go to Gal.putImage, videos to Gal.putVideo, others are left untouched. Temporary files are deleted only after a successful gallery copy. Adds a CHANGELOG entry noting the mobile video save fix. No public APIs changed.

Changes

Cohort / File(s) Summary
Documentation
packages/stream_chat_flutter/CHANGELOG.md
Added Upcoming fix note: videos not saved to gallery correctly on mobile; references issue #2357. No code changes.
Attachment handling (IO)
packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_io.dart
Replaced unconditional gallery copy with mime-type dispatch: image/* → Gal.putImage, video/* → Gal.putVideo, otherwise no copy. Delete temp file only after successful copy. No public API changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as Attachment UI
  participant Handler as StreamAttachmentHandler (IO)
  participant Gal as Gal plugin

  User->>UI: Download attachment
  UI->>Handler: downloadAttachment(file, mimeType)

  alt mimeType starts with image/
    Handler->>Gal: putImage(path)
    Gal-->>Handler: success/failure
    opt on success
      Handler->>Handler: delete temp file
    end
  else mimeType starts with video/
    Handler->>Gal: putVideo(path)
    Gal-->>Handler: success/failure
    opt on success
      Handler->>Handler: delete temp file
    end
  else other mimeType
    Handler-->>UI: return original path (no gallery copy, no delete)
  end

  Handler-->>UI: result (path or error)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • renefloor

Poem

I hopped through bytes with careful cheer,
Sorting pics from vids, crystal-clear.
Image? This burrow. Video? That lane.
Others can rest, we don’t complain.
Temp crumbs swept when copies gleam—
A tidy warren, download dream. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title clearly and concisely summarizes the primary change — fixing video saves to the device gallery on mobile by using the correct gallery API — and it matches the implemented code changes.
Linked Issues Check ✅ Passed The changes implement a mime-type dispatch that calls Gal.putVideo for video/* and Gal.putImage for image/*, avoids using image-only APIs for videos, skips gallery copying for non-image/non-video types, and only deletes the temporary file after a successful gallery copy, which addresses the coding requirements described in issue #2357.
Out of Scope Changes Check ✅ Passed The diff is limited to the attachment handler implementation and the package changelog, both of which are directly related to the reported bug fix; there are no unrelated files or broad refactors included in this changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/copy-to-gallery

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9c6f741 and 68c3370.

📒 Files selected for processing (1)
  • packages/stream_chat_flutter/CHANGELOG.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/stream_chat_flutter/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: build (ios)
  • GitHub Check: build (android)
  • GitHub Check: test
  • GitHub Check: analyze_legacy_versions
  • GitHub Check: analyze
  • GitHub Check: stream_chat_localizations
  • GitHub Check: stream_chat_persistence
  • GitHub Check: stream_chat_flutter
  • GitHub Check: stream_chat_flutter_core

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/stream_chat_flutter/CHANGELOG.md (1)

6-7: Changelog entry looks good; consider tightening phrasing.

Optional: “Fixed videos not being saved to the gallery on Android and iOS.” reads a bit tighter. Current entry is fine if you prefer existing style.

packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_io.dart (1)

201-206: Good MIME‑based routing; add a fallback when mimeType is null.

On some downloads XFile.mimeType can be null. Fall back to inferring from the file path to avoid silently skipping gallery copy for valid images/videos.

Apply within this hunk:

-    final copyToGallery = switch (file.mimeType) {
-      final type? when type.startsWith('image/') => Gal.putImage,
-      final type? when type.startsWith('video/') => Gal.putVideo,
-      _ => null,
-    };
+    final inferredMime = file.mimeType ?? lookupMimeType(path);
+    final copyToGallery = switch (inferredMime) {
+      final type? when type.startsWith('image/') => Gal.putImage,
+      final type? when type.startsWith('video/') => Gal.putVideo,
+      _ => null,
+    };

Add the import (outside this hunk):

import 'package:mime/mime.dart';
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ee78069 and 9c6f741.

📒 Files selected for processing (2)
  • packages/stream_chat_flutter/CHANGELOG.md (1 hunks)
  • packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_io.dart (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: analyze_legacy_versions
  • GitHub Check: build (android)
  • GitHub Check: test
  • GitHub Check: stream_chat_localizations
  • GitHub Check: analyze
  • GitHub Check: stream_chat_flutter
  • GitHub Check: stream_chat_flutter_core
  • GitHub Check: stream_chat_persistence
🔇 Additional comments (1)
packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_io.dart (1)

207-213: Returned path may be deleted — confirm callers & API contract

Repo search: callers in packages/stream_chat_flutter/lib/src/context_menu_items/download_menu_item.dart (call at ~line 28) and packages/stream_chat_flutter/lib/src/attachment/file_attachment.dart (call at ~line 186) invoke downloadAttachment(...) and ignore the returned path; tests mock downloadAttachment to return 'filePath'; the base contract (packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_base.dart:63) documents returning a file path. The IO impl (packages/stream_chat_flutter/lib/src/attachment/handler/stream_attachment_handler_io.dart — snippet that deletes the temp file after copying to gallery) still returns that now-deleted path.

Either stop deleting the temp file here (delegate cleanup) or change the API to return a structured result (e.g., {copiedToGallery: true, path}) and update the docs/tests to reflect that the returned path may be removed.

@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.84%. Comparing base (1b93d73) to head (68c3370).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...tachment/handler/stream_attachment_handler_io.dart 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2386      +/-   ##
==========================================
- Coverage   63.85%   63.84%   -0.01%     
==========================================
  Files         413      413              
  Lines       25859    25862       +3     
==========================================
  Hits        16511    16511              
- Misses       9348     9351       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

renefloor
renefloor previously approved these changes Sep 23, 2025
@xsahil03x xsahil03x merged commit 3f24135 into master Sep 23, 2025
17 of 19 checks passed
@xsahil03x xsahil03x deleted the fix/copy-to-gallery branch September 23, 2025 14:10
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.

Downloading videos does not work on Android

3 participants