From e8b7d2ae4d27dacc9b3eeeaa3f413d7e651d770c Mon Sep 17 00:00:00 2001 From: MacKinley Smith Date: Fri, 31 Jul 2026 12:15:32 -0600 Subject: [PATCH 1/6] fix(mobile): let images be shared and saved from the thread 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) --- .../src/components/FullScreenImageViewer.tsx | 41 +++++ .../files/WorkspaceFileImagePreview.tsx | 24 +-- .../review/ReviewCommentComposerSheet.tsx | 14 +- .../src/features/threads/ThreadComposer.tsx | 15 +- .../src/features/threads/ThreadFeed.tsx | 28 +-- .../src/lib/fullScreenImageActions.test.ts | 160 +++++++++++++++++ apps/mobile/src/lib/fullScreenImageActions.ts | 168 ++++++++++++++++++ apps/mobile/src/lib/useShareImage.ts | 28 +++ 8 files changed, 432 insertions(+), 46 deletions(-) create mode 100644 apps/mobile/src/components/FullScreenImageViewer.tsx create mode 100644 apps/mobile/src/lib/fullScreenImageActions.test.ts create mode 100644 apps/mobile/src/lib/fullScreenImageActions.ts create mode 100644 apps/mobile/src/lib/useShareImage.ts diff --git a/apps/mobile/src/components/FullScreenImageViewer.tsx b/apps/mobile/src/components/FullScreenImageViewer.tsx new file mode 100644 index 00000000000..ebc7cd5b036 --- /dev/null +++ b/apps/mobile/src/components/FullScreenImageViewer.tsx @@ -0,0 +1,41 @@ +import { useCallback, useMemo } from "react"; +import ImageViewing from "react-native-image-viewing"; + +import type { FullScreenImageSource } from "../lib/fullScreenImageActions"; +import { useShareImage } from "../lib/useShareImage"; + +/** + * Fullscreen image viewer. Long-pressing the image opens the system share + * sheet, which is where both platforms already put Copy and Save Image, so + * this owns no chrome of its own. + */ +export function FullScreenImageViewer(props: { + readonly source: FullScreenImageSource | null; + readonly onRequestClose: () => void; +}) { + const { source } = props; + const share = useShareImage(); + + const images = useMemo( + () => (source === null ? [] : [{ uri: source.uri, cache: source.cache }]), + [source], + ); + + const onLongPress = useCallback(() => { + if (source !== null) { + share(source); + } + }, [share, source]); + + return ( + + ); +} diff --git a/apps/mobile/src/features/files/WorkspaceFileImagePreview.tsx b/apps/mobile/src/features/files/WorkspaceFileImagePreview.tsx index 73eca66bf99..4a0d4bbf757 100644 --- a/apps/mobile/src/features/files/WorkspaceFileImagePreview.tsx +++ b/apps/mobile/src/features/files/WorkspaceFileImagePreview.tsx @@ -1,11 +1,12 @@ import { useAtomValue } from "@effect/atom-react"; import { useMemo, useState } from "react"; import { ActivityIndicator, Image, Pressable, View } from "react-native"; -import ImageViewing from "react-native-image-viewing"; import { AsyncResult } from "effect/unstable/reactivity"; import { AppText as Text } from "../../components/AppText"; import { EmptyState } from "../../components/EmptyState"; +import { FullScreenImageViewer } from "../../components/FullScreenImageViewer"; +import type { FullScreenImageSource } from "../../lib/fullScreenImageActions"; import { workspaceFileImageAtom } from "./workspace-file-image-cache"; function ResolvedWorkspaceFileImagePreview(props: { @@ -13,12 +14,11 @@ function ResolvedWorkspaceFileImagePreview(props: { readonly uri: string; }) { const [loadError, setLoadError] = useState(null); - const [fullScreenVisible, setFullScreenVisible] = useState(false); + const [fullScreenSource, setFullScreenSource] = useState(null); const imageSource = useMemo( () => ({ uri: props.uri, cache: "force-cache" as const }), [props.uri], ); - const fullScreenImages = useMemo(() => [imageSource], [imageSource]); return ( @@ -27,7 +27,13 @@ function ResolvedWorkspaceFileImagePreview(props: { accessibilityLabel={`Open full-screen preview of ${props.accessibilityLabel}`} disabled={loadError !== null} className="flex-1 p-4 active:bg-subtle-strong" - onPress={() => setFullScreenVisible(true)} + onPress={() => + setFullScreenSource({ + uri: props.uri, + fileName: props.accessibilityLabel, + cache: "force-cache", + }) + } > ) : null} - setFullScreenVisible(false)} - swipeToCloseEnabled - doubleTapToZoomEnabled + setFullScreenSource(null)} /> ); diff --git a/apps/mobile/src/features/review/ReviewCommentComposerSheet.tsx b/apps/mobile/src/features/review/ReviewCommentComposerSheet.tsx index c6d678ddca9..279313891b1 100644 --- a/apps/mobile/src/features/review/ReviewCommentComposerSheet.tsx +++ b/apps/mobile/src/features/review/ReviewCommentComposerSheet.tsx @@ -12,12 +12,12 @@ import { } from "react-native"; import { KeyboardAvoidingView, KeyboardStickyView } from "react-native-keyboard-controller"; import { useSafeAreaInsets } from "react-native-safe-area-context"; -import ImageViewing from "react-native-image-viewing"; import { AppText as Text, AppTextInput as TextInput } from "../../components/AppText"; import { SymbolView } from "../../components/AppSymbol"; import { ComposerAttachmentStrip } from "../../components/ComposerAttachmentStrip"; import { ControlPill } from "../../components/ControlPill"; +import { FullScreenImageViewer } from "../../components/FullScreenImageViewer"; import { cn } from "../../lib/cn"; import type { DraftComposerImageAttachment } from "../../lib/composerImages"; import { convertPastedImagesToAttachments, pickComposerImages } from "../../lib/composerImages"; @@ -63,6 +63,10 @@ export function ReviewCommentComposerSheet(props: ReviewCommentComposerSheetProp >({}); const [attachments, setAttachments] = useState>([]); const [previewImageUri, setPreviewImageUri] = useState(null); + const previewSource = useMemo( + () => (previewImageUri === null ? null : { uri: previewImageUri }), + [previewImageUri], + ); const selectedLines = useMemo( () => (target ? getSelectedReviewCommentLines(target) : []), @@ -337,13 +341,9 @@ export function ReviewCommentComposerSheet(props: ReviewCommentComposerSheetProp ) : null} - setPreviewImageUri(null)} - swipeToCloseEnabled - doubleTapToZoomEnabled /> ); diff --git a/apps/mobile/src/features/threads/ThreadComposer.tsx b/apps/mobile/src/features/threads/ThreadComposer.tsx index fc45cba4260..7a25014e5a4 100644 --- a/apps/mobile/src/features/threads/ThreadComposer.tsx +++ b/apps/mobile/src/features/threads/ThreadComposer.tsx @@ -25,7 +25,6 @@ import { View, type ViewStyle, } from "react-native"; -import ImageViewing from "react-native-image-viewing"; import Animated, { FadeIn, FadeInDown, @@ -51,6 +50,7 @@ import { ComposerToolbarTrigger, } from "../../components/ComposerToolbarTrigger"; import { ControlPill, ControlPillMenu } from "../../components/ControlPill"; +import { FullScreenImageViewer } from "../../components/FullScreenImageViewer"; import { ProviderIcon } from "../../components/ProviderIcon"; import type { DraftComposerImageAttachment } from "../../lib/composerImages"; import { buildModelOptions, groupByProvider } from "../../lib/modelOptions"; @@ -277,6 +277,10 @@ export const ThreadComposer = memo(function ThreadComposer(props: ThreadComposer const { onExpandedChange } = props; const [previewImageUri, setPreviewImageUri] = useState(null); + const previewSource = useMemo( + () => (previewImageUri === null ? null : { uri: previewImageUri }), + [previewImageUri], + ); const hasContent = props.draftMessage.trim().length > 0 || props.draftAttachments.length > 0; const isExpanded = isFocused; const canSend = hasContent; @@ -922,14 +926,7 @@ export const ThreadComposer = memo(function ThreadComposer(props: ThreadComposer ) : null} - + ); }); diff --git a/apps/mobile/src/features/threads/ThreadFeed.tsx b/apps/mobile/src/features/threads/ThreadFeed.tsx index 8ad117c8635..0e56c82b676 100644 --- a/apps/mobile/src/features/threads/ThreadFeed.tsx +++ b/apps/mobile/src/features/threads/ThreadFeed.tsx @@ -43,7 +43,6 @@ import { View, } from "react-native"; import { TouchableOpacity } from "react-native-gesture-handler"; -import ImageViewing from "react-native-image-viewing"; import { useSafeAreaInsets } from "react-native-safe-area-context"; import Animated, { FadeIn, @@ -65,6 +64,8 @@ import { import { AppText as Text } from "../../components/AppText"; import { CopyTextButton } from "../../components/CopyTextButton"; +import { FullScreenImageViewer } from "../../components/FullScreenImageViewer"; +import { useShareImage } from "../../lib/useShareImage"; import { parseReviewCommentMessageSegments, type ReviewInlineComment, @@ -176,6 +177,7 @@ function MessageAttachmentImage(props: { _tag: "attachment", attachmentId: props.attachmentId, }); + const share = useShareImage(); if (uri === null) { return ( @@ -186,7 +188,11 @@ function MessageAttachmentImage(props: { } return ( - props.onPressImage(uri)}> + props.onPressImage(uri)} + onLongPress={() => share({ uri })} + > ); @@ -1916,23 +1922,7 @@ export const ThreadFeed = memo(function ThreadFeed(props: ThreadFeedProps) { ) : null} - setExpandedImage(null)} - swipeToCloseEnabled - doubleTapToZoomEnabled - /> + setExpandedImage(null)} /> ); }); diff --git a/apps/mobile/src/lib/fullScreenImageActions.test.ts b/apps/mobile/src/lib/fullScreenImageActions.test.ts new file mode 100644 index 00000000000..4e83463c02e --- /dev/null +++ b/apps/mobile/src/lib/fullScreenImageActions.test.ts @@ -0,0 +1,160 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + create: vi.fn(), + directoryCreate: vi.fn(), + delete: vi.fn(), + downloadFileAsync: vi.fn(), + write: vi.fn(), + isAvailableAsync: vi.fn(), + shareAsync: vi.fn(), +})); + +class FakeFile { + readonly uri: string; + constructor(...segments: ReadonlyArray<{ uri: string } | string>) { + this.uri = segments + .map((segment) => (typeof segment === "string" ? segment : segment.uri)) + .join("/"); + } + create = (options?: unknown) => mocks.create(this.uri, options); + delete = () => mocks.delete(this.uri); + write = (content: string, options?: unknown) => mocks.write(this.uri, content, options); + static downloadFileAsync = (url: string, destination: FakeFile, options?: unknown) => + mocks.downloadFileAsync(url, destination, options); +} + +class FakeDirectory { + readonly uri: string; + constructor(...segments: ReadonlyArray<{ uri: string } | string>) { + this.uri = segments + .map((segment) => (typeof segment === "string" ? segment : segment.uri)) + .join("/"); + } + create = (options?: unknown) => mocks.directoryCreate(this.uri, options); +} + +vi.mock("expo-file-system", () => ({ + Directory: FakeDirectory, + File: FakeFile, + Paths: { cache: { uri: "file:///cache" } }, +})); + +vi.mock("expo-sharing", () => ({ + isAvailableAsync: mocks.isAvailableAsync, + shareAsync: mocks.shareAsync, +})); + +import { + SHARE_FAILED_MESSAGE, + SHARING_UNAVAILABLE_MESSAGE, + redactUri, + shareImage, +} from "./fullScreenImageActions"; + +describe("redactUri", () => { + it("reduces a data URI to its media type so bytes never reach a log", () => { + expect(redactUri("data:image/png;base64,U0VDUkVU")).toBe("data:image/png"); + expect(redactUri("data:image/png;charset=utf-8;base64,U0VDUkVU")).toBe("data:image/png"); + }); + + it("leaves ordinary URLs alone", () => { + expect(redactUri("https://example.test/a.png")).toBe("https://example.test/a.png"); + }); +}); + +describe("shareImage", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.isAvailableAsync.mockResolvedValue(true); + mocks.shareAsync.mockResolvedValue(undefined); + mocks.downloadFileAsync.mockImplementation(async (_url, destination) => destination); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("shares a local file directly and never deletes it", async () => { + const result = await shareImage({ uri: "file:///tmp/shot.png" }); + + expect(result).toEqual({ ok: true }); + expect(mocks.shareAsync).toHaveBeenCalledWith( + "file:///tmp/shot.png", + expect.objectContaining({ mimeType: "image/png", UTI: "public.png" }), + ); + expect(mocks.downloadFileAsync).not.toHaveBeenCalled(); + expect(mocks.delete).not.toHaveBeenCalled(); + }); + + it("writes a data URI to a temp file, shares it, then deletes it", async () => { + const result = await shareImage({ uri: "data:image/png;base64,QUJD" }); + + expect(result).toEqual({ ok: true }); + expect(mocks.write).toHaveBeenCalledWith( + expect.stringContaining(".png"), + "QUJD", + expect.objectContaining({ encoding: "base64" }), + ); + expect(mocks.delete).toHaveBeenCalledTimes(1); + }); + + it("handles data URIs carrying extra media-type parameters", async () => { + const result = await shareImage({ uri: "data:image/png;charset=utf-8;base64,QUJD" }); + + expect(result).toEqual({ ok: true }); + expect(mocks.write).toHaveBeenCalledWith(expect.anything(), "QUJD", expect.anything()); + }); + + it("downloads a remote image, then cleans the temp file up", async () => { + const result = await shareImage({ uri: "https://example.test/assets/a.png?revision=3" }); + + expect(result).toEqual({ ok: true }); + expect(mocks.downloadFileAsync).toHaveBeenCalledWith( + "https://example.test/assets/a.png?revision=3", + expect.anything(), + expect.anything(), + ); + // The cache-buster must not be read as part of the extension. + expect(mocks.shareAsync).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ mimeType: "image/png" }), + ); + expect(mocks.delete).toHaveBeenCalledTimes(1); + }); + + it("derives the temp file name from fileName, stripping any path", async () => { + await shareImage({ uri: "https://example.test/a.png", fileName: "../../etc/logo.png" }); + + expect(mocks.downloadFileAsync).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ uri: expect.stringContaining("logo.png") }), + expect.anything(), + ); + }); + + it("reports when sharing is unavailable instead of throwing", async () => { + mocks.isAvailableAsync.mockResolvedValue(false); + + const result = await shareImage({ uri: "file:///tmp/shot.png" }); + + expect(result).toEqual({ ok: false, message: SHARING_UNAVAILABLE_MESSAGE }); + expect(mocks.shareAsync).not.toHaveBeenCalled(); + }); + + it("reports a failure without logging image bytes, and still deletes the temp file", async () => { + const logged: Array = []; + vi.spyOn(console, "error").mockImplementation((value: unknown) => { + logged.push(value); + }); + mocks.shareAsync.mockRejectedValue(new Error("sheet failed")); + + const result = await shareImage({ uri: "data:image/png;base64,U0VDUkVU" }); + + expect(result).toEqual({ ok: false, message: SHARE_FAILED_MESSAGE }); + expect(mocks.delete).toHaveBeenCalledTimes(1); + const serialized = logged.map((entry) => String((entry as Error).message)).join("\n"); + expect(serialized).not.toContain("U0VDUkVU"); + expect(serialized).toContain("data:image/png"); + }); +}); diff --git a/apps/mobile/src/lib/fullScreenImageActions.ts b/apps/mobile/src/lib/fullScreenImageActions.ts new file mode 100644 index 00000000000..2d8ec386762 --- /dev/null +++ b/apps/mobile/src/lib/fullScreenImageActions.ts @@ -0,0 +1,168 @@ +import * as Schema from "effect/Schema"; +import * as Sharing from "expo-sharing"; +import type { ImageURISource } from "react-native"; + +export type FullScreenImageSource = { + readonly uri: string; + readonly fileName?: string; + /** Forwarded to `react-native-image-viewing`'s underlying `Image` source. */ + readonly cache?: ImageURISource["cache"]; +}; + +export type ImageActionResult = + | { readonly ok: true } + | { readonly ok: false; readonly message: string }; + +export const SHARING_UNAVAILABLE_MESSAGE = "Sharing isn't available on this device."; +export const SHARE_FAILED_MESSAGE = "Couldn't share the image."; + +export class ImageShareError extends Schema.TaggedErrorClass()("ImageShareError", { + uri: Schema.String, + cause: Schema.Defect(), +}) { + override get message(): string { + return `Failed to share the image at ${this.uri}.`; + } +} + +const CACHE_DIRECTORY_NAME = "fullscreen-image-share"; +const DATA_URI_PATTERN = /^data:([^;,]*)(?:;[^;,=]+=[^;,]+)*(?:(;base64))?,/i; + +/** iOS needs a UTI alongside the mime type for the sheet to offer the right targets. */ +const IMAGE_TYPES: ReadonlyArray<{ + readonly extension: string; + readonly mimeType: string; + readonly uti: string; +}> = [ + { extension: "png", mimeType: "image/png", uti: "public.png" }, + { extension: "jpg", mimeType: "image/jpeg", uti: "public.jpeg" }, + { extension: "jpeg", mimeType: "image/jpeg", uti: "public.jpeg" }, + { extension: "gif", mimeType: "image/gif", uti: "com.compuserve.gif" }, + { extension: "webp", mimeType: "image/webp", uti: "org.webmproject.webp" }, + { extension: "heic", mimeType: "image/heic", uti: "public.heic" }, + { extension: "bmp", mimeType: "image/bmp", uti: "com.microsoft.bmp" }, +]; + +type ImageType = (typeof IMAGE_TYPES)[number]; + +let temporaryFileCounter = 0; + +/** `data:` URIs *are* the image bytes, so they are never logged verbatim. */ +export function redactUri(uri: string): string { + const match = DATA_URI_PATTERN.exec(uri); + return match === null ? uri : `data:${match[1] || "application/octet-stream"}`; +} + +function imageTypeFor(uri: string): ImageType | null { + const dataMatch = DATA_URI_PATTERN.exec(uri); + if (dataMatch !== null) { + const mimeType = dataMatch[1]?.toLowerCase(); + return IMAGE_TYPES.find((type) => type.mimeType === mimeType) ?? null; + } + + // Only the last path segment, so dots in the hostname are not read as an extension. + const lastSegment = (uri.split(/[?#]/, 1)[0] ?? "").split("/").pop() ?? ""; + const dotIndex = lastSegment.lastIndexOf("."); + if (dotIndex <= 0) { + return null; + } + const extension = lastSegment.slice(dotIndex + 1).toLowerCase(); + return IMAGE_TYPES.find((type) => type.extension === extension) ?? null; +} + +/** Returns "" when nothing usable is left, so the caller falls back to the counter. */ +function sanitizeFileNameStem(fileName: string): string { + const withoutDirectories = fileName.split(/[\\/]/).pop() ?? ""; + const dotIndex = withoutDirectories.lastIndexOf("."); + const stem = dotIndex > 0 ? withoutDirectories.slice(0, dotIndex) : withoutDirectories; + return stem.replace(/[^A-Za-z0-9._-]+/g, "-").replace(/^[.-]+|-+$/g, ""); +} + +function temporaryFileName(source: FullScreenImageSource): string { + const extension = imageTypeFor(source.uri)?.extension ?? "img"; + const stem = source.fileName ? sanitizeFileNameStem(source.fileName) : ""; + if (stem.length > 0) { + return `${stem}.${extension}`; + } + temporaryFileCounter += 1; + return `image-${temporaryFileCounter}.${extension}`; +} + +function isLocalFileUri(uri: string): boolean { + return uri.startsWith("file://") || uri.startsWith("/"); +} + +async function cacheDirectory() { + const { Directory, Paths } = await import("expo-file-system"); + const directory = new Directory(Paths.cache, CACHE_DIRECTORY_NAME); + directory.create({ idempotent: true, intermediates: true }); + return directory; +} + +function deleteQuietly(file: { delete: () => void }): void { + try { + file.delete(); + } catch { + // A leftover file in the cache directory is harmless; the OS reclaims it. + } +} + +type MaterializedImage = { + readonly file: { readonly uri: string; delete: () => void }; + /** True when we created the file and are therefore responsible for removing it. */ + readonly ownsTemporaryFile: boolean; +}; + +/** `Sharing.shareAsync` only accepts a local file, so remote and data URIs land on disk first. */ +async function materializeImageFile(source: FullScreenImageSource): Promise { + const { File } = await import("expo-file-system"); + + const dataMatch = DATA_URI_PATTERN.exec(source.uri); + if (dataMatch !== null) { + if (dataMatch[2] === undefined) { + throw new Error("Only base64-encoded data URIs are supported."); + } + const file = new File(await cacheDirectory(), temporaryFileName(source)); + file.create({ overwrite: true }); + file.write(source.uri.slice(dataMatch[0].length), { encoding: "base64" }); + return { file, ownsTemporaryFile: true }; + } + + if (isLocalFileUri(source.uri)) { + return { file: new File(source.uri), ownsTemporaryFile: false }; + } + + const destination = new File(await cacheDirectory(), temporaryFileName(source)); + const downloaded = await File.downloadFileAsync(source.uri, destination, { + idempotent: true, + }); + return { file: downloaded, ownsTemporaryFile: true }; +} + +export async function shareImage(source: FullScreenImageSource): Promise { + let materialized: MaterializedImage | null = null; + try { + if (!(await Sharing.isAvailableAsync())) { + return { ok: false, message: SHARING_UNAVAILABLE_MESSAGE }; + } + + materialized = await materializeImageFile(source); + const imageType = imageTypeFor(source.uri); + + // Resolves only after the sheet is dismissed, so the file outlives every read. + await Sharing.shareAsync(materialized.file.uri, { + dialogTitle: source.fileName, + mimeType: imageType?.mimeType, + UTI: imageType?.uti, + }); + + return { ok: true }; + } catch (cause) { + console.error(new ImageShareError({ uri: redactUri(source.uri), cause })); + return { ok: false, message: SHARE_FAILED_MESSAGE }; + } finally { + if (materialized?.ownsTemporaryFile) { + deleteQuietly(materialized.file); + } + } +} diff --git a/apps/mobile/src/lib/useShareImage.ts b/apps/mobile/src/lib/useShareImage.ts new file mode 100644 index 00000000000..9ad0b23182b --- /dev/null +++ b/apps/mobile/src/lib/useShareImage.ts @@ -0,0 +1,28 @@ +import { useCallback, useRef } from "react"; +import { Alert } from "react-native"; + +import { shareImage, type FullScreenImageSource } from "./fullScreenImageActions"; + +/** + * Opens the system share sheet for an image, ignoring further presses until + * the current one settles so a double press cannot stack two sheets. + */ +export function useShareImage() { + const sharingRef = useRef(false); + + return useCallback((source: FullScreenImageSource) => { + if (sharingRef.current) { + return; + } + sharingRef.current = true; + void shareImage(source) + .then((result) => { + if (!result.ok) { + Alert.alert(result.message); + } + }) + .finally(() => { + sharingRef.current = false; + }); + }, []); +} From 5a7ad6027837eb2960de8a90e79352b037aa677a Mon Sep 17 00:00:00 2001 From: MacKinley Smith Date: Fri, 31 Jul 2026 14:57:47 -0600 Subject: [PATCH 2/6] fix(mobile): keep signed asset URLs out of share error logs 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. --- .../src/lib/fullScreenImageActions.test.ts | 97 ++++++++------ apps/mobile/src/lib/fullScreenImageActions.ts | 119 +++++++++++------- 2 files changed, 139 insertions(+), 77 deletions(-) diff --git a/apps/mobile/src/lib/fullScreenImageActions.test.ts b/apps/mobile/src/lib/fullScreenImageActions.test.ts index 4e83463c02e..eee51796bfa 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.test.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.test.ts @@ -3,7 +3,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test" const mocks = vi.hoisted(() => ({ create: vi.fn(), directoryCreate: vi.fn(), - delete: vi.fn(), + directoryDelete: vi.fn(), downloadFileAsync: vi.fn(), write: vi.fn(), isAvailableAsync: vi.fn(), @@ -18,7 +18,6 @@ class FakeFile { .join("/"); } create = (options?: unknown) => mocks.create(this.uri, options); - delete = () => mocks.delete(this.uri); write = (content: string, options?: unknown) => mocks.write(this.uri, content, options); static downloadFileAsync = (url: string, destination: FakeFile, options?: unknown) => mocks.downloadFileAsync(url, destination, options); @@ -32,6 +31,7 @@ class FakeDirectory { .join("/"); } create = (options?: unknown) => mocks.directoryCreate(this.uri, options); + delete = () => mocks.directoryDelete(this.uri); } vi.mock("expo-file-system", () => ({ @@ -48,18 +48,36 @@ vi.mock("expo-sharing", () => ({ import { SHARE_FAILED_MESSAGE, SHARING_UNAVAILABLE_MESSAGE, - redactUri, + imageUriMetadata, shareImage, } from "./fullScreenImageActions"; -describe("redactUri", () => { - it("reduces a data URI to its media type so bytes never reach a log", () => { - expect(redactUri("data:image/png;base64,U0VDUkVU")).toBe("data:image/png"); - expect(redactUri("data:image/png;charset=utf-8;base64,U0VDUkVU")).toBe("data:image/png"); +/** A signed asset URL of the shape `assets.createUrl` mints. */ +const SIGNED_ASSET_URL = + "https://relay.example.test/api/assets/eyJhIjoxfQ.s3cr3tS1gnatur3/light-ui.png"; + +describe("imageUriMetadata", () => { + it("keeps the signed token out of asset URL diagnostics", () => { + const metadata = imageUriMetadata(SIGNED_ASSET_URL); + + expect(metadata).toEqual({ scheme: "https", host: "relay.example.test" }); + expect(JSON.stringify(metadata)).not.toContain("s3cr3tS1gnatur3"); + }); + + it("reduces a data URI to its media type", () => { + expect(imageUriMetadata("data:image/png;base64,U0VDUkVU")).toEqual({ + scheme: "data", + host: "image/png", + }); + expect(imageUriMetadata("data:image/png;charset=utf-8;base64,U0VDUkVU")).toEqual({ + scheme: "data", + host: "image/png", + }); }); - it("leaves ordinary URLs alone", () => { - expect(redactUri("https://example.test/a.png")).toBe("https://example.test/a.png"); + it("handles local files and unparseable input", () => { + expect(imageUriMetadata("file:///tmp/shot.png").scheme).toBe("file"); + expect(imageUriMetadata("not a url").scheme).toBe("unknown"); }); }); @@ -75,7 +93,7 @@ describe("shareImage", () => { vi.restoreAllMocks(); }); - it("shares a local file directly and never deletes it", async () => { + it("shares a local file directly and never deletes anything", async () => { const result = await shareImage({ uri: "file:///tmp/shot.png" }); expect(result).toEqual({ ok: true }); @@ -84,10 +102,10 @@ describe("shareImage", () => { expect.objectContaining({ mimeType: "image/png", UTI: "public.png" }), ); expect(mocks.downloadFileAsync).not.toHaveBeenCalled(); - expect(mocks.delete).not.toHaveBeenCalled(); + expect(mocks.directoryDelete).not.toHaveBeenCalled(); }); - it("writes a data URI to a temp file, shares it, then deletes it", async () => { + it("writes a data URI to a temp directory, shares it, then removes the directory", async () => { const result = await shareImage({ uri: "data:image/png;base64,QUJD" }); expect(result).toEqual({ ok: true }); @@ -96,7 +114,7 @@ describe("shareImage", () => { "QUJD", expect.objectContaining({ encoding: "base64" }), ); - expect(mocks.delete).toHaveBeenCalledTimes(1); + expect(mocks.directoryDelete).toHaveBeenCalledTimes(1); }); it("handles data URIs carrying extra media-type parameters", async () => { @@ -106,31 +124,39 @@ describe("shareImage", () => { expect(mocks.write).toHaveBeenCalledWith(expect.anything(), "QUJD", expect.anything()); }); - it("downloads a remote image, then cleans the temp file up", async () => { + it("downloads a remote image, then removes the temp directory", async () => { const result = await shareImage({ uri: "https://example.test/assets/a.png?revision=3" }); expect(result).toEqual({ ok: true }); - expect(mocks.downloadFileAsync).toHaveBeenCalledWith( - "https://example.test/assets/a.png?revision=3", - expect.anything(), - expect.anything(), - ); // The cache-buster must not be read as part of the extension. expect(mocks.shareAsync).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ mimeType: "image/png" }), ); - expect(mocks.delete).toHaveBeenCalledTimes(1); + expect(mocks.directoryDelete).toHaveBeenCalledTimes(1); }); - it("derives the temp file name from fileName, stripping any path", async () => { - await shareImage({ uri: "https://example.test/a.png", fileName: "../../etc/logo.png" }); + it("keeps the display name intact while isolating each share in its own directory", async () => { + await shareImage({ uri: SIGNED_ASSET_URL, fileName: "../../etc/light-ui.png" }); + await shareImage({ uri: SIGNED_ASSET_URL, fileName: "light-ui.png" }); - expect(mocks.downloadFileAsync).toHaveBeenCalledWith( - expect.anything(), - expect.objectContaining({ uri: expect.stringContaining("logo.png") }), - expect.anything(), - ); + const [first, second] = mocks.downloadFileAsync.mock.calls.map((call) => call[1].uri); + expect(first).toContain("light-ui.png"); + expect(second).toContain("light-ui.png"); + expect(first).not.toBe(second); + }); + + it("removes the temp directory when writing the file fails", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + mocks.write.mockImplementation(() => { + throw new Error("storage exhausted"); + }); + + const result = await shareImage({ uri: "data:image/png;base64,QUJD" }); + + expect(result).toEqual({ ok: false, message: SHARE_FAILED_MESSAGE }); + expect(mocks.directoryDelete).toHaveBeenCalledTimes(1); + expect(mocks.shareAsync).not.toHaveBeenCalled(); }); it("reports when sharing is unavailable instead of throwing", async () => { @@ -142,19 +168,20 @@ describe("shareImage", () => { expect(mocks.shareAsync).not.toHaveBeenCalled(); }); - it("reports a failure without logging image bytes, and still deletes the temp file", async () => { - const logged: Array = []; + it("logs neither the signed token nor image bytes, and records the failing stage", async () => { + const logged: Array<{ message: string; stage: string; host?: string }> = []; vi.spyOn(console, "error").mockImplementation((value: unknown) => { - logged.push(value); + logged.push(value as { message: string; stage: string; host?: string }); }); mocks.shareAsync.mockRejectedValue(new Error("sheet failed")); - const result = await shareImage({ uri: "data:image/png;base64,U0VDUkVU" }); + await shareImage({ uri: SIGNED_ASSET_URL }); + await shareImage({ uri: "data:image/png;base64,U0VDUkVU" }); - expect(result).toEqual({ ok: false, message: SHARE_FAILED_MESSAGE }); - expect(mocks.delete).toHaveBeenCalledTimes(1); - const serialized = logged.map((entry) => String((entry as Error).message)).join("\n"); + const serialized = logged.map((entry) => entry.message).join("\n"); + expect(serialized).not.toContain("s3cr3tS1gnatur3"); expect(serialized).not.toContain("U0VDUkVU"); - expect(serialized).toContain("data:image/png"); + expect(logged[0]?.host).toBe("relay.example.test"); + expect(logged[0]?.stage).toBe("share"); }); }); diff --git a/apps/mobile/src/lib/fullScreenImageActions.ts b/apps/mobile/src/lib/fullScreenImageActions.ts index 2d8ec386762..8fefc32d322 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.ts @@ -17,11 +17,15 @@ export const SHARING_UNAVAILABLE_MESSAGE = "Sharing isn't available on this devi export const SHARE_FAILED_MESSAGE = "Couldn't share the image."; export class ImageShareError extends Schema.TaggedErrorClass()("ImageShareError", { - uri: Schema.String, + stage: Schema.Literals(["materialize", "share"]), + scheme: Schema.String, + host: Schema.optional(Schema.String), cause: Schema.Defect(), }) { override get message(): string { - return `Failed to share the image at ${this.uri}.`; + const action = this.stage === "materialize" ? "prepare" : "share"; + const from = this.host === undefined ? "" : ` from ${this.host}`; + return `Failed to ${action} the ${this.scheme} image${from}.`; } } @@ -45,12 +49,30 @@ const IMAGE_TYPES: ReadonlyArray<{ type ImageType = (typeof IMAGE_TYPES)[number]; -let temporaryFileCounter = 0; - -/** `data:` URIs *are* the image bytes, so they are never logged verbatim. */ -export function redactUri(uri: string): string { - const match = DATA_URI_PATTERN.exec(uri); - return match === null ? uri : `data:${match[1] || "application/octet-stream"}`; +let temporaryDirectoryCounter = 0; + +/** + * Safe diagnostics for an image URI, mirroring `openExternalUrl`. Asset URLs are + * signed capabilities and `data:` URIs are the image bytes, so neither may be + * logged whole. For a `data:` URI `host` carries the media type instead. + */ +export function imageUriMetadata(uri: string): { + readonly scheme: string; + readonly host?: string; +} { + const dataMatch = DATA_URI_PATTERN.exec(uri); + if (dataMatch !== null) { + return { scheme: "data", host: dataMatch[1] || "application/octet-stream" }; + } + try { + const parsed = new URL(uri); + return { + scheme: parsed.protocol.replace(/:$/, "") || "unknown", + host: parsed.hostname || undefined, + }; + } catch { + return { scheme: /^([a-z][a-z\d+.-]*):/i.exec(uri)?.[1]?.toLowerCase() ?? "unknown" }; + } } function imageTypeFor(uri: string): ImageType | null { @@ -70,7 +92,7 @@ function imageTypeFor(uri: string): ImageType | null { return IMAGE_TYPES.find((type) => type.extension === extension) ?? null; } -/** Returns "" when nothing usable is left, so the caller falls back to the counter. */ +/** Returns "" when nothing usable is left, so the caller falls back to a generic name. */ function sanitizeFileNameStem(fileName: string): string { const withoutDirectories = fileName.split(/[\\/]/).pop() ?? ""; const dotIndex = withoutDirectories.lastIndexOf("."); @@ -81,72 +103,85 @@ function sanitizeFileNameStem(fileName: string): string { function temporaryFileName(source: FullScreenImageSource): string { const extension = imageTypeFor(source.uri)?.extension ?? "img"; const stem = source.fileName ? sanitizeFileNameStem(source.fileName) : ""; - if (stem.length > 0) { - return `${stem}.${extension}`; - } - temporaryFileCounter += 1; - return `image-${temporaryFileCounter}.${extension}`; + return `${stem.length > 0 ? stem : "image"}.${extension}`; } function isLocalFileUri(uri: string): boolean { return uri.startsWith("file://") || uri.startsWith("/"); } -async function cacheDirectory() { +/** + * A fresh directory per share. The file inside keeps its display name, which is + * what the share sheet shows, while the unique parent stops two shares of + * same-named images from overwriting each other. + */ +async function createTemporaryDirectory() { const { Directory, Paths } = await import("expo-file-system"); - const directory = new Directory(Paths.cache, CACHE_DIRECTORY_NAME); + temporaryDirectoryCounter += 1; + const directory = new Directory( + Paths.cache, + CACHE_DIRECTORY_NAME, + String(temporaryDirectoryCounter), + ); directory.create({ idempotent: true, intermediates: true }); return directory; } -function deleteQuietly(file: { delete: () => void }): void { +function deleteQuietly(target: { delete: () => void }): void { try { - file.delete(); + target.delete(); } catch { - // A leftover file in the cache directory is harmless; the OS reclaims it. + // A leftover entry in the cache directory is harmless; the OS reclaims it. } } type MaterializedImage = { - readonly file: { readonly uri: string; delete: () => void }; - /** True when we created the file and are therefore responsible for removing it. */ - readonly ownsTemporaryFile: boolean; + readonly file: { readonly uri: string }; + /** Set when we created a temporary directory that must be removed afterwards. */ + readonly temporaryDirectory: { delete: () => void } | null; }; /** `Sharing.shareAsync` only accepts a local file, so remote and data URIs land on disk first. */ async function materializeImageFile(source: FullScreenImageSource): Promise { const { File } = await import("expo-file-system"); - const dataMatch = DATA_URI_PATTERN.exec(source.uri); - if (dataMatch !== null) { - if (dataMatch[2] === undefined) { - throw new Error("Only base64-encoded data URIs are supported."); - } - const file = new File(await cacheDirectory(), temporaryFileName(source)); - file.create({ overwrite: true }); - file.write(source.uri.slice(dataMatch[0].length), { encoding: "base64" }); - return { file, ownsTemporaryFile: true }; - } - if (isLocalFileUri(source.uri)) { - return { file: new File(source.uri), ownsTemporaryFile: false }; + return { file: new File(source.uri), temporaryDirectory: null }; } - const destination = new File(await cacheDirectory(), temporaryFileName(source)); - const downloaded = await File.downloadFileAsync(source.uri, destination, { - idempotent: true, - }); - return { file: downloaded, ownsTemporaryFile: true }; + const directory = await createTemporaryDirectory(); + try { + const dataMatch = DATA_URI_PATTERN.exec(source.uri); + if (dataMatch !== null) { + if (dataMatch[2] === undefined) { + throw new Error("Only base64-encoded data URIs are supported."); + } + 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 }; + } + + const destination = new File(directory, temporaryFileName(source)); + const downloaded = await File.downloadFileAsync(source.uri, destination, { idempotent: true }); + return { file: downloaded, temporaryDirectory: directory }; + } catch (cause) { + // The directory is ours and the caller never sees it, so clean up here. + deleteQuietly(directory); + throw cause; + } } export async function shareImage(source: FullScreenImageSource): Promise { let materialized: MaterializedImage | null = null; + let stage: "materialize" | "share" = "materialize"; try { if (!(await Sharing.isAvailableAsync())) { return { ok: false, message: SHARING_UNAVAILABLE_MESSAGE }; } materialized = await materializeImageFile(source); + stage = "share"; const imageType = imageTypeFor(source.uri); // Resolves only after the sheet is dismissed, so the file outlives every read. @@ -158,11 +193,11 @@ export async function shareImage(source: FullScreenImageSource): Promise Date: Fri, 31 Jul 2026 15:06:21 -0600 Subject: [PATCH 3/6] fix(mobile): share one image at a time across every long-press target 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. --- apps/mobile/src/lib/useShareImage.test.ts | 73 +++++++++++++++++++++++ apps/mobile/src/lib/useShareImage.ts | 44 +++++++------- 2 files changed, 97 insertions(+), 20 deletions(-) create mode 100644 apps/mobile/src/lib/useShareImage.test.ts diff --git a/apps/mobile/src/lib/useShareImage.test.ts b/apps/mobile/src/lib/useShareImage.test.ts new file mode 100644 index 00000000000..e0edc86a01d --- /dev/null +++ b/apps/mobile/src/lib/useShareImage.test.ts @@ -0,0 +1,73 @@ +import { beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +const mocks = vi.hoisted(() => ({ + alert: vi.fn(), + shareImage: vi.fn(), +})); + +vi.mock("react-native", () => ({ + Alert: { alert: mocks.alert }, +})); + +vi.mock("./fullScreenImageActions", () => ({ + shareImage: mocks.shareImage, +})); + +import { shareImageExclusively } from "./useShareImage"; + +describe("shareImageExclusively", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("ignores a second request while a sheet is already open", async () => { + let release: (() => void) | undefined; + mocks.shareImage.mockReturnValue( + new Promise((resolve) => { + release = () => resolve({ ok: true }); + }), + ); + + // Two different thumbnails long-pressed before the first sheet appears. + const first = shareImageExclusively({ uri: "https://example.test/a.png" }); + const second = shareImageExclusively({ uri: "https://example.test/b.png" }); + + expect(mocks.shareImage).toHaveBeenCalledTimes(1); + + release?.(); + await Promise.all([first, second]); + }); + + it("allows a new share once the previous one settles", async () => { + mocks.shareImage.mockResolvedValue({ ok: true }); + + await shareImageExclusively({ uri: "https://example.test/a.png" }); + await shareImageExclusively({ uri: "https://example.test/b.png" }); + + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + }); + + it("releases the guard when a share fails, and surfaces the message", async () => { + mocks.shareImage.mockResolvedValue({ ok: false, message: "Couldn't share the image." }); + + await shareImageExclusively({ uri: "https://example.test/a.png" }); + + expect(mocks.alert).toHaveBeenCalledWith("Couldn't share the image."); + + mocks.shareImage.mockResolvedValue({ ok: true }); + await shareImageExclusively({ uri: "https://example.test/b.png" }); + + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + }); + + it("releases the guard when shareImage throws", async () => { + mocks.shareImage.mockRejectedValue(new Error("boom")); + + await expect(shareImageExclusively({ uri: "https://example.test/a.png" })).rejects.toThrow(); + + mocks.shareImage.mockResolvedValue({ ok: true }); + await shareImageExclusively({ uri: "https://example.test/b.png" }); + + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/mobile/src/lib/useShareImage.ts b/apps/mobile/src/lib/useShareImage.ts index 9ad0b23182b..9a9e1e0f9fb 100644 --- a/apps/mobile/src/lib/useShareImage.ts +++ b/apps/mobile/src/lib/useShareImage.ts @@ -1,28 +1,32 @@ -import { useCallback, useRef } from "react"; +import { useCallback } from "react"; import { Alert } from "react-native"; import { shareImage, type FullScreenImageSource } from "./fullScreenImageActions"; -/** - * Opens the system share sheet for an image, ignoring further presses until - * the current one settles so a double press cannot stack two sheets. - */ -export function useShareImage() { - const sharingRef = useRef(false); +// Module scope, not per hook. The hook is called once per thumbnail and again +// by the fullscreen viewer, so a ref would guard each caller separately and +// still let two long-presses stack two system sheets. +let sharing = false; - return useCallback((source: FullScreenImageSource) => { - if (sharingRef.current) { - return; +/** Exported for tests. The hook is a thin wrapper around this. */ +export async function shareImageExclusively(source: FullScreenImageSource): Promise { + if (sharing) { + return; + } + sharing = true; + try { + const result = await shareImage(source); + if (!result.ok) { + Alert.alert(result.message); } - sharingRef.current = true; - void shareImage(source) - .then((result) => { - if (!result.ok) { - Alert.alert(result.message); - } - }) - .finally(() => { - sharingRef.current = false; - }); + } finally { + sharing = false; + } +} + +/** Opens the system share sheet for an image, one at a time across the app. */ +export function useShareImage() { + return useCallback((source: FullScreenImageSource) => { + void shareImageExclusively(source); }, []); } From 319fcf2387f33152c21cb86ec0368544a445b9eb Mon Sep 17 00:00:00 2001 From: MacKinley Smith Date: Fri, 31 Jul 2026 15:44:32 -0600 Subject: [PATCH 4/6] fix(mobile): handle Android content URIs and expire the share lock 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. --- .../src/lib/fullScreenImageActions.test.ts | 12 ++++++++++ apps/mobile/src/lib/fullScreenImageActions.ts | 4 +++- apps/mobile/src/lib/useShareImage.test.ts | 23 +++++++++++++++++++ apps/mobile/src/lib/useShareImage.ts | 14 +++++++---- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/apps/mobile/src/lib/fullScreenImageActions.test.ts b/apps/mobile/src/lib/fullScreenImageActions.test.ts index eee51796bfa..ab8fc2001f5 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.test.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.test.ts @@ -105,6 +105,18 @@ describe("shareImage", () => { expect(mocks.directoryDelete).not.toHaveBeenCalled(); }); + it("treats an Android content:// URI as local rather than downloading it", async () => { + const result = await shareImage({ uri: "content://media/external/images/media/42" }); + + expect(result).toEqual({ ok: true }); + expect(mocks.downloadFileAsync).not.toHaveBeenCalled(); + expect(mocks.shareAsync).toHaveBeenCalledWith( + "content://media/external/images/media/42", + expect.anything(), + ); + expect(mocks.directoryDelete).not.toHaveBeenCalled(); + }); + it("writes a data URI to a temp directory, shares it, then removes the directory", async () => { const result = await shareImage({ uri: "data:image/png;base64,QUJD" }); diff --git a/apps/mobile/src/lib/fullScreenImageActions.ts b/apps/mobile/src/lib/fullScreenImageActions.ts index 8fefc32d322..39624b3aa2b 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.ts @@ -107,7 +107,9 @@ function temporaryFileName(source: FullScreenImageSource): string { } function isLocalFileUri(uri: string): boolean { - return uri.startsWith("file://") || uri.startsWith("/"); + // Android hands back content:// for picked and shared media. It is already on + // the device, so it must not fall through to the download branch. + return uri.startsWith("file://") || uri.startsWith("content://") || uri.startsWith("/"); } /** diff --git a/apps/mobile/src/lib/useShareImage.test.ts b/apps/mobile/src/lib/useShareImage.test.ts index e0edc86a01d..eec6a4eacec 100644 --- a/apps/mobile/src/lib/useShareImage.test.ts +++ b/apps/mobile/src/lib/useShareImage.test.ts @@ -60,6 +60,29 @@ describe("shareImageExclusively", () => { expect(mocks.shareImage).toHaveBeenCalledTimes(2); }); + it("expires the lock so a sheet that never settles cannot disable sharing", async () => { + vi.useFakeTimers(); + try { + // A share that hangs forever, as some Android targets do after a cancel. + mocks.shareImage.mockReturnValueOnce(new Promise(() => {})); + void shareImageExclusively({ uri: "https://example.test/a.png" }); + expect(mocks.shareImage).toHaveBeenCalledTimes(1); + + // Still held while the lock is fresh. + vi.setSystemTime(Date.now() + 59_000); + void shareImageExclusively({ uri: "https://example.test/b.png" }); + expect(mocks.shareImage).toHaveBeenCalledTimes(1); + + // Released once it goes stale. + vi.setSystemTime(Date.now() + 2_000); + mocks.shareImage.mockResolvedValue({ ok: true }); + await shareImageExclusively({ uri: "https://example.test/c.png" }); + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + } finally { + vi.useRealTimers(); + } + }); + it("releases the guard when shareImage throws", async () => { mocks.shareImage.mockRejectedValue(new Error("boom")); diff --git a/apps/mobile/src/lib/useShareImage.ts b/apps/mobile/src/lib/useShareImage.ts index 9a9e1e0f9fb..f50daf00d8f 100644 --- a/apps/mobile/src/lib/useShareImage.ts +++ b/apps/mobile/src/lib/useShareImage.ts @@ -6,21 +6,27 @@ import { shareImage, type FullScreenImageSource } from "./fullScreenImageActions // Module scope, not per hook. The hook is called once per thumbnail and again // by the fullscreen viewer, so a ref would guard each caller separately and // still let two long-presses stack two system sheets. -let sharing = false; +// +// Held as a start time rather than a boolean so the lock expires on its own. A +// share sheet that never settles would otherwise disable sharing app-wide until +// the app restarts, which is a worse failure than the stacking it prevents. +const SHARE_LOCK_TIMEOUT_MS = 60_000; +let shareStartedAt: number | null = null; /** Exported for tests. The hook is a thin wrapper around this. */ export async function shareImageExclusively(source: FullScreenImageSource): Promise { - if (sharing) { + const startedAt = Date.now(); + if (shareStartedAt !== null && startedAt - shareStartedAt < SHARE_LOCK_TIMEOUT_MS) { return; } - sharing = true; + shareStartedAt = startedAt; try { const result = await shareImage(source); if (!result.ok) { Alert.alert(result.message); } } finally { - sharing = false; + shareStartedAt = null; } } From 1bfbfdb153865e062cc138079e2e96d5cc09c5f2 Mon Sep 17 00:00:00 2001 From: MacKinley Smith Date: Fri, 31 Jul 2026 15:49:58 -0600 Subject: [PATCH 5/6] fix(mobile): only release the share lock its own share still holds 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. --- apps/mobile/src/lib/useShareImage.test.ts | 39 +++++++++++++++++++++++ apps/mobile/src/lib/useShareImage.ts | 13 +++++--- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/apps/mobile/src/lib/useShareImage.test.ts b/apps/mobile/src/lib/useShareImage.test.ts index eec6a4eacec..b2f445585c7 100644 --- a/apps/mobile/src/lib/useShareImage.test.ts +++ b/apps/mobile/src/lib/useShareImage.test.ts @@ -83,6 +83,45 @@ describe("shareImageExclusively", () => { } }); + it("does not let a stale share release the lock a newer share holds", async () => { + vi.useFakeTimers(); + try { + let releaseStale: (() => void) | undefined; + mocks.shareImage.mockReturnValueOnce( + new Promise((resolve) => { + releaseStale = () => resolve({ ok: true }); + }), + ); + // A hangs and its lock goes stale. + void shareImageExclusively({ uri: "https://example.test/a.png" }); + vi.setSystemTime(Date.now() + 61_000); + + // B takes the lock. + let releaseCurrent: (() => void) | undefined; + mocks.shareImage.mockReturnValueOnce( + new Promise((resolve) => { + releaseCurrent = () => resolve({ ok: true }); + }), + ); + const current = shareImageExclusively({ uri: "https://example.test/b.png" }); + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + + // A finally settles. It must not clear B's lock. + releaseStale?.(); + await Promise.resolve(); + await Promise.resolve(); + + void shareImageExclusively({ uri: "https://example.test/c.png" }); + expect(mocks.shareImage).toHaveBeenCalledTimes(2); + + // Leave the module-level lock free for the next test. + releaseCurrent?.(); + await current; + } finally { + vi.useRealTimers(); + } + }); + it("releases the guard when shareImage throws", async () => { mocks.shareImage.mockRejectedValue(new Error("boom")); diff --git a/apps/mobile/src/lib/useShareImage.ts b/apps/mobile/src/lib/useShareImage.ts index f50daf00d8f..8afb70a2cef 100644 --- a/apps/mobile/src/lib/useShareImage.ts +++ b/apps/mobile/src/lib/useShareImage.ts @@ -11,22 +11,27 @@ import { shareImage, type FullScreenImageSource } from "./fullScreenImageActions // share sheet that never settles would otherwise disable sharing app-wide until // the app restarts, which is a worse failure than the stacking it prevents. const SHARE_LOCK_TIMEOUT_MS = 60_000; -let shareStartedAt: number | null = null; +let activeShare: { readonly startedAt: number } | null = null; /** Exported for tests. The hook is a thin wrapper around this. */ export async function shareImageExclusively(source: FullScreenImageSource): Promise { const startedAt = Date.now(); - if (shareStartedAt !== null && startedAt - shareStartedAt < SHARE_LOCK_TIMEOUT_MS) { + if (activeShare !== null && startedAt - activeShare.startedAt < SHARE_LOCK_TIMEOUT_MS) { return; } - shareStartedAt = startedAt; + // Identity, not the timestamp: a share that settles after its lock expired + // must not release the lock a newer share has since taken. + const share = { startedAt }; + activeShare = share; try { const result = await shareImage(source); if (!result.ok) { Alert.alert(result.message); } } finally { - shareStartedAt = null; + if (activeShare === share) { + activeShare = null; + } } } From e9c3d1e6f7b17be3ea15178b530ab7894b1dc9ce Mon Sep 17 00:00:00 2001 From: MacKinley Smith Date: Sat, 1 Aug 2026 09:51:48 -0600 Subject: [PATCH 6/6] fix(mobile): copy Android content URIs into a file before sharing The previous commit routed content:// down the local branch, which was wrong. expo-sharing's Android module rejects any scheme that is not file: if ("file" != uri.scheme) throw InvalidArgumentException(...) so handing it a content URI trades one failure for another. The bytes are on the device but the sheet still needs a real file, so a content URI is now copied into the temporary directory and the copy is shared. --- .../src/lib/fullScreenImageActions.test.ts | 15 +++++++----- apps/mobile/src/lib/fullScreenImageActions.ts | 23 +++++++++++++++---- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/apps/mobile/src/lib/fullScreenImageActions.test.ts b/apps/mobile/src/lib/fullScreenImageActions.test.ts index ab8fc2001f5..b881df97f84 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.test.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test" const mocks = vi.hoisted(() => ({ create: vi.fn(), + copy: vi.fn(), directoryCreate: vi.fn(), directoryDelete: vi.fn(), downloadFileAsync: vi.fn(), @@ -18,6 +19,7 @@ class FakeFile { .join("/"); } create = (options?: unknown) => mocks.create(this.uri, options); + copy = (destination: FakeFile) => mocks.copy(this.uri, destination.uri); write = (content: string, options?: unknown) => mocks.write(this.uri, content, options); static downloadFileAsync = (url: string, destination: FakeFile, options?: unknown) => mocks.downloadFileAsync(url, destination, options); @@ -105,16 +107,17 @@ describe("shareImage", () => { expect(mocks.directoryDelete).not.toHaveBeenCalled(); }); - it("treats an Android content:// URI as local rather than downloading it", async () => { + it("copies an Android content:// URI into a real file before sharing", async () => { const result = await shareImage({ uri: "content://media/external/images/media/42" }); expect(result).toEqual({ ok: true }); expect(mocks.downloadFileAsync).not.toHaveBeenCalled(); - expect(mocks.shareAsync).toHaveBeenCalledWith( - "content://media/external/images/media/42", - expect.anything(), - ); - expect(mocks.directoryDelete).not.toHaveBeenCalled(); + // Android's shareAsync throws on any scheme other than file, so the sheet + // must never be handed the content URI itself. + expect(mocks.copy).toHaveBeenCalledTimes(1); + const sharedUri = mocks.shareAsync.mock.calls[0]?.[0] as string; + expect(sharedUri).not.toContain("content://"); + expect(mocks.directoryDelete).toHaveBeenCalledTimes(1); }); it("writes a data URI to a temp directory, shares it, then removes the directory", async () => { diff --git a/apps/mobile/src/lib/fullScreenImageActions.ts b/apps/mobile/src/lib/fullScreenImageActions.ts index 39624b3aa2b..6bc99537ffe 100644 --- a/apps/mobile/src/lib/fullScreenImageActions.ts +++ b/apps/mobile/src/lib/fullScreenImageActions.ts @@ -106,10 +106,17 @@ function temporaryFileName(source: FullScreenImageSource): string { return `${stem.length > 0 ? stem : "image"}.${extension}`; } -function isLocalFileUri(uri: string): boolean { - // Android hands back content:// for picked and shared media. It is already on - // the device, so it must not fall through to the download branch. - return uri.startsWith("file://") || uri.startsWith("content://") || uri.startsWith("/"); +/** + * Only a real file can be handed straight to the sheet. Android's `shareAsync` + * rejects every other scheme outright, so `content://` is copied first even + * though the bytes are already on the device. + */ +function isShareableFileUri(uri: string): boolean { + return uri.startsWith("file://") || uri.startsWith("/"); +} + +function isContentUri(uri: string): boolean { + return uri.startsWith("content://"); } /** @@ -147,7 +154,7 @@ type MaterializedImage = { async function materializeImageFile(source: FullScreenImageSource): Promise { const { File } = await import("expo-file-system"); - if (isLocalFileUri(source.uri)) { + if (isShareableFileUri(source.uri)) { return { file: new File(source.uri), temporaryDirectory: null }; } @@ -164,6 +171,12 @@ async function materializeImageFile(source: FullScreenImageSource): Promise