From 574ea1c7a34a49248b13f88cd4fd512274b373dd Mon Sep 17 00:00:00 2001 From: Samuel Newman Date: Fri, 4 Sep 2026 17:17:04 +0300 Subject: [PATCH] address file handling review --- __tests__/lib/images.test.ts | 45 ++++++++++++++++++++++++++++++ src/lib/media/manip.ts | 37 +++++++++++++++++++++--- src/state/shell/composer/index.tsx | 42 ++++++++++++++++++++++------ 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/__tests__/lib/images.test.ts b/__tests__/lib/images.test.ts index 29c0f11f55..5cc6f7be4e 100644 --- a/__tests__/lib/images.test.ts +++ b/__tests__/lib/images.test.ts @@ -1,12 +1,21 @@ import {File, Paths} from 'expo-file-system' import {ImageManipulator, SaveFormat} from 'expo-image-manipulator' +import * as Sharing from 'expo-sharing' import {IMAGE_SIZE_CONFIG_2K_1MB} from '../../src/lib/constants' import { downloadAndResize, type DownloadAndResizeOpts, + saveToDevice, + shareImageModal, } from '../../src/lib/media/manip' import {getResizedDimensions} from '../../src/lib/media/util' +import {logger} from '../../src/logger' + +jest.mock('expo-sharing', () => ({ + isAvailableAsync: jest.fn().mockResolvedValue(true), + shareAsync: jest.fn().mockResolvedValue(undefined), +})) const mockResizedImage = { size: 100, @@ -221,3 +230,39 @@ describe('downloadAndResize', () => { }) }) }) + +describe('temporary file lifecycles', () => { + beforeEach(() => { + ;(File.downloadFileAsync as jest.Mock).mockResolvedValue( + new File('file://downloaded-image.jpg'), + ) + }) + + afterEach(() => { + jest.clearAllMocks() + }) + + it('retains a successfully shared image for lazy consumers', async () => { + await shareImageModal({uri: 'https://example.com/image.jpg'}) + + const sharedPath = (Sharing.shareAsync as jest.Mock).mock.calls[0][0] + const deletedPaths = (Paths.info as jest.Mock).mock.calls.map( + ([path]) => path, + ) + expect(sharedPath).toContain('/bsky-share/') + expect(deletedPaths).not.toContain(sharedPath) + }) + + it('does not report user cancellation as an error', async () => { + const errorSpy = jest.spyOn(logger, 'error') + ;(Sharing.shareAsync as jest.Mock).mockRejectedValueOnce( + new Error('Picker cancelled'), + ) + + await expect( + saveToDevice('test.txt', 'dGVzdA==', 'text/plain'), + ).resolves.toBe(false) + expect(errorSpy).not.toHaveBeenCalled() + errorSpy.mockRestore() + }) +}) diff --git a/src/lib/media/manip.ts b/src/lib/media/manip.ts index ea4ec838cc..43c30dee17 100644 --- a/src/lib/media/manip.ts +++ b/src/lib/media/manip.ts @@ -5,6 +5,7 @@ import {SaveFormat} from 'expo-image-manipulator' import * as MediaLibrary from 'expo-media-library/legacy' import * as Sharing from 'expo-sharing' +import {isCancelledError} from '#/lib/strings/errors' import {logger} from '#/logger' import {IS_ANDROID, IS_IOS} from '#/env' import {renderImage} from './image-manipulator' @@ -67,9 +68,11 @@ export async function shareImageModal({uri}: {uri: string}) { return } + const shareDirectory = prepareShareDirectory() const downloadedPath = await downloadImage(uri, String(uuid.v4()), 15e3) let jpegUri: string | undefined let imagePath: string | undefined + let didShare = false try { const jpeg = await renderImage(downloadedPath, undefined, { @@ -77,15 +80,16 @@ export async function shareImageModal({uri}: {uri: string}) { compress: 1.0, }) jpegUri = jpeg.uri - imagePath = await moveToPermanentPath(jpegUri, '.jpg') + imagePath = await moveToPermanentPath(jpegUri, '.jpg', shareDirectory) await Sharing.shareAsync(imagePath, { mimeType: 'image/jpeg', UTI: 'image/jpeg', }) + didShare = true } finally { safeDelete(downloadedPath) if (jpegUri) safeDelete(jpegUri) - if (imagePath) safeDelete(imagePath) + if (imagePath && !didShare) safeDelete(imagePath) } } @@ -269,7 +273,11 @@ async function doResize( } } -async function moveToPermanentPath(path: string, ext: string): Promise { +async function moveToPermanentPath( + path: string, + ext: string, + destinationDirectory = Paths.cache, +): Promise { /* Since this package stores images in a temp directory, we need to move the file to a permanent location. Relevant: IOS bug when trying to open a second time: @@ -277,12 +285,30 @@ async function moveToPermanentPath(path: string, ext: string): Promise { */ const filename = uuid.v4() - const destination = new File(Paths.cache, filename + ext) + const destination = new File(destinationDirectory, filename + ext) await new File(normalizePath(path)).copy(destination) safeDelete(path) return normalizePath(destination.uri) } +function prepareShareDirectory(): Directory { + const directory = new Directory(Paths.cache, 'bsky-share') + + /* + * Keep the current file alive after the share sheet closes because Android + * targets may consume its content URI lazily. Remove the previous share the + * next time a share starts so the cache remains bounded. + */ + if (directory.exists) { + for (const entry of directory.list()) { + safeDelete(entry.uri) + } + } else { + directory.create({intermediates: true}) + } + return directory +} + export function safeDelete(path: string) { const normalizedPath = normalizePath(path) try { @@ -338,6 +364,9 @@ export async function saveToDevice( return true } } catch (e) { + if (isCancelledError(e)) { + return false + } logger.error('Error occurred while saving file', {message: e}) return false } diff --git a/src/state/shell/composer/index.tsx b/src/state/shell/composer/index.tsx index 0c60a77134..eec920d646 100644 --- a/src/state/shell/composer/index.tsx +++ b/src/state/shell/composer/index.tsx @@ -1,4 +1,12 @@ -import {createContext, useContext, useMemo, useState} from 'react' +import { + createContext, + useContext, + useEffect, + useMemo, + useRef, + useState, +} from 'react' +import {InteractionManager} from 'react-native' import {type ModerationDecision} from '@bsky/sdk/moderation' import {msg} from '@lingui/core/macro' import {useLingui} from '@lingui/react' @@ -68,6 +76,14 @@ export function Provider({children}: React.PropsWithChildren<{}>) { const {_} = useLingui() const [state, setState] = useState() const queryClient = useQueryClient() + const pendingPurgeRef = + useRef>( + undefined, + ) + + useEffect(() => { + return () => pendingPurgeRef.current?.cancel() + }, []) const openComposer = useNonReactiveCallback((opts: ComposerOpts) => { if (opts.quote) { @@ -97,6 +113,12 @@ export function Provider({children}: React.PropsWithChildren<{}>) { type: 'warning', }) } else { + /* + * A purge scheduled by the previous composer must not delete files from + * a composer that is opened before the callback gets to run. + */ + pendingPurgeRef.current?.cancel() + pendingPurgeRef.current = undefined setState(prevOpts => { if (prevOpts) { // Never replace an already open composer. @@ -111,13 +133,17 @@ export function Provider({children}: React.PropsWithChildren<{}>) { let wasOpen = !!state if (wasOpen) { setState(undefined) - purgeTemporaryImageFiles() - // Purging deletes cached thumbnails on disk, so remove the query - // caches that may hold references to those now-deleted file paths. - // Without this, restoring a draft would serve stale ResolvedLink - // data pointing at missing files, causing "Failed to load blob". - queryClient.removeQueries({queryKey: [RQKEY_LINK_ROOT]}) - queryClient.removeQueries({queryKey: [RQKEY_GIF_ROOT]}) + pendingPurgeRef.current?.cancel() + pendingPurgeRef.current = InteractionManager.runAfterInteractions(() => { + pendingPurgeRef.current = undefined + purgeTemporaryImageFiles() + // Purging deletes cached thumbnails on disk, so remove the query + // caches that may hold references to those now-deleted file paths. + // Without this, restoring a draft would serve stale ResolvedLink + // data pointing at missing files, causing "Failed to load blob". + queryClient.removeQueries({queryKey: [RQKEY_LINK_ROOT]}) + queryClient.removeQueries({queryKey: [RQKEY_GIF_ROOT]}) + }) } return wasOpen