fix(mobile): let images be shared and saved from the thread - #5143
fix(mobile): let images be shared and saved from the thread#5143mackinleysmith wants to merge 6 commits into
Conversation
Images on mobile could not be copied, saved or shared. Tapping one opened react-native-image-viewing with its default chrome, which is a lone close button, and thumbnails in a thread offered nothing at all. Long-pressing an image now opens the system share sheet, where both platforms already put Copy and Save Image. This works on the thumbnail in a thread as well as on the fullscreen view, so you no longer have to open an image before acting on it. The four viewers that each hand-rolled their own ImageViewing block now share one component. Most of the work is resolving an image into something shareAsync accepts. Thread attachments are https URLs, picker attachments are file:// paths, and pasted attachments are data: URIs, while shareAsync only takes a local file. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-actions
Bot
added
size:L
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against the Effect service conventions. The new ImageShareError is defined and constructed in this PR; two of the error-modelling rules are not met. Details inline.
Posted via Macroscope — Effect Service Conventions
ApprovabilityVerdict: Needs human review This PR introduces new image sharing capability with substantial file system and async logic. Additionally, there is an unresolved high-severity bug: the data URI write is not awaited before sharing, causing potential race conditions. You can customize Macroscope's approvability policy. Learn more. |
Review found three problems with the share path. Asset URLs are signed capabilities, so putting one in an error attribute leaks the token to anything that reads logs. The error now carries scheme and host the way ExternalUrlOpenError already does, plus the stage that failed so a materialize failure is distinguishable from a share failure. Temporary files were named after the image, so two shares of same-named images could overwrite each other while a sheet was still reading. Each share now gets its own directory and the file inside keeps its display name, which is what the share sheet shows. A throw between creating and writing the temp file left it behind, because the caller had no reference to clean up yet. Materializing now removes its own directory before rethrowing.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const file = new File(directory, temporaryFileName(source)); | ||
| file.create({ overwrite: true }); | ||
| file.write(source.uri.slice(dataMatch[0].length), { encoding: "base64" }); | ||
| return { file, temporaryDirectory: directory }; |
There was a problem hiding this comment.
Data URI share races write
High Severity
When sharing a data: image, materializeImageFile calls file.write without awaiting it, then immediately returns so Sharing.shareAsync can run on a file that is still empty or incomplete. Pasted composer previews and other base64 data URIs can fail to copy or save, or surface “Couldn’t share the image,” even though remote https shares work because that path awaits downloadFileAsync.
Reviewed by Cursor Bugbot for commit 5a7ad60. Configure here.
There was a problem hiding this comment.
I do not think this one is real. File.write in expo-file-system v56 is synchronous, not a promise:
// expo-file-system/build/internal/NativeFileSystem.types.d.ts
/**
* Writes content to the file.
* @param content The content to write into the file.
*/
write(content: string | Uint8Array, options?: FileWriteOptions): void;It returns void, and there is no writeAsync counterpart in the package. create is synchronous too. So by the time materializeImageFile returns, the bytes are on disk and there is nothing to await.
The asymmetry with the https branch is because downloadFileAsync genuinely is async and is awaited. The data: branch has no equivalent.
Happy to be shown otherwise if you are looking at a different version of the API.
The in-flight guard lived in a useRef, so each caller got its own. The hook runs once per thread thumbnail and again in the fullscreen viewer, so long pressing two attachments could stack two system share sheets. The guard now lives at module scope, which is what one system sheet at a time actually means. Adds tests for the guard, including that it is released when a share fails or throws.
Android hands back content:// for picked and shared media. isLocalFileUri only recognised file:// and absolute paths, so those fell through to the download branch and failed. They are already on the device, so they now take the local path. The share lock was a boolean held for the lifetime of the share sheet, which made it app-wide once the guard moved to module scope. A sheet that never settles would have disabled sharing until restart. It is now a start time with a timeout, so a stuck share degrades to the stacking it was meant to prevent rather than to no sharing at all.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 319fcf2. Configure here.
The finally block cleared the lock unconditionally, so a share that settled after its lock had expired would release the lock a newer share had taken, letting a third long-press stack another sheet. The lock now holds the share's identity and is only cleared by that share, so a stale one settling late is a no-op.
1 participant
What Changed
Moving an image from one thread to another on a phone was not really possible. Pasting an image into the composer already works, but there was no way to get an image onto the clipboard in the first place, so the round trip was broken at the copy end.
Long-pressing an image now opens the system share sheet, which is where iOS and Android already put Copy and Save Image. It works on the thumbnail in a thread as well as on the fullscreen view, so you no longer have to open an image before acting on it. Copy an image from one thread, paste it into another, done.
The four screens that each rendered their own
ImageViewingblock (thread feed, workspace file preview, thread composer, review comment composer) now share oneFullScreenImageViewer. It draws no chrome of its own. Everything visible here is rendered by the OS.Most of the diff is
fullScreenImageActions.ts, which turns an image into somethingSharing.shareAsyncwill take. That is where the work is, because the app has three URI shapes for images andshareAsynconly accepts a local file:https://asset URLfile://pathdata:base64Why
The concrete thing I wanted was to copy an image out of one thread and paste it into another from my phone, which is something I hit often. The paste half already worked through
expo-paste-input. The copy half did not exist.Opening an image left you stuck with it.
react-native-image-viewingrenders a lone close button by default, and none of the four call sites passed a header or footer, so there was no way to copy, save or share. Thumbnails in a thread offered nothing at all.I used the system share sheet instead of building a toolbar because it already contains Copy and Save Image, and because it leaves nothing custom to maintain. An earlier version of this PR did build a toolbar, and testing it on device turned up a contrast bug that only existed because the chrome was mine.
One note for review: a
data:URI is the image bytes, soredactUrireduces it to its media type before anything reaches a log. There is a test asserting the payload never appears in an error.UI Changes
Before and after, same image, fullscreen. Before, the close button is the only control.
The interaction:
Testing
9 unit tests covering the three URI shapes, temp file cleanup on both the success and failure paths, filename sanitising, and the redaction guarantee.
Verified on the iOS Simulator (iPhone 17 Pro, iOS 26.5): long press, share sheet, Copy, and the system clipboard ends up holding a 133,403 byte PNG, matching the size the sheet reports for the source file. Pasting a copied image back into a composer works on a physical device.
Two gaps worth stating. The thumbnail long-press is not device verified. It is a four line change routing through the same code as the fullscreen case, but I could not stage a thread with a real image attachment locally. And Android is unverified. The change is platform neutral and the sheet is drawn by the OS, but it has not been run there.
Checklist
Note
Add long-press image sharing and saving from thread feed and composers
FullScreenImageViewercomponent wrappingreact-native-image-viewingwith long-press wired to a newuseShareImagehook, replacing directImageViewingusage in thread feed, composers, and file preview.shareImageinfullScreenImageActions.tsto materialize images to a temp file before callingSharing.shareAsync, supporting local, content, data URI, and remote URL sources with appropriate mime/UTI selection.shareImageExclusivelyprevents concurrent share sheets from stacking across the app, with a 60-second auto-expiry for stuck locks.Macroscope summarized 1bfbfdb.
Note
Medium Risk
Touches remote asset download, temp file handling, and clipboard-adjacent sharing paths; logic is well tested but Android and thumbnail long-press are noted as less device-verified in the PR.
Overview
Long-pressing images in the thread feed (thumbnails and fullscreen) and in composer attachment previews now opens the system share sheet, so copy/save no longer require custom UI on
react-native-image-viewing.Four screens that each inlined
ImageViewingnow use a sharedFullScreenImageViewer, which wires long-press to share and keeps swipe/double-tap zoom behavior.New
shareImage/fullScreenImageActionsmaterializehttps,data:,file://, and Androidcontent://URIs into a temp local file forexpo-sharing, sanitize filenames, clean up cache dirs, and redact signed URLs and base64 in error logs.useShareImageapplies a module-level 60s lock so stacked share sheets cannot wedge sharing app-wide; failures show anAlert.Reviewed by Cursor Bugbot for commit 1bfbfdb. Bugbot is set up for automated code reviews on this repo. Configure here.