From 646ef7e34415e9f267badb4b90866c6931d0484e Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:05:00 -0400 Subject: [PATCH 01/11] Let a cloud image be copied under a new id on the server A placed image's id is also its image's id, so copying one to another page of a cloud strategy needs a row of its own. images:copyAsset gives the copy's id a row pointing at the same stored bytes, as a duplicated strategy's images do, taking over a placeholder if the copy's content landed first. Additive: no client calls it yet. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 53 ++++ convex/imageCopy.test.ts | 295 ++++++++++++++++++++ convex/images.ts | 55 ++++ lib/collab/generated/convex_models.dart | 40 +++ lib/collab/generated/icarus_convex_api.dart | 25 ++ 5 files changed, 468 insertions(+) create mode 100644 convex/imageCopy.test.ts diff --git a/convex/function_spec.json b/convex/function_spec.json index 93232fbf..b56f025c 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -40101,6 +40101,59 @@ "kind": "public" } }, + { + "args": { + "type": "object", + "value": { + "clientProtocolVersion": { + "fieldType": { + "type": "number" + }, + "optional": false + }, + "sourceAssetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "strategyPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "targetAssetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + } + } + }, + "functionType": "Mutation", + "identifier": "images.js:copyAsset", + "returns": { + "type": "union", + "value": [ + { + "type": "literal", + "value": "copied" + }, + { + "type": "literal", + "value": "uploading" + }, + { + "type": "literal", + "value": "unavailable" + } + ] + }, + "visibility": { + "kind": "public" + } + }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts new file mode 100644 index 00000000..a8ba8db2 --- /dev/null +++ b/convex/imageCopy.test.ts @@ -0,0 +1,295 @@ +import { + convexTest, + type TestConvexForDataModel, + type TestConvexForDataModelAndIdentity, +} from "convex-test"; +import { makeFunctionReference } from "convex/server"; +import { afterEach, beforeAll, describe, expect, test, vi } from "vitest"; +import type { DataModel } from "./_generated/dataModel"; +import { markAssetReferencesReady } from "./lib/assetReferences"; +import { CURRENT_CLOUD_PROTOCOL_VERSION } from "./lib/cloudProtocol"; +import schema from "./schema"; +import { modules } from "./test.setup"; + +const ensureCurrentUser = makeFunctionReference<"mutation">( + "users:ensureCurrentUser", +); +const createStrategy = makeFunctionReference<"mutation">( + "strategies:createWithInitialPage", +); +const deleteStrategy = makeFunctionReference<"mutation">("strategies:delete"); +const addPage = makeFunctionReference<"mutation">("pages:add"); +const applyBatch = makeFunctionReference<"mutation">("ops:applyBatch"); +const purgeOldTombstones = makeFunctionReference<"mutation">( + "maintenance:purgeOldTombstones", +); +const listImages = makeFunctionReference<"query">("images:listForStrategy"); +const copyAsset = makeFunctionReference<"mutation">("images:copyAsset"); + +type Harness = TestConvexForDataModel; +type RootHarness = TestConvexForDataModelAndIdentity; + +const protocol = { clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION }; +const strategyPublicId = "copy-strategy"; +const firstPage = "copy-page-1"; +const secondPage = "copy-page-2"; +const original = "placed-image"; +const copy = `placed-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`; +const settings = { agentSize: 40, abilitySize: 30, useNeutralTeamColors: true }; + +function identity(subject: string) { + return { + issuer: "https://image-copy.test", + subject, + tokenIdentifier: `image-copy|${subject}`, + name: `User ${subject}`, + }; +} + +/// A two-page strategy whose first page shows a placed image, with the +/// image's upload [status] ("active" once its bytes landed). +async function createHarness(status: "active" | "pending" = "active"): Promise<{ + t: RootHarness; + owner: Harness; + other: Harness; +}> { + const t = convexTest(schema, modules); + await t.run(markAssetReferencesReady); + const owner = t.withIdentity(identity("owner")); + const other = t.withIdentity(identity("other")); + await owner.mutation(ensureCurrentUser, protocol); + await other.mutation(ensureCurrentUser, protocol); + await owner.mutation(createStrategy, { + ...protocol, + publicId: strategyPublicId, + name: "Bind B split", + mapData: "bind", + initialPagePublicId: firstPage, + initialPageName: "Setup", + initialPageIsAutoNamed: false, + initialPageIsAttack: true, + initialPageSettings: settings, + }); + await owner.mutation(addPage, { + ...protocol, + strategyPublicId, + pagePublicId: secondPage, + name: "Hit", + sortIndex: 1, + isAttack: true, + expectedRevision: 0, + }); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: original, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "placed-image-attempt", + objectKey: `strategies/${strategyPublicId}/${original}.png`, + uploadStatus: status, + fileExtension: ".png", + mimeType: "image/png", + width: 64, + height: 32, + byteSize: 100, + ...(status === "active" ? { uploadedAt: now } : {}), + createdAt: now, + updatedAt: now, + }); + }); + await addImage(owner, original, firstPage); + return { t, owner, other }; +} + +async function addImage(user: Harness, id: string, pagePublicId: string) { + await user.mutation(applyBatch, { + ...protocol, + strategyPublicId, + clientId: `add-${id}`, + ops: [ + { + opId: `add-${id}`, + type: "element.add", + elementPublicId: id, + pagePublicId, + payload: { + kind: "image", + payloadVersion: 1, + data: { id, elementType: "image", scale: 2 }, + }, + sortIndex: 1, + }, + ], + }); +} + +async function copyImage(user: Harness) { + return await user.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: copy, + }); +} + +type ImageRow = { publicId: string; uploadStatus: string; url: string | null }; + +async function images(user: Harness) { + const rows = (await user.query(listImages, { strategyPublicId })) as ImageRow[]; + return Object.fromEntries( + rows.map((row) => [row.publicId, [row.uploadStatus, row.url]]), + ); +} + +async function rowsFor(t: RootHarness, publicId: string) { + return await t.run(async (ctx) => + (await ctx.db.query("imageAssets").collect()).filter( + (row) => row.publicId === publicId, + ), + ); +} + +const url = `https://media.copy.test/strategies/${strategyPublicId}/${original}.png`; + +beforeAll(() => { + process.env.R2_ACCOUNT_ID = "copy-account"; + process.env.R2_BUCKET = "copy-bucket"; + process.env.R2_ACCESS_KEY_ID = "copy-access-key"; + process.env.R2_SECRET_ACCESS_KEY = "copy-secret"; + process.env.R2_PUBLIC_BASE_URL = "https://media.copy.test"; + process.env.R2_S3_ENDPOINT = "https://copy.r2.test"; +}); + +afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); +}); + +describe("images:copyAsset", () => { + test("a copied image shows the original's picture under its own id", async () => { + const { owner } = await createHarness(); + + expect(await copyImage(owner)).toBe("copied"); + await addImage(owner, copy, secondPage); + + expect(await images(owner)).toEqual({ + [original]: ["active", url], + [copy]: ["active", url], + }); + }); + + test("a copy whose content landed first takes over its placeholder", async () => { + const { t, owner } = await createHarness(); + await addImage(owner, copy, secondPage); + expect((await images(owner))[copy]).toEqual(["pending", null]); + + expect(await copyImage(owner)).toBe("copied"); + + expect((await images(owner))[copy]).toEqual(["active", url]); + expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ + "active", + ]); + }); + + test("copying again changes nothing", async () => { + const { t, owner } = await createHarness(); + await copyImage(owner); + + expect(await copyImage(owner)).toBe("copied"); + expect(await rowsFor(t, copy)).toHaveLength(1); + }); + + test("an image still uploading has nothing to copy yet", async () => { + const { t, owner } = await createHarness("pending"); + await addImage(owner, copy, secondPage); + + expect(await copyImage(owner)).toBe("uploading"); + // The copy's placeholder waits, as for any image on its way. + expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ + "pending", + ]); + }); + + test("an image the strategy cannot show is unavailable", async () => { + const { owner } = await createHarness(); + + expect( + await owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: "missing-image", + targetAssetPublicId: `missing-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`, + }), + ).toBe("unavailable"); + }); + + test("only an editor of the strategy can copy, and never onto itself", async () => { + const { owner, other } = await createHarness(); + + await expect(copyImage(other)).rejects.toThrow(); + await expect( + owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: original, + }), + ).rejects.toThrow(); + }); + + test("deleting the original keeps the copy's bytes; deleting both frees them", async () => { + vi.useFakeTimers(); + const fetchMock = vi.fn( + async (_input: RequestInfo | URL, _init?: RequestInit) => + new Response(null, { status: 204 }), + ); + vi.stubGlobal("fetch", fetchMock); + const { t, owner } = await createHarness(); + await copyImage(owner); + await addImage(owner, copy, secondPage); + + await owner.mutation(applyBatch, { + ...protocol, + strategyPublicId, + clientId: "remove-original", + ops: [ + { + opId: "remove-original", + type: "element.delete", + elementPublicId: original, + pagePublicId: firstPage, + expectedElementRevision: 1, + }, + ], + }); + vi.setSystemTime(Date.now() + 31 * 24 * 60 * 60 * 1000); + await t.mutation(purgeOldTombstones, {}); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect(fetchMock).not.toHaveBeenCalled(); + expect((await images(owner))[copy]).toEqual(["active", url]); + + const revision = await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + return strategy!.revision; + }); + await owner.mutation(deleteStrategy, { + ...protocol, + strategyPublicId, + expectedRevision: revision, + }); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect( + fetchMock.mock.calls.map((call) => new URL(String(call[0])).pathname), + ).toEqual([`/copy-bucket/strategies/${strategyPublicId}/${original}.png`]); + }); +}); diff --git a/convex/images.ts b/convex/images.ts index 945b30ec..48aaa523 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -7,6 +7,7 @@ import { isAssetReferenced, } from "./lib/assetReferences"; import { + copyActiveAssetToStrategy, getActiveAssetForStrategy, inferFileExtension, getViewerAssetForStrategy, @@ -23,6 +24,7 @@ import { internalAction, internalMutation, internalQuery, + mutation, query, type MutationCtx, type QueryCtx, @@ -383,6 +385,59 @@ export const createR2UploadIntent = internalMutation({ }, }); +/// Gives image `targetAssetPublicId` the picture the strategy already shows +/// as `sourceAssetPublicId`, for a placed image copied to another page: a +/// placed image's id is also its image's id, so the copy needs a row of its +/// own. The row points at the same stored bytes, as a duplicated strategy's +/// images do (`copyActiveAssetToStrategy`), so nothing is uploaded twice. +/// +/// The copy's content may reach the server first and leave a placeholder; +/// the copied row replaces it. Copying again is harmless. "uploading" means +/// the source's upload has not finished, so there is nothing to copy yet; +/// "unavailable" means the strategy cannot show the source either. +export const copyAsset = mutation({ + args: { + ...cloudProtocolArgs, + strategyPublicId: v.string(), + sourceAssetPublicId: v.string(), + targetAssetPublicId: v.string(), + }, + returns: v.union( + v.literal("copied"), + v.literal("uploading"), + v.literal("unavailable"), + ), + handler: async (ctx, args) => { + assertSupportedCloudProtocol(args.clientProtocolVersion); + if (args.sourceAssetPublicId === args.targetAssetPublicId) { + throw invalidPayloadError("An image cannot be copied onto itself."); + } + const strategy = await getStrategyByPublicId(ctx, args.strategyPublicId); + const { user } = await assertStrategyRole(ctx, strategy, "editor"); + + const target = await getViewerAssetForStrategy( + ctx, + strategy._id, + args.targetAssetPublicId, + ); + if (target !== null && !isUploadPlaceholder(target)) { + return inferUploadStatus(target) === "active" ? "copied" : "uploading"; + } + const copied = await copyActiveAssetToStrategy(ctx, { + sourceStrategyId: strategy._id, + sourceAssetPublicId: args.sourceAssetPublicId, + targetStrategyId: strategy._id, + targetAssetPublicId: args.targetAssetPublicId, + userId: user._id, + now: Date.now(), + }); + if (copied === "copied" && target !== null) { + await ctx.db.delete(target._id); + } + return copied; + }, +}); + export const completeUpload = action({ args: { ...cloudProtocolArgs, diff --git a/lib/collab/generated/convex_models.dart b/lib/collab/generated/convex_models.dart index 8d05c6ab..9b121823 100644 --- a/lib/collab/generated/convex_models.dart +++ b/lib/collab/generated/convex_models.dart @@ -221,6 +221,25 @@ enum ImagesCompleteUploadArgsProvider { } } +enum ImagesCopyAssetResult { + copied('copied'), + unavailable('unavailable'), + uploading('uploading'); + + const ImagesCopyAssetResult(this.wireName); + final String wireName; + + static ImagesCopyAssetResult fromWireName(String wireName, String path) { + for (final value in values) { + if (value.wireName == wireName) return value; + } + throw ConvexDecodingException( + path, + 'unknown ImagesCopyAssetResult $wireName', + ); + } +} + enum ImagesGenerateUploadUrlResultProvider { r2('r2'); @@ -5410,6 +5429,27 @@ ImagesCompleteUploadResult decodeImagesCompleteUploadResult( 'images.js:completeUpload.returns', ); +ConvexObject encodeImagesCopyAssetArgs({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, +}) => ConvexObject({ + 'clientProtocolVersion': _encodeNumber( + clientProtocolVersion, + 'images.js:copyAsset.args.clientProtocolVersion', + ), + 'sourceAssetPublicId': ConvexString(sourceAssetPublicId), + 'strategyPublicId': ConvexString(strategyPublicId), + 'targetAssetPublicId': ConvexString(targetAssetPublicId), +}); + +ImagesCopyAssetResult decodeImagesCopyAssetResult(ConvexValue value) => + ImagesCopyAssetResult.fromWireName( + _decodeString(value, 'images.js:copyAsset.returns'), + 'images.js:copyAsset.returns', + ); + ConvexObject encodeImagesDeleteAssetRefArgs({ required String assetPublicId, required double clientProtocolVersion, diff --git a/lib/collab/generated/icarus_convex_api.dart b/lib/collab/generated/icarus_convex_api.dart index a4facdd0..12597b9f 100644 --- a/lib/collab/generated/icarus_convex_api.dart +++ b/lib/collab/generated/icarus_convex_api.dart @@ -408,6 +408,12 @@ abstract interface class ImagesModule { ConvexOptional uploadId = const ConvexOptional.absent(), ConvexOptional width = const ConvexOptional.absent(), }); + Future copyAsset({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, + }); Future deleteAssetRef({ required String assetPublicId, required double clientProtocolVersion, @@ -478,6 +484,25 @@ final class _ImagesModule implements ImagesModule { ); } + @override + Future copyAsset({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, + }) { + final args = encodeImagesCopyAssetArgs( + clientProtocolVersion: clientProtocolVersion, + sourceAssetPublicId: sourceAssetPublicId, + strategyPublicId: strategyPublicId, + targetAssetPublicId: targetAssetPublicId, + ); + return _invoke( + () => _transport.mutation('images:copyAsset', args), + decodeImagesCopyAssetResult, + ); + } + @override Future deleteAssetRef({ required String assetPublicId, From 8f39a67ab57987ae30000ed637af6f8efca66422 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:16:33 -0400 Subject: [PATCH 02/11] Copy placed images to the next or previous cloud page An image's id also names its picture on the server, so a copy, which gets an id of its own, first has the server give that id the original's picture (images:copyAsset, sharing the stored bytes), and only then is sent. An image still uploading, or one the cloud cannot show, is not copied, and a toast says why. Co-Authored-By: Claude Opus 5.5 --- lib/collab/cloud_media_models.dart | 13 ++ lib/collab/convex_strategy_repository.dart | 21 ++++ lib/providers/strategy_provider.dart | 42 ++++++- .../adjacent_page_copy_menu.dart | 10 ++ test/strategy_page_session_provider_test.dart | 111 +++++++++++++++--- 5 files changed, 175 insertions(+), 22 deletions(-) diff --git a/lib/collab/cloud_media_models.dart b/lib/collab/cloud_media_models.dart index 0e1713ab..6cbb6e1b 100644 --- a/lib/collab/cloud_media_models.dart +++ b/lib/collab/cloud_media_models.dart @@ -142,6 +142,19 @@ class CloudMediaUploadJob { } } +/// What became of giving a copied image the picture of the image it was +/// copied from (images:copyAsset). +enum CloudImageCopyResult { + /// The copy shows the original's picture. + copied, + + /// The original's upload has not finished, so there is nothing to copy yet. + uploading, + + /// The strategy cannot show the original either. + unavailable, +} + class CloudImageUploadIntent { const CloudImageUploadIntent({ required this.provider, diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 89c6330b..0cdd785b 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -231,6 +231,27 @@ class ConvexStrategyRepository { )); } + /// Gives image [targetAssetPublicId] the picture the strategy shows as + /// [sourceAssetPublicId], sharing its stored bytes: for a placed image + /// copied to another page, whose id is also its image's id. + Future copyImageAsset({ + required String strategyPublicId, + required String sourceAssetPublicId, + required String targetAssetPublicId, + }) async { + final result = await _api.images.copyAsset( + clientProtocolVersion: currentCloudProtocolVersion.toDouble(), + strategyPublicId: strategyPublicId, + sourceAssetPublicId: sourceAssetPublicId, + targetAssetPublicId: targetAssetPublicId, + ); + return switch (result) { + ImagesCopyAssetResult.copied => CloudImageCopyResult.copied, + ImagesCopyAssetResult.uploading => CloudImageCopyResult.uploading, + ImagesCopyAssetResult.unavailable => CloudImageCopyResult.unavailable, + }; + } + Future generateImageUploadUrl({ required String strategyPublicId, required String assetPublicId, diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 9a46ea79..f65b20d2 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -37,6 +37,7 @@ import 'package:path/path.dart' as path; import 'package:path_provider/path_provider.dart'; import 'package:uuid/uuid.dart'; import 'package:icarus/collab/canonical_json.dart'; +import 'package:icarus/collab/cloud_media_models.dart'; import 'package:icarus/collab/collab_models.dart'; import 'package:icarus/collab/strategy_capabilities.dart'; import 'package:icarus/collab/convex_strategy_repository.dart'; @@ -74,6 +75,12 @@ enum PageCopyResult { /// There was nothing to copy, or no page to copy it to. unavailable, + + /// The image's upload has not finished, so it cannot be copied yet. + imageUploading, + + /// The server cannot show the image, so it was not copied. + imageUnavailable, } class StrategyProvider extends Notifier { @@ -942,9 +949,7 @@ class StrategyProvider extends Notifier { } /// [widgetId] on the page on screen as cloud element data, or null when it - /// cannot go to another cloud page. An image's id also names its file on - /// the server, so a copy of one needs its own file; images stay local-only - /// for now. + /// cannot go to another cloud page. ({String kind, Map data})? _cloudElementOnScreen( String widgetId, ) { @@ -971,6 +976,11 @@ class StrategyProvider extends Notifier { for (final utility in ref.read(utilityProvider)) { if (utility.id == widgetId) return element('utility', utility.toJson()); } + for (final image in ref.read(placedImageProvider).images) { + if (image.id == widgetId) { + return element('image', cloudImagePayloadFromPlacedImage(image)); + } + } return null; } @@ -1050,6 +1060,32 @@ class StrategyProvider extends Notifier { 255) { return PageCopyResult.unavailable; } + // An image's id also names its picture on the server, so the copy gets + // the original's picture under its own id first. It shares the stored + // bytes, so nothing is uploaded again. + if (element.kind == 'image') { + final CloudImageCopyResult picture; + try { + picture = + await ref.read(convexStrategyRepositoryProvider).copyImageAsset( + strategyPublicId: strategyId, + sourceAssetPublicId: widgetId, + targetAssetPublicId: copyId, + ); + } catch (error) { + log('Could not copy the picture of image $widgetId: $error'); + return PageCopyResult.unreachable; + } + switch (picture) { + case CloudImageCopyResult.uploading: + return PageCopyResult.imageUploading; + case CloudImageCopyResult.unavailable: + return PageCopyResult.imageUnavailable; + case CloudImageCopyResult.copied: + break; + } + if (state.strategyId != strategyId) return PageCopyResult.unavailable; + } // The canvas never draws the copy: its page shows it from the server. final queued = await ref.read(strategyOpQueueProvider.notifier).enqueueOffCanvas( diff --git a/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart b/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart index ee18d4a2..c9fbf8e4 100644 --- a/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart +++ b/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart @@ -39,6 +39,16 @@ List buildAdjacentPageCopyMenuItems( 'copied.', backgroundColor: Settings.tacticalVioletTheme.destructive, ); + case PageCopyResult.imageUploading: + Settings.showToast( + message: 'The image is still uploading. Copy it again in a moment.', + backgroundColor: Settings.tacticalVioletTheme.primary, + ); + case PageCopyResult.imageUnavailable: + Settings.showToast( + message: "The cloud doesn't have this image, so it wasn't copied.", + backgroundColor: Settings.tacticalVioletTheme.destructive, + ); case PageCopyResult.copied || PageCopyResult.unavailable: break; } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index ba16808d..229fe50a 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11523,26 +11523,81 @@ void main() { expect(adds(container).single.pagePublicId, 'page-3'); }); - test('an image is not offered: its copy would need a file of its own', - () async { - final (container, _, _) = await open(); - container.read(placedImageProvider.notifier).fromHive([ - PlacedImage( - id: 'image', - position: Offset.zero, - aspectRatio: 1, - scale: 100, - fileExtension: '.png', - ), - ]); - await _settle(); + group('an image', () { + /// Opens page 2 with a placed image on it, whose picture the server + /// copies with [result] (null: the call fails, as when offline). + Future<(ProviderContainer, _PageReader)> openWithImage( + CloudImageCopyResult? result, + ) async { + final (container, _, reader) = await open(); + reader.imageCopy = result; + container.read(placedImageProvider.notifier).fromHive([ + PlacedImage( + id: 'image', + position: const Offset(12, 34), + aspectRatio: 1.5, + scale: 100, + fileExtension: '.png', + ), + ]); + await _settle(); + return (container, reader); + } - expect( - container - .read(strategyProvider.notifier) - .copyDirectionsForPlacedWidget('image'), - isEmpty, - ); + Iterable copies(ProviderContainer container) => + adds(container).where((op) => op.pagePublicId == 'page-3'); + + Future copy(ProviderContainer container) => container + .read(strategyProvider.notifier) + .copyPlacedWidgetToAdjacentPage( + widgetId: 'image', + direction: PageTransitionDirection.forward, + ); + + test('gets its picture copied under the new id, then is sent', () async { + final (container, reader) = + await openWithImage(CloudImageCopyResult.copied); + expect( + container + .read(strategyProvider.notifier) + .copyDirectionsForPlacedWidget('image'), + [PageTransitionDirection.forward, PageTransitionDirection.backward], + ); + + expect(await copy(container), PageCopyResult.copied); + + final add = copies(container).single; + expect(add.payload['kind'], 'image'); + final data = cloudPayloadData(add.payload); + expect(data['id'], add.elementPublicId); + expect(data['aspectRatio'], 1.5); + expect(pageCopyRoot(add.elementPublicId), 'image'); + expect(reader.imageCopies, [('image', add.elementPublicId)]); + await _settle(); + }); + + test('still uploading is not copied yet', () async { + final (container, _) = + await openWithImage(CloudImageCopyResult.uploading); + + expect(await copy(container), PageCopyResult.imageUploading); + expect(copies(container), isEmpty); + }); + + test('the cloud cannot show is not copied', () async { + final (container, _) = + await openWithImage(CloudImageCopyResult.unavailable); + + expect(await copy(container), PageCopyResult.imageUnavailable); + expect(copies(container), isEmpty); + }); + + test('whose picture cannot be copied right now is not copied', () async { + final (container, _) = await openWithImage(null); + + expect(await copy(container), PageCopyResult.unreachable); + expect(copies(container), isEmpty); + }); }); }); } @@ -11559,6 +11614,24 @@ class _PageReader extends Fake implements ConvexStrategyRepository { /// While set, a read waits for it. Completer? gate; + /// What copying an image's picture returns; null: the call fails. + CloudImageCopyResult? imageCopy = CloudImageCopyResult.copied; + + /// The pictures copied, as (source, target) image ids. + final List<(String, String)> imageCopies = []; + + @override + Future copyImageAsset({ + required String strategyPublicId, + required String sourceAssetPublicId, + required String targetAssetPublicId, + }) async { + final result = imageCopy; + if (result == null) throw const SocketException('offline'); + imageCopies.add((sourceAssetPublicId, targetAssetPublicId)); + return result; + } + @override Future fetchPageSnapshot({ required String strategyPublicId, From a692d7c2ff47fcae013eb24cf8b8e8f728501a0e Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:31:24 -0400 Subject: [PATCH 03/11] Replace a failed upload with a copy; reclaim copies nothing uses Astra's review: a copied image whose content never reaches the server kept its row, and so its bytes, for good. A day after each copy, the copy now goes through the reclaim check, which removes it only if nothing shows it. A failed upload under the copy's id no longer reads as one still on its way: the copy replaces it. Tests cover both, and a viewer collaborator being refused. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 26 ++++++++++++++ convex/imageCopy.test.ts | 73 +++++++++++++++++++++++++++++++++++++++ convex/images.ts | 40 ++++++++++++++++++--- 3 files changed, 135 insertions(+), 4 deletions(-) diff --git a/convex/function_spec.json b/convex/function_spec.json index b56f025c..2ce6921e 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -41018,6 +41018,32 @@ "kind": "internal" } }, + { + "args": { + "type": "object", + "value": { + "assetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "strategyId": { + "fieldType": { + "tableName": "strategies", + "type": "id" + }, + "optional": false + } + } + }, + "functionType": "Mutation", + "identifier": "images.js:reclaimCopiedAssetIfUnused", + "returns": null, + "visibility": { + "kind": "internal" + } + }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index a8ba8db2..10a85278 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -25,6 +25,8 @@ const purgeOldTombstones = makeFunctionReference<"mutation">( ); const listImages = makeFunctionReference<"query">("images:listForStrategy"); const copyAsset = makeFunctionReference<"mutation">("images:copyAsset"); +const createShare = makeFunctionReference<"mutation">("shares:create"); +const redeemShare = makeFunctionReference<"mutation">("shares:redeem"); type Harness = TestConvexForDataModel; type RootHarness = TestConvexForDataModelAndIdentity; @@ -204,6 +206,62 @@ describe("images:copyAsset", () => { expect(await rowsFor(t, copy)).toHaveLength(1); }); + test("a copy replaces a failed upload under its id", async () => { + const { t, owner } = await createHarness(); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: copy, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "failed-attempt", + objectKey: `strategies/${strategyPublicId}/failed.png`, + uploadStatus: "failed", + fileExtension: ".png", + mimeType: "image/png", + createdAt: now, + updatedAt: now, + }); + }); + await addImage(owner, copy, secondPage); + + expect(await copyImage(owner)).toBe("copied"); + expect((await images(owner))[copy]).toEqual(["active", url]); + }); + + test("a copy nothing shows is removed a day later; one in use stays", async () => { + vi.useFakeTimers(); + const fetchMock = vi.fn(async () => new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetchMock); + const { t, owner } = await createHarness(); + const unused = `placed-image~cp1~0b1c2d3e-4f50-4a6b-8c7d-9e0f1a2b3c4d`; + await copyImage(owner); + await addImage(owner, copy, secondPage); + await owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: unused, + }); + + vi.setSystemTime(Date.now() + 25 * 60 * 60 * 1000); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect((await rowsFor(t, unused)).map((row) => row.uploadStatus)).toEqual( + [], + ); + // Its bytes are the original's, so they stay. + expect(fetchMock).not.toHaveBeenCalled(); + expect(await images(owner)).toEqual({ + [original]: ["active", url], + [copy]: ["active", url], + }); + }); + test("an image still uploading has nothing to copy yet", async () => { const { t, owner } = await createHarness("pending"); await addImage(owner, copy, secondPage); @@ -232,6 +290,21 @@ describe("images:copyAsset", () => { const { owner, other } = await createHarness(); await expect(copyImage(other)).rejects.toThrow(); + // A collaborator who may only view cannot copy; an editor can. + for (const role of ["viewer", "editor"] as const) { + await owner.mutation(createShare, { + ...protocol, + targetType: "strategy", + targetPublicId: strategyPublicId, + token: `as-${role}`, + role, + }); + } + await other.mutation(redeemShare, { ...protocol, token: "as-viewer" }); + await expect(copyImage(other)).rejects.toThrow(); + await other.mutation(redeemShare, { ...protocol, token: "as-editor" }); + expect(await copyImage(other)).toBe("copied"); + await expect( owner.mutation(copyAsset, { ...protocol, diff --git a/convex/images.ts b/convex/images.ts index 48aaa523..b8713d4d 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -88,6 +88,9 @@ export const markDeletedStrategyImageAssetsRef = makeFunctionReference<"mutation">("images:markDeletedStrategyImageAssets"); export const processAssetReclaimCandidatesRef = makeFunctionReference<"mutation">("images:processAssetReclaimCandidates"); +const reclaimCopiedAssetIfUnusedRef = makeFunctionReference<"mutation">( + "images:reclaimCopiedAssetIfUnused", +); const markDeletedPageImageAssetsRef = makeFunctionReference<"mutation">( "images:markDeletedPageImageAssets", ); @@ -392,9 +395,14 @@ export const createR2UploadIntent = internalMutation({ /// images do (`copyActiveAssetToStrategy`), so nothing is uploaded twice. /// /// The copy's content may reach the server first and leave a placeholder; -/// the copied row replaces it. Copying again is harmless. "uploading" means -/// the source's upload has not finished, so there is nothing to copy yet; +/// the copied row replaces it, as it replaces a failed upload's. Copying +/// again is harmless. "uploading" means an upload is still on its way (the +/// source's, so there is nothing to copy yet, or one under the target's id); /// "unavailable" means the strategy cannot show the source either. +/// +/// Content that never arrives (its page deleted first, say) would leave the +/// row unused for good, so a day later the copy goes through the reclaim +/// check, which removes it only if nothing shows it. export const copyAsset = mutation({ args: { ...cloudProtocolArgs, @@ -421,7 +429,12 @@ export const copyAsset = mutation({ args.targetAssetPublicId, ); if (target !== null && !isUploadPlaceholder(target)) { - return inferUploadStatus(target) === "active" ? "copied" : "uploading"; + const status = inferUploadStatus(target); + if (status === "active") return "copied"; + if (status === "pending") return "uploading"; + // A failed upload under the target's id: the copied row, being newer, + // is the one readers see, and the stale-upload sweep removes the + // failed one. } const copied = await copyActiveAssetToStrategy(ctx, { sourceStrategyId: strategy._id, @@ -431,13 +444,32 @@ export const copyAsset = mutation({ userId: user._id, now: Date.now(), }); - if (copied === "copied" && target !== null) { + if (copied !== "copied") return copied; + if (target !== null && isUploadPlaceholder(target)) { await ctx.db.delete(target._id); } + await ctx.scheduler.runAfter( + staleUploadAgeMs, + reclaimCopiedAssetIfUnusedRef, + { strategyId: strategy._id, assetPublicId: args.targetAssetPublicId }, + ); return copied; }, }); +/// Puts a copied image through the reclaim check a day after the copy (see +/// [copyAsset]): it is removed only if no content, live or restorable, +/// shows it. +export const reclaimCopiedAssetIfUnused = internalMutation({ + args: { + strategyId: v.id("strategies"), + assetPublicId: v.string(), + }, + handler: async (ctx, args) => { + await queueAssetReclaim(ctx, args.strategyId, [args.assetPublicId]); + }, +}); + export const completeUpload = action({ args: { ...cloudProtocolArgs, From f49e0c3297ae1173de609c6d78d82ef9ab267dac Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:37:45 -0400 Subject: [PATCH 04/11] Keep a copied image's row until something that showed it goes Astra's review of the app side: a copy can wait in a device's outbox for days and still lands needing its picture, so the day-later reclaim of unused copies broke copies delivered late. Copied rows now go as any image does, once content that showed it is purged or with the strategy; a copy whose content never arrives keeps its row until then. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 26 -------------------------- convex/imageCopy.test.ts | 23 +++++------------------ convex/images.ts | 28 ++++------------------------ 3 files changed, 9 insertions(+), 68 deletions(-) diff --git a/convex/function_spec.json b/convex/function_spec.json index 2ce6921e..b56f025c 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -41018,32 +41018,6 @@ "kind": "internal" } }, - { - "args": { - "type": "object", - "value": { - "assetPublicId": { - "fieldType": { - "type": "string" - }, - "optional": false - }, - "strategyId": { - "fieldType": { - "tableName": "strategies", - "type": "id" - }, - "optional": false - } - } - }, - "functionType": "Mutation", - "identifier": "images.js:reclaimCopiedAssetIfUnused", - "returns": null, - "visibility": { - "kind": "internal" - } - }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index 10a85278..7ce8569c 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -233,33 +233,20 @@ describe("images:copyAsset", () => { expect((await images(owner))[copy]).toEqual(["active", url]); }); - test("a copy nothing shows is removed a day later; one in use stays", async () => { + test("a copy whose content arrives days later still has its picture", async () => { vi.useFakeTimers(); const fetchMock = vi.fn(async () => new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchMock); const { t, owner } = await createHarness(); - const unused = `placed-image~cp1~0b1c2d3e-4f50-4a6b-8c7d-9e0f1a2b3c4d`; await copyImage(owner); - await addImage(owner, copy, secondPage); - await owner.mutation(copyAsset, { - ...protocol, - strategyPublicId, - sourceAssetPublicId: original, - targetAssetPublicId: unused, - }); - vi.setSystemTime(Date.now() + 25 * 60 * 60 * 1000); + // The copy waited in a device's outbox, offline, for three days. + vi.setSystemTime(Date.now() + 3 * 24 * 60 * 60 * 1000); await t.finishAllScheduledFunctions(vi.runAllTimers); + await addImage(owner, copy, secondPage); - expect((await rowsFor(t, unused)).map((row) => row.uploadStatus)).toEqual( - [], - ); - // Its bytes are the original's, so they stay. + expect((await images(owner))[copy]).toEqual(["active", url]); expect(fetchMock).not.toHaveBeenCalled(); - expect(await images(owner)).toEqual({ - [original]: ["active", url], - [copy]: ["active", url], - }); }); test("an image still uploading has nothing to copy yet", async () => { diff --git a/convex/images.ts b/convex/images.ts index b8713d4d..986144b3 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -88,9 +88,6 @@ export const markDeletedStrategyImageAssetsRef = makeFunctionReference<"mutation">("images:markDeletedStrategyImageAssets"); export const processAssetReclaimCandidatesRef = makeFunctionReference<"mutation">("images:processAssetReclaimCandidates"); -const reclaimCopiedAssetIfUnusedRef = makeFunctionReference<"mutation">( - "images:reclaimCopiedAssetIfUnused", -); const markDeletedPageImageAssetsRef = makeFunctionReference<"mutation">( "images:markDeletedPageImageAssets", ); @@ -400,9 +397,10 @@ export const createR2UploadIntent = internalMutation({ /// source's, so there is nothing to copy yet, or one under the target's id); /// "unavailable" means the strategy cannot show the source either. /// -/// Content that never arrives (its page deleted first, say) would leave the -/// row unused for good, so a day later the copy goes through the reclaim -/// check, which removes it only if nothing shows it. +/// The row is not swept on a timer: the copy's content may wait in a +/// device's outbox for days and still needs it. It goes as any image does, +/// once content that showed it is purged, or with the strategy; a copy whose +/// content never arrives keeps its row until then. export const copyAsset = mutation({ args: { ...cloudProtocolArgs, @@ -448,28 +446,10 @@ export const copyAsset = mutation({ if (target !== null && isUploadPlaceholder(target)) { await ctx.db.delete(target._id); } - await ctx.scheduler.runAfter( - staleUploadAgeMs, - reclaimCopiedAssetIfUnusedRef, - { strategyId: strategy._id, assetPublicId: args.targetAssetPublicId }, - ); return copied; }, }); -/// Puts a copied image through the reclaim check a day after the copy (see -/// [copyAsset]): it is removed only if no content, live or restorable, -/// shows it. -export const reclaimCopiedAssetIfUnused = internalMutation({ - args: { - strategyId: v.id("strategies"), - assetPublicId: v.string(), - }, - handler: async (ctx, args) => { - await queueAssetReclaim(ctx, args.strategyId, [args.assetPublicId]); - }, -}); - export const completeUpload = action({ args: { ...cloudProtocolArgs, From bb2001ca510c674ef46f98cd443a33eaee5835e4 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:40:36 -0400 Subject: [PATCH 05/11] Call an image this device is still uploading uploading, not missing Astra's review: an image placed on this device whose upload had not reached the server yet read as one the cloud doesn't have. The copy now checks the upload queue and says it is still uploading. The test also proves the copy waits for its picture before it is queued. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 13 ++++++- test/strategy_page_session_provider_test.dart | 37 ++++++++++++++++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index f65b20d2..3ea172b4 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1080,7 +1080,18 @@ class StrategyProvider extends Notifier { case CloudImageCopyResult.uploading: return PageCopyResult.imageUploading; case CloudImageCopyResult.unavailable: - return PageCopyResult.imageUnavailable; + // An image this device placed may not have reached the server yet. + final stillUploading = ref + .read(cloudMediaUploadQueueProvider) + .jobsForStrategy(strategyId) + .any( + (job) => + job.assetPublicId == widgetId && + job.state != CloudMediaJobState.failed, + ); + return stillUploading + ? PageCopyResult.imageUploading + : PageCopyResult.imageUnavailable; case CloudImageCopyResult.copied: break; } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 229fe50a..0ab7b943 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11564,7 +11564,13 @@ void main() { [PageTransitionDirection.forward, PageTransitionDirection.backward], ); - expect(await copy(container), PageCopyResult.copied); + // Nothing is queued until the picture is copied. + final picture = reader.imageCopyGate = Completer(); + final copied = copy(container); + await _settle(); + expect(copies(container), isEmpty); + picture.complete(); + expect(await copied, PageCopyResult.copied); final add = copies(container).single; expect(add.payload['kind'], 'image'); @@ -11584,6 +11590,31 @@ void main() { expect(copies(container), isEmpty); }); + test('this device is still uploading is not copied yet', () async { + final (container, _) = + await openWithImage(CloudImageCopyResult.unavailable); + container.read(cloudMediaUploadQueueProvider.notifier).state = + CloudMediaUploadQueueState( + jobs: [ + CloudMediaUploadJob( + jobId: 'image', + accountId: 'account-a', + strategyPublicId: 'cloud-strategy', + assetPublicId: 'image', + fileExtension: '.png', + mimeType: 'image/png', + state: CloudMediaJobState.pendingUpload, + attempts: 0, + updatedAt: DateTime.utc(2026), + ), + ], + isProcessing: false, + ); + + expect(await copy(container), PageCopyResult.imageUploading); + expect(copies(container), isEmpty); + }); + test('the cloud cannot show is not copied', () async { final (container, _) = await openWithImage(CloudImageCopyResult.unavailable); @@ -11617,6 +11648,9 @@ class _PageReader extends Fake implements ConvexStrategyRepository { /// What copying an image's picture returns; null: the call fails. CloudImageCopyResult? imageCopy = CloudImageCopyResult.copied; + /// While set, copying an image's picture waits for it. + Completer? imageCopyGate; + /// The pictures copied, as (source, target) image ids. final List<(String, String)> imageCopies = []; @@ -11626,6 +11660,7 @@ class _PageReader extends Fake implements ConvexStrategyRepository { required String sourceAssetPublicId, required String targetAssetPublicId, }) async { + await imageCopyGate?.future; final result = imageCopy; if (result == null) throw const SocketException('offline'); imageCopies.add((sourceAssetPublicId, targetAssetPublicId)); From f3922cb5c8746368cf3e089d677a2edef2570fbf Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:42:28 -0400 Subject: [PATCH 06/11] Refuse a copy onto an id that already shows another image CodeRabbit's review: an active row under the target's id counted as the copy whatever it showed. It now counts only if it has the source's stored bytes; otherwise the copy is refused. Co-Authored-By: Claude Opus 5.5 --- convex/imageCopy.test.ts | 29 +++++++++++++++++++++++++++++ convex/images.ts | 18 +++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index 7ce8569c..18bbbd8a 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -206,6 +206,35 @@ describe("images:copyAsset", () => { expect(await rowsFor(t, copy)).toHaveLength(1); }); + test("an id that already shows another image is refused", async () => { + const { t, owner } = await createHarness(); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: copy, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "other-attempt", + objectKey: `strategies/${strategyPublicId}/other.png`, + uploadStatus: "active", + fileExtension: ".png", + mimeType: "image/png", + uploadedAt: now, + createdAt: now, + updatedAt: now, + }); + }); + + await expect(copyImage(owner)).rejects.toThrow(); + expect((await rowsFor(t, copy)).map((row) => row.objectKey)).toEqual([ + `strategies/${strategyPublicId}/other.png`, + ]); + }); + test("a copy replaces a failed upload under its id", async () => { const { t, owner } = await createHarness(); await t.run(async (ctx) => { diff --git a/convex/images.ts b/convex/images.ts index 986144b3..1438a1ae 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -428,7 +428,23 @@ export const copyAsset = mutation({ ); if (target !== null && !isUploadPlaceholder(target)) { const status = inferUploadStatus(target); - if (status === "active") return "copied"; + if (status === "active") { + // Copied before, or another image entirely: only a row with the + // source's bytes is this copy. + const source = await getActiveAssetForStrategy( + ctx, + strategy._id, + args.sourceAssetPublicId, + ); + if ( + source !== null && + (source.objectKey !== target.objectKey || + source.storageId !== target.storageId) + ) { + throw conflictError("That image id already shows another image."); + } + return "copied"; + } if (status === "pending") return "uploading"; // A failed upload under the target's id: the copied row, being newer, // is the one readers see, and the stale-upload sweep removes the From 610133f1a95822f194dbe66c462d957cdf7d873f Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:45:54 -0400 Subject: [PATCH 07/11] Don't call an image uploading when its upload will never go Astra's review: an image whose own save failed keeps an upload job that never runs, since an upload waits for its image to be saved to send. A copy of it said "still uploading" for good. Only uploads that will go count now. Co-Authored-By: Claude Opus 5.5 --- lib/providers/strategy_provider.dart | 3 ++ test/strategy_page_session_provider_test.dart | 35 ++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 3ea172b4..7d0c3116 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1081,12 +1081,15 @@ class StrategyProvider extends Notifier { return PageCopyResult.imageUploading; case CloudImageCopyResult.unavailable: // An image this device placed may not have reached the server yet. + // Its upload only goes once the image itself is saved to send + // (referenceDurable); one whose save failed never will. final stillUploading = ref .read(cloudMediaUploadQueueProvider) .jobsForStrategy(strategyId) .any( (job) => job.assetPublicId == widgetId && + job.referenceDurable && job.state != CloudMediaJobState.failed, ); return stillUploading diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index 0ab7b943..d51db593 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11590,7 +11590,13 @@ void main() { expect(copies(container), isEmpty); }); - test('this device is still uploading is not copied yet', () async { + /// Opens the image, unknown to the server, with this device's upload + /// of it in [state]; [saved] false: the image's own save failed, so + /// its upload never goes. + Future openUploading( + CloudMediaJobState state, { + bool saved = true, + }) async { final (container, _) = await openWithImage(CloudImageCopyResult.unavailable); container.read(cloudMediaUploadQueueProvider.notifier).state = @@ -11603,16 +11609,37 @@ void main() { assetPublicId: 'image', fileExtension: '.png', mimeType: 'image/png', - state: CloudMediaJobState.pendingUpload, + state: state, attempts: 0, updatedAt: DateTime.utc(2026), + referenceDurable: saved, ), ], isProcessing: false, ); + return container; + } - expect(await copy(container), PageCopyResult.imageUploading); - expect(copies(container), isEmpty); + test('this device is still uploading is not copied yet', () async { + for (final state in [ + CloudMediaJobState.pendingUpload, + CloudMediaJobState.pendingAttach, + ]) { + final container = await openUploading(state); + expect(await copy(container), PageCopyResult.imageUploading); + expect(copies(container), isEmpty); + } + }); + + test('whose upload failed or will never go is not called uploading', + () async { + for (final container in [ + await openUploading(CloudMediaJobState.failed), + await openUploading(CloudMediaJobState.pendingUpload, saved: false), + ]) { + expect(await copy(container), PageCopyResult.imageUnavailable); + expect(copies(container), isEmpty); + } }); test('the cloud cannot show is not copied', () async { From 26658e615e58335ce8db880a15e7d47ad8fc1402 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:00:10 -0400 Subject: [PATCH 08/11] Find an image's picture by its picture id, not its item id A placed image may name the picture it shows (assetId, Hive field 11); without one, its own id names the picture, as before, so every image, .ica file and backup made so far reads exactly as it did. pictureId is the id the picture is stored, uploaded and found under: rendering, the page transition, screenshots, video export, the upload queue, cloud export, local-to-cloud migration and the cleanup of unused picture files all use it. Item identity (undo, transitions, sync rows, hero tags) stays the image's own id. Nothing makes such images yet; copies will (see the server half). Co-Authored-By: Claude Opus 5.5 --- lib/collab/cloud_media_models.dart | 2 +- lib/const/placed_classes.dart | 13 +++ lib/const/placed_classes.g.dart | 2 + lib/hive/hive_adapters.g.dart | 7 +- lib/hive/hive_adapters.g.yaml | 4 +- .../cloud_media_upload_queue_provider.dart | 25 +++-- lib/providers/image_provider.dart | 9 +- lib/providers/strategy_provider.dart | 2 +- lib/screenshot/page_screenshot.dart | 5 +- .../video_export/video_export_source.dart | 7 +- lib/strategy/strategy_cloud_migration.dart | 6 +- lib/strategy/strategy_import_export.dart | 6 +- lib/strategy/strategy_page_source.dart | 4 +- .../draggable_widgets/image/image_widget.dart | 12 ++- .../image/placed_image_builder.dart | 2 + lib/widgets/page_transition_overlay.dart | 1 + test/canonical_coordinates_test.dart | 1 + test/color_persistence_test.dart | 43 ++++++++ test/strategy_image_source_test.dart | 1 + test/strategy_integrity_test.dart | 97 +++++++++++++++++++ test/web_media_bytes_upload_test.dart | 43 ++++++++ 21 files changed, 265 insertions(+), 27 deletions(-) diff --git a/lib/collab/cloud_media_models.dart b/lib/collab/cloud_media_models.dart index 0e1713ab..6bb26a37 100644 --- a/lib/collab/cloud_media_models.dart +++ b/lib/collab/cloud_media_models.dart @@ -170,7 +170,7 @@ Set collectStrategyImageAssetIds(StrategyDataLike strategy) { final assetIds = {}; for (final page in strategy.pages) { for (final image in page.imageData) { - assetIds.add(image.id); + assetIds.add(image.pictureId); } for (final link in page.lineUpLinks) { for (final image in link.images) { diff --git a/lib/const/placed_classes.dart b/lib/const/placed_classes.dart index ee5b22d3..8b3db22b 100644 --- a/lib/const/placed_classes.dart +++ b/lib/const/placed_classes.dart @@ -218,10 +218,21 @@ class PlacedImage extends PlacedWidget { this.sizeVersion, this.tagColorValue, this.link = '', + this.assetId, }); final double aspectRatio; + /// The picture this image shows, when it isn't the image's own id: a + /// copy of an image is a new item showing its original's picture, with + /// nothing copied or uploaded. Absent on every image made before copies + /// shared pictures. Use [pictureId] to find the picture. + @JsonKey(includeIfNull: false) + final String? assetId; + + /// The id the image's picture is stored, uploaded and found under. + String get pictureId => assetId ?? id; + final String? fileExtension; double scale; @@ -264,6 +275,7 @@ class PlacedImage extends PlacedWidget { int? tagColorValue, bool? isDeleted, String? link, + String? assetId, }) { final cloned = PlacedImage( position: position ?? this.position, @@ -273,6 +285,7 @@ class PlacedImage extends PlacedWidget { fileExtension: fileExtension ?? this.fileExtension, sizeVersion: sizeVersion ?? this.sizeVersion, tagColorValue: tagColorValue ?? this.tagColorValue, + assetId: assetId ?? this.assetId, ); // Base class field // cloned.isDeleted = isDeleted ?? this.isDeleted; diff --git a/lib/const/placed_classes.g.dart b/lib/const/placed_classes.g.dart index 0c1e8765..e2aebc8d 100644 --- a/lib/const/placed_classes.g.dart +++ b/lib/const/placed_classes.g.dart @@ -54,6 +54,7 @@ PlacedImage _$PlacedImageFromJson(Map json) => PlacedImage( sizeVersion: (json['sizeVersion'] as num?)?.toInt(), tagColorValue: (json['tagColorValue'] as num?)?.toInt(), link: json['link'] as String? ?? '', + assetId: json['assetId'] as String?, )..isDeleted = json['isDeleted'] as bool? ?? false; Map _$PlacedImageToJson(PlacedImage instance) => @@ -62,6 +63,7 @@ Map _$PlacedImageToJson(PlacedImage instance) => 'isDeleted': instance.isDeleted, 'position': const OffsetConverter().toJson(instance.position), 'aspectRatio': instance.aspectRatio, + if (instance.assetId case final value?) 'assetId': value, 'fileExtension': instance.fileExtension, 'scale': instance.scale, 'sizeVersion': instance.sizeVersion, diff --git a/lib/hive/hive_adapters.g.dart b/lib/hive/hive_adapters.g.dart index e9baf321..2ffb967c 100644 --- a/lib/hive/hive_adapters.g.dart +++ b/lib/hive/hive_adapters.g.dart @@ -218,13 +218,14 @@ class PlacedImageAdapter extends TypeAdapter { sizeVersion: (fields[10] as num?)?.toInt(), tagColorValue: (fields[9] as num?)?.toInt(), link: fields[3] == null ? '' : fields[3] as String, + assetId: fields[11] as String?, )..isDeleted = fields[5] as bool; } @override void write(BinaryWriter writer, PlacedImage obj) { writer - ..writeByte(9) + ..writeByte(10) ..writeByte(1) ..write(obj.aspectRatio) ..writeByte(2) @@ -242,7 +243,9 @@ class PlacedImageAdapter extends TypeAdapter { ..writeByte(9) ..write(obj.tagColorValue) ..writeByte(10) - ..write(obj.sizeVersion); + ..write(obj.sizeVersion) + ..writeByte(11) + ..write(obj.assetId); } @override diff --git a/lib/hive/hive_adapters.g.yaml b/lib/hive/hive_adapters.g.yaml index 7fa3e40c..6a40ad03 100644 --- a/lib/hive/hive_adapters.g.yaml +++ b/lib/hive/hive_adapters.g.yaml @@ -75,7 +75,7 @@ types: index: 7 PlacedImage: typeId: 5 - nextIndex: 11 + nextIndex: 12 fields: aspectRatio: index: 1 @@ -95,6 +95,8 @@ types: index: 9 sizeVersion: index: 10 + assetId: + index: 11 MapValue: typeId: 6 nextIndex: 13 diff --git a/lib/providers/collab/cloud_media_upload_queue_provider.dart b/lib/providers/collab/cloud_media_upload_queue_provider.dart index 0a4f6e0a..98c1aeed 100644 --- a/lib/providers/collab/cloud_media_upload_queue_provider.dart +++ b/lib/providers/collab/cloud_media_upload_queue_provider.dart @@ -449,21 +449,24 @@ class CloudMediaUploadQueueNotifier assetPublicId: assetPublicId, ); + // By picture: a copy of an image shows its original's picture, which + // is uploaded once, for the original. for (final image in placedImages) { - final asset = assetsById[image.id]; + final pictureId = image.pictureId; + final asset = assetsById[pictureId]; final hasActiveRemote = asset?.uploadStatus == 'active' && (asset?.url?.isNotEmpty ?? false); - if (hasActiveRemote || _getJob(image.id) != null) { + if (hasActiveRemote || _getJob(pictureId) != null) { continue; } final bytes = await _findMediaBytes( - keyFor(image.id), + keyFor(pictureId), fileExtension: image.fileExtension ?? '', ); if (bytes == null) { _logMedia( - 'reconcile.local_missing image=${image.id} ' + 'reconcile.local_missing image=$pictureId ' 'strategy=$strategyPublicId status=${asset?.uploadStatus ?? 'none'}', ); continue; @@ -471,7 +474,7 @@ class CloudMediaUploadQueueNotifier await enqueueJobForLocalBytes( strategyPublicId: strategyPublicId, - assetPublicId: image.id, + assetPublicId: pictureId, fileExtension: image.fileExtension ?? '', ); } @@ -1192,10 +1195,10 @@ class CloudMediaUploadQueueNotifier bool _opReferencesAsset(StrategyOp op, String assetPublicId) { if (op is ElementAddOp) { - return op.elementPublicId == assetPublicId; + return _pictureOf(op.elementPublicId, op.payload) == assetPublicId; } if (op is ElementPatchOp) { - return op.elementPublicId == assetPublicId; + return _pictureOf(op.elementPublicId, op.payload) == assetPublicId; } if (op is LineupAddOp) { return _jsonContainsAssetId(op.payload, assetPublicId); @@ -1206,6 +1209,14 @@ class CloudMediaUploadQueueNotifier return false; } + /// The picture an element op shows when it is an image: its payload's + /// `assetId`, else the element's own id (see PlacedImage.pictureId). + static String _pictureOf(String elementPublicId, CloudPayload? payload) { + final assetId = + payload == null ? null : cloudPayloadData(payload)['assetId']; + return assetId is String && assetId.isNotEmpty ? assetId : elementPublicId; + } + bool _jsonContainsAssetId(Object? value, String assetPublicId) { if (value is Map) { if (value['id'] == assetPublicId) return true; diff --git a/lib/providers/image_provider.dart b/lib/providers/image_provider.dart index 1048e924..5017bcf4 100644 --- a/lib/providers/image_provider.dart +++ b/lib/providers/image_provider.dart @@ -679,7 +679,7 @@ class PlacedImageSerializer { /// /// It uses the application support directory, creates a custom folder based /// on [strategyID] and an `images` subfolder, and forms the filename from the - /// image's [id] and [fileExtension]. + /// image's picture id ([PlacedImage.pictureId]) and [fileExtension]. static Future _computeFilePath( PlacedImage image, String strategyID) async { // Get the system's application support directory. @@ -699,8 +699,11 @@ class PlacedImageSerializer { await imagesDirectory.create(recursive: true); } - // The final file path: [id][fileExtension] - return path.join(imagesDirectory.path, '${image.id}${image.fileExtension}'); + // The final file path: [pictureId][fileExtension] + return path.join( + imagesDirectory.path, + '${image.pictureId}${image.fileExtension}', + ); } static String? detectImageFormat(Uint8List bytes) { diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 0fa31be6..f634ba8c 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1127,7 +1127,7 @@ class StrategyProvider extends Notifier { if (!kIsWeb) { List allImageIds = []; for (final page in newStrat.pages) { - allImageIds.addAll(page.imageData.map((image) => image.id)); + allImageIds.addAll(page.imageData.map((image) => image.pictureId)); for (final link in page.lineUpLinks) { allImageIds.addAll(link.images.map((image) => image.id)); } diff --git a/lib/screenshot/page_screenshot.dart b/lib/screenshot/page_screenshot.dart index 620182f5..0242d2ba 100644 --- a/lib/screenshot/page_screenshot.dart +++ b/lib/screenshot/page_screenshot.dart @@ -47,10 +47,11 @@ Future captureEditorPage(WidgetRef ref) async { final images = await resolveCaptureImages( { + // By picture, as the captured page's images look them up. for (final image in page.imageData) - image.id: readStrategyImageSource( + image.pictureId: readStrategyImageSource( ref, - (id: image.id, fileExtension: image.fileExtension), + (id: image.pictureId, fileExtension: image.fileExtension), ), }, fetch: (imageId, url, client) => downloadCloudImageBytes( diff --git a/lib/services/video_export/video_export_source.dart b/lib/services/video_export/video_export_source.dart index 5b073d0e..203af513 100644 --- a/lib/services/video_export/video_export_source.dart +++ b/lib/services/video_export/video_export_source.dart @@ -149,18 +149,19 @@ Future loadVideoExportSource( final images = await resolveCaptureImages( { for (final page in pages) + // By picture, as the captured pages' images look them up. for (final image in page.imageData) - image.id: resolveStrategyImageSource( + image.pictureId: resolveStrategyImageSource( localFilePath: findLocalImageFile( storageDirectory: state.storageDirectory, - imageId: image.id, + imageId: image.pictureId, fileExtension: image.fileExtension, ), isCloudStrategy: isCloud, // The whole strategy was just read, and this device has // nothing left to upload. assetsLoaded: true, - remoteAsset: assets[image.id], + remoteAsset: assets[image.pictureId], uploadMayBeQueuedHere: false, ), }, diff --git a/lib/strategy/strategy_cloud_migration.dart b/lib/strategy/strategy_cloud_migration.dart index e225702e..63167b1f 100644 --- a/lib/strategy/strategy_cloud_migration.dart +++ b/lib/strategy/strategy_cloud_migration.dart @@ -57,7 +57,11 @@ void appendMigratedPageOps( for (final image in page.imageData) { final elementId = nextUniqueMigrationId(image.id, usedElementIds); - final payload = cloudImagePayloadFromPlacedImage(image) + // A renamed image keeps showing its picture, which is stored under its + // old id. + final payload = cloudImagePayloadFromPlacedImage( + elementId == image.id ? image : image.copyWith(assetId: image.pictureId), + ) ..putIfAbsent('elementType', () => 'image') ..['id'] = elementId; ops.add( diff --git a/lib/strategy/strategy_import_export.dart b/lib/strategy/strategy_import_export.dart index f483ac64..b66a9acc 100644 --- a/lib/strategy/strategy_import_export.dart +++ b/lib/strategy/strategy_import_export.dart @@ -2408,7 +2408,11 @@ class StrategyImportExportService { if (element.deleted || element.elementType != 'image') { continue; } - assetIds.add(element.publicId); + // The picture it shows (PlacedImage.pictureId). + final assetId = cloudPayloadData(element.payload)['assetId']; + assetIds.add( + assetId is String && assetId.isNotEmpty ? assetId : element.publicId, + ); } final lineups = lineUpGraphFromRemoteLineups( diff --git a/lib/strategy/strategy_page_source.dart b/lib/strategy/strategy_page_source.dart index 1d0457e8..3db8653f 100644 --- a/lib/strategy/strategy_page_source.dart +++ b/lib/strategy/strategy_page_source.dart @@ -261,7 +261,7 @@ class CloudStrategyPageSource implements StrategyPageSource { break; case 'image': final hydrated = PlacedImage.fromJson(payload); - final remoteAsset = snapshot.assetsById[hydrated.id]; + final remoteAsset = snapshot.assetsById[hydrated.pictureId]; images.add(hydrated); if (remoteAsset != null) { ref.read(cloudMediaCacheProvider.notifier).ensureAssetCached( @@ -440,7 +440,7 @@ class CloudStrategyPageSource implements StrategyPageSource { break; case 'image': final hydrated = PlacedImage.fromJson(payload); - final remoteAsset = snapshot.assetsById[hydrated.id]; + final remoteAsset = snapshot.assetsById[hydrated.pictureId]; images.add(hydrated); if (remoteAsset != null) { ref.read(cloudMediaCacheProvider.notifier).ensureAssetCached( diff --git a/lib/widgets/draggable_widgets/image/image_widget.dart b/lib/widgets/draggable_widgets/image/image_widget.dart index f32a87ce..329cc58a 100644 --- a/lib/widgets/draggable_widgets/image/image_widget.dart +++ b/lib/widgets/draggable_widgets/image/image_widget.dart @@ -45,7 +45,6 @@ class _ImageFullScreenOverlay extends StatelessWidget { @override Widget build(BuildContext context) { - return CallbackShortcuts( bindings: { const SingleActivator(LogicalKeyboardKey.escape): () { @@ -129,13 +128,20 @@ class ImageWidget extends ConsumerStatefulWidget { required this.scale, required this.fileExtension, required this.id, + required this.pictureId, this.tagColorValue, this.isFeedback = false, }); final double aspectRatio; final double scale; final String? fileExtension; + + /// The placed image's id, which its hero tag carries: unique on a page. final String id; + + /// The id of the picture it shows (PlacedImage.pictureId), which images + /// on several pages, or a copy and its original, can share. + final String pictureId; final int? tagColorValue; final bool isFeedback; @@ -161,7 +167,7 @@ class _ImageWidgetState extends ConsumerState { .clamp(1.0, double.infinity); final source = watchStrategyImageSource( ref, - (id: widget.id, fileExtension: widget.fileExtension), + (id: widget.pictureId, fileExtension: widget.fileExtension), ); final image = source.imageProvider; @@ -170,7 +176,7 @@ class _ImageWidgetState extends ConsumerState { // while its cloud URL loads, and no other image's frame carries // over. LocalImageFile() || RemoteImageUrl() || ImageBytes() => Image( - key: ValueKey(widget.id), + key: ValueKey(widget.pictureId), image: image!, fit: BoxFit.contain, gaplessPlayback: true, diff --git a/lib/widgets/draggable_widgets/image/placed_image_builder.dart b/lib/widgets/draggable_widgets/image/placed_image_builder.dart index eba8db3f..ba7e58e2 100644 --- a/lib/widgets/draggable_widgets/image/placed_image_builder.dart +++ b/lib/widgets/draggable_widgets/image/placed_image_builder.dart @@ -189,6 +189,7 @@ class _PlacedImageBuilderState extends State { scale: localScale!, fileExtension: widget.placedImage.fileExtension, id: widget.placedImage.id, + pictureId: widget.placedImage.pictureId, tagColorValue: widget.placedImage.tagColorValue, ), ), @@ -220,6 +221,7 @@ class _PlacedImageBuilderState extends State { aspectRatio: widget.placedImage.aspectRatio, scale: localScale!, id: widget.placedImage.id, + pictureId: widget.placedImage.pictureId, tagColorValue: widget.placedImage.tagColorValue, ), ), diff --git a/lib/widgets/page_transition_overlay.dart b/lib/widgets/page_transition_overlay.dart index e1730767..c3fa48ff 100644 --- a/lib/widgets/page_transition_overlay.dart +++ b/lib/widgets/page_transition_overlay.dart @@ -774,6 +774,7 @@ class PlacedWidgetPreview { aspectRatio: w.aspectRatio, scale: scale ?? w.scale, id: w.id, + pictureId: w.pictureId, tagColorValue: w.tagColorValue, ); } diff --git a/test/canonical_coordinates_test.dart b/test/canonical_coordinates_test.dart index d49c2fe9..aaec9148 100644 --- a/test/canonical_coordinates_test.dart +++ b/test/canonical_coordinates_test.dart @@ -388,6 +388,7 @@ void main() { ImageWidget( key: const ValueKey('image-card'), id: image.id, + pictureId: image.pictureId, aspectRatio: image.aspectRatio, scale: image.scale, fileExtension: image.fileExtension, diff --git a/test/color_persistence_test.dart b/test/color_persistence_test.dart index e4329b4f..8929e98c 100644 --- a/test/color_persistence_test.dart +++ b/test/color_persistence_test.dart @@ -366,5 +366,48 @@ void main() { expect(restored.link, isEmpty); }); + + test('a placed image stored before picture ids shows its own picture', () { + final restored = PlacedImageAdapter().read( + _legacyFieldReader({ + 1: 1.5, + 2: 200.0, + 3: '', + 4: 'legacy-image', + 5: false, + 6: const Offset(3, 4), + 8: '.png', + 9: 0xFF3B82F6, + 10: worldSizedMediaVersion, + }), + ); + + expect(restored.assetId, isNull); + expect(restored.pictureId, 'legacy-image'); + // Its JSON, and so its cloud payload, is as it was. + expect(restored.toJson().containsKey('assetId'), isFalse); + }); + + test("a placed image showing another image's picture keeps it", () { + final restored = PlacedImageAdapter().read( + _legacyFieldReader({ + 1: 1.5, + 2: 200.0, + 3: '', + 4: 'copy-image', + 5: false, + 6: const Offset(3, 4), + 8: '.png', + 10: worldSizedMediaVersion, + 11: 'original-image', + }), + ); + + expect(restored.pictureId, 'original-image'); + final reloaded = PlacedImage.fromJson(restored.toJson()); + expect(reloaded.id, 'copy-image'); + expect(reloaded.pictureId, 'original-image'); + expect(restored.copyWith(scale: 300).pictureId, 'original-image'); + }); }); } diff --git a/test/strategy_image_source_test.dart b/test/strategy_image_source_test.dart index 55373476..8d0dfc25 100644 --- a/test/strategy_image_source_test.dart +++ b/test/strategy_image_source_test.dart @@ -202,6 +202,7 @@ Widget _imageApp({ // ignore: prefer_const_constructors ImageWidget( id: _imageId, + pictureId: _imageId, aspectRatio: 16 / 9, scale: 320, fileExtension: '.png', diff --git a/test/strategy_integrity_test.dart b/test/strategy_integrity_test.dart index 7fbe6b7d..2b836236 100644 --- a/test/strategy_integrity_test.dart +++ b/test/strategy_integrity_test.dart @@ -16,6 +16,7 @@ import 'package:icarus/const/hive_boxes.dart'; import 'package:icarus/const/drawing_element.dart'; import 'package:icarus/const/line_provider.dart'; import 'package:icarus/const/maps.dart'; +import 'package:icarus/const/coordinate_system.dart'; import 'package:icarus/const/placed_classes.dart'; import 'package:icarus/const/settings.dart'; import 'package:icarus/const/utilities.dart'; @@ -772,6 +773,102 @@ void main() { expect(reExported, equals(exported)); }); + test( + 'an image showing another image\'s picture round-trips, and opening ' + 'keeps the picture while only the copy shows it', () async { + final harness = await _IcaHarness.open(); + addTearDown(harness.close); + Map image(String id, {String? assetId}) => { + 'id': id, + 'isDeleted': false, + 'position': {'dx': 500.0, 'dy': 600.0}, + 'aspectRatio': 1.0, + 'fileExtension': '.png', + 'scale': 220.0, + 'tagColorValue': null, + if (assetId != null) 'assetId': assetId, + }; + final payload = { + 'versionNumber': '${Settings.versionNumber}', + 'mapData': 'ascent', + 'pages': [ + { + 'id': 'page-1', + 'sortIndex': '0', + 'name': 'Page 1', + 'isAutoNamed': true, + 'drawingData': [], + 'agentData': [], + 'abilityData': [], + 'textData': [], + 'imageData': [image('img-1'), image('img-copy', assetId: 'img-1')], + 'utilityData': [], + 'isAttack': 'true', + 'settings': {'agentSize': 35.0, 'abilitySize': 25.0}, + 'lineUpData': [], + }, + ], + }; + final picture = [137, 80, 78, 71, 13, 10, 26, 10]; + final json = utf8.encode(jsonEncode(payload)); + final archive = Archive() + ..addFile(ArchiveFile('Pictures.json', json.length, json)) + ..addFile(ArchiveFile('img-1.png', picture.length, picture)); + final file = File(path.join(harness.directory.path, 'Pictures.ica')); + await file.writeAsBytes(ZipEncoder().encodeBytes(archive)); + + final imported = await harness.importIca(file); + final images = { + for (final image in imported.pages.single.imageData) image.id: image, + }; + expect(images['img-1']!.pictureId, 'img-1'); + expect(images['img-copy']!.assetId, 'img-1'); + expect(images['img-copy']!.pictureId, 'img-1'); + + // Exported: the copy names the picture, which is packed once. + final exported = await harness.exportIca(imported); + final exportedImages = ((await _readIcaJson(exported))['pages'] as List) + .cast>() + .single['imageData'] as List; + expect( + {for (final image in exportedImages) image['id']: image['assetId']}, + {'img-1': null, 'img-copy': 'img-1'}, + ); + expect((await _icaAttachments(exported)).keys, ['img-1.png']); + final reImported = await harness.importIca(exported); + expect( + reImported.pages.single.imageData + .singleWhere((image) => image.id == 'img-copy') + .pictureId, + 'img-1', + ); + + // The original is deleted; opening the strategy keeps the picture the + // copy still shows. + final onlyCopy = reImported.copyWith(pages: [ + reImported.pages.single.copyWith( + imageData: [ + reImported.pages.single.imageData + .singleWhere((image) => image.id == 'img-copy'), + ], + ), + ]); + await harness.strategies.put(onlyCopy.id, onlyCopy); + CoordinateSystem(playAreaSize: const Size(1920, 1080)); + await harness.container + .read(strategyProvider.notifier) + .loadFromHive(onlyCopy.id); + expect( + File(path.join( + harness.directory.path, + onlyCopy.id, + 'images', + 'img-1.png', + )).existsSync(), + isTrue, + ); + }); + test('custom shape utility dimensions support undo and redo', () { final rectangle = PlacedUtility( id: 'rectangle-undo', diff --git a/test/web_media_bytes_upload_test.dart b/test/web_media_bytes_upload_test.dart index 978ef07e..a47314a3 100644 --- a/test/web_media_bytes_upload_test.dart +++ b/test/web_media_bytes_upload_test.dart @@ -549,6 +549,49 @@ void main() { ); }); + test("a copy showing another image's picture uploads it once, under it", + () async { + final bytesStore = MemoryPendingMediaBytesStore(); + final mediaStore = MemoryDurableCloudMediaOutboxStore(); + final container = + _webSession(mediaStore: mediaStore, bytesStore: bytesStore); + addTearDown(container.dispose); + final queue = container.read(cloudMediaUploadQueueProvider.notifier); + PlacedImage image(String id, {String? assetId}) => PlacedImage( + id: id, + position: Offset.zero, + aspectRatio: 1, + scale: 1, + fileExtension: '.png', + assetId: assetId, + ); + await container + .read(pendingMediaBytesProvider.notifier) + .put(_key('original'), _imageBytes); + + // Only the copy is on the page: its picture is the original's. + await queue.reconcilePageMedia( + strategyPublicId: 'strategy-a', + placedImages: [image('copy', assetId: 'original')], + assetsById: const {}, + ); + expect( + container + .read(cloudMediaUploadQueueProvider) + .jobs + .map((job) => job.assetPublicId), + ['original'], + ); + + // With the original beside it, still the one upload. + await queue.reconcilePageMedia( + strategyPublicId: 'strategy-a', + placedImages: [image('original'), image('copy', assetId: 'original')], + assetsById: const {}, + ); + expect(container.read(cloudMediaUploadQueueProvider).jobs, hasLength(1)); + }); + group('the painted copy goes once attached and served from the cloud', () { Future race({required bool urlFirst}) async { final gate = Completer(); From cd490fb26eb0ec355ff1b612c0d03bbc61e141f7 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:00:29 -0400 Subject: [PATCH 09/11] Take back the server-side picture copy Copies will show their original's picture instead (see the picture id change), so the server never needs to copy a picture within a strategy. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 53 --- convex/imageCopy.test.ts | 384 -------------------- convex/images.ts | 83 ----- lib/collab/generated/convex_models.dart | 40 -- lib/collab/generated/icarus_convex_api.dart | 25 -- 5 files changed, 585 deletions(-) delete mode 100644 convex/imageCopy.test.ts diff --git a/convex/function_spec.json b/convex/function_spec.json index b56f025c..93232fbf 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -40101,59 +40101,6 @@ "kind": "public" } }, - { - "args": { - "type": "object", - "value": { - "clientProtocolVersion": { - "fieldType": { - "type": "number" - }, - "optional": false - }, - "sourceAssetPublicId": { - "fieldType": { - "type": "string" - }, - "optional": false - }, - "strategyPublicId": { - "fieldType": { - "type": "string" - }, - "optional": false - }, - "targetAssetPublicId": { - "fieldType": { - "type": "string" - }, - "optional": false - } - } - }, - "functionType": "Mutation", - "identifier": "images.js:copyAsset", - "returns": { - "type": "union", - "value": [ - { - "type": "literal", - "value": "copied" - }, - { - "type": "literal", - "value": "uploading" - }, - { - "type": "literal", - "value": "unavailable" - } - ] - }, - "visibility": { - "kind": "public" - } - }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts deleted file mode 100644 index 18bbbd8a..00000000 --- a/convex/imageCopy.test.ts +++ /dev/null @@ -1,384 +0,0 @@ -import { - convexTest, - type TestConvexForDataModel, - type TestConvexForDataModelAndIdentity, -} from "convex-test"; -import { makeFunctionReference } from "convex/server"; -import { afterEach, beforeAll, describe, expect, test, vi } from "vitest"; -import type { DataModel } from "./_generated/dataModel"; -import { markAssetReferencesReady } from "./lib/assetReferences"; -import { CURRENT_CLOUD_PROTOCOL_VERSION } from "./lib/cloudProtocol"; -import schema from "./schema"; -import { modules } from "./test.setup"; - -const ensureCurrentUser = makeFunctionReference<"mutation">( - "users:ensureCurrentUser", -); -const createStrategy = makeFunctionReference<"mutation">( - "strategies:createWithInitialPage", -); -const deleteStrategy = makeFunctionReference<"mutation">("strategies:delete"); -const addPage = makeFunctionReference<"mutation">("pages:add"); -const applyBatch = makeFunctionReference<"mutation">("ops:applyBatch"); -const purgeOldTombstones = makeFunctionReference<"mutation">( - "maintenance:purgeOldTombstones", -); -const listImages = makeFunctionReference<"query">("images:listForStrategy"); -const copyAsset = makeFunctionReference<"mutation">("images:copyAsset"); -const createShare = makeFunctionReference<"mutation">("shares:create"); -const redeemShare = makeFunctionReference<"mutation">("shares:redeem"); - -type Harness = TestConvexForDataModel; -type RootHarness = TestConvexForDataModelAndIdentity; - -const protocol = { clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION }; -const strategyPublicId = "copy-strategy"; -const firstPage = "copy-page-1"; -const secondPage = "copy-page-2"; -const original = "placed-image"; -const copy = `placed-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`; -const settings = { agentSize: 40, abilitySize: 30, useNeutralTeamColors: true }; - -function identity(subject: string) { - return { - issuer: "https://image-copy.test", - subject, - tokenIdentifier: `image-copy|${subject}`, - name: `User ${subject}`, - }; -} - -/// A two-page strategy whose first page shows a placed image, with the -/// image's upload [status] ("active" once its bytes landed). -async function createHarness(status: "active" | "pending" = "active"): Promise<{ - t: RootHarness; - owner: Harness; - other: Harness; -}> { - const t = convexTest(schema, modules); - await t.run(markAssetReferencesReady); - const owner = t.withIdentity(identity("owner")); - const other = t.withIdentity(identity("other")); - await owner.mutation(ensureCurrentUser, protocol); - await other.mutation(ensureCurrentUser, protocol); - await owner.mutation(createStrategy, { - ...protocol, - publicId: strategyPublicId, - name: "Bind B split", - mapData: "bind", - initialPagePublicId: firstPage, - initialPageName: "Setup", - initialPageIsAutoNamed: false, - initialPageIsAttack: true, - initialPageSettings: settings, - }); - await owner.mutation(addPage, { - ...protocol, - strategyPublicId, - pagePublicId: secondPage, - name: "Hit", - sortIndex: 1, - isAttack: true, - expectedRevision: 0, - }); - await t.run(async (ctx) => { - const strategy = await ctx.db - .query("strategies") - .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) - .unique(); - const now = Date.now(); - await ctx.db.insert("imageAssets", { - publicId: original, - provider: "r2", - strategyId: strategy!._id, - uploadAttemptPublicId: "placed-image-attempt", - objectKey: `strategies/${strategyPublicId}/${original}.png`, - uploadStatus: status, - fileExtension: ".png", - mimeType: "image/png", - width: 64, - height: 32, - byteSize: 100, - ...(status === "active" ? { uploadedAt: now } : {}), - createdAt: now, - updatedAt: now, - }); - }); - await addImage(owner, original, firstPage); - return { t, owner, other }; -} - -async function addImage(user: Harness, id: string, pagePublicId: string) { - await user.mutation(applyBatch, { - ...protocol, - strategyPublicId, - clientId: `add-${id}`, - ops: [ - { - opId: `add-${id}`, - type: "element.add", - elementPublicId: id, - pagePublicId, - payload: { - kind: "image", - payloadVersion: 1, - data: { id, elementType: "image", scale: 2 }, - }, - sortIndex: 1, - }, - ], - }); -} - -async function copyImage(user: Harness) { - return await user.mutation(copyAsset, { - ...protocol, - strategyPublicId, - sourceAssetPublicId: original, - targetAssetPublicId: copy, - }); -} - -type ImageRow = { publicId: string; uploadStatus: string; url: string | null }; - -async function images(user: Harness) { - const rows = (await user.query(listImages, { strategyPublicId })) as ImageRow[]; - return Object.fromEntries( - rows.map((row) => [row.publicId, [row.uploadStatus, row.url]]), - ); -} - -async function rowsFor(t: RootHarness, publicId: string) { - return await t.run(async (ctx) => - (await ctx.db.query("imageAssets").collect()).filter( - (row) => row.publicId === publicId, - ), - ); -} - -const url = `https://media.copy.test/strategies/${strategyPublicId}/${original}.png`; - -beforeAll(() => { - process.env.R2_ACCOUNT_ID = "copy-account"; - process.env.R2_BUCKET = "copy-bucket"; - process.env.R2_ACCESS_KEY_ID = "copy-access-key"; - process.env.R2_SECRET_ACCESS_KEY = "copy-secret"; - process.env.R2_PUBLIC_BASE_URL = "https://media.copy.test"; - process.env.R2_S3_ENDPOINT = "https://copy.r2.test"; -}); - -afterEach(() => { - vi.useRealTimers(); - vi.unstubAllGlobals(); -}); - -describe("images:copyAsset", () => { - test("a copied image shows the original's picture under its own id", async () => { - const { owner } = await createHarness(); - - expect(await copyImage(owner)).toBe("copied"); - await addImage(owner, copy, secondPage); - - expect(await images(owner)).toEqual({ - [original]: ["active", url], - [copy]: ["active", url], - }); - }); - - test("a copy whose content landed first takes over its placeholder", async () => { - const { t, owner } = await createHarness(); - await addImage(owner, copy, secondPage); - expect((await images(owner))[copy]).toEqual(["pending", null]); - - expect(await copyImage(owner)).toBe("copied"); - - expect((await images(owner))[copy]).toEqual(["active", url]); - expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ - "active", - ]); - }); - - test("copying again changes nothing", async () => { - const { t, owner } = await createHarness(); - await copyImage(owner); - - expect(await copyImage(owner)).toBe("copied"); - expect(await rowsFor(t, copy)).toHaveLength(1); - }); - - test("an id that already shows another image is refused", async () => { - const { t, owner } = await createHarness(); - await t.run(async (ctx) => { - const strategy = await ctx.db - .query("strategies") - .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) - .unique(); - const now = Date.now(); - await ctx.db.insert("imageAssets", { - publicId: copy, - provider: "r2", - strategyId: strategy!._id, - uploadAttemptPublicId: "other-attempt", - objectKey: `strategies/${strategyPublicId}/other.png`, - uploadStatus: "active", - fileExtension: ".png", - mimeType: "image/png", - uploadedAt: now, - createdAt: now, - updatedAt: now, - }); - }); - - await expect(copyImage(owner)).rejects.toThrow(); - expect((await rowsFor(t, copy)).map((row) => row.objectKey)).toEqual([ - `strategies/${strategyPublicId}/other.png`, - ]); - }); - - test("a copy replaces a failed upload under its id", async () => { - const { t, owner } = await createHarness(); - await t.run(async (ctx) => { - const strategy = await ctx.db - .query("strategies") - .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) - .unique(); - const now = Date.now(); - await ctx.db.insert("imageAssets", { - publicId: copy, - provider: "r2", - strategyId: strategy!._id, - uploadAttemptPublicId: "failed-attempt", - objectKey: `strategies/${strategyPublicId}/failed.png`, - uploadStatus: "failed", - fileExtension: ".png", - mimeType: "image/png", - createdAt: now, - updatedAt: now, - }); - }); - await addImage(owner, copy, secondPage); - - expect(await copyImage(owner)).toBe("copied"); - expect((await images(owner))[copy]).toEqual(["active", url]); - }); - - test("a copy whose content arrives days later still has its picture", async () => { - vi.useFakeTimers(); - const fetchMock = vi.fn(async () => new Response(null, { status: 204 })); - vi.stubGlobal("fetch", fetchMock); - const { t, owner } = await createHarness(); - await copyImage(owner); - - // The copy waited in a device's outbox, offline, for three days. - vi.setSystemTime(Date.now() + 3 * 24 * 60 * 60 * 1000); - await t.finishAllScheduledFunctions(vi.runAllTimers); - await addImage(owner, copy, secondPage); - - expect((await images(owner))[copy]).toEqual(["active", url]); - expect(fetchMock).not.toHaveBeenCalled(); - }); - - test("an image still uploading has nothing to copy yet", async () => { - const { t, owner } = await createHarness("pending"); - await addImage(owner, copy, secondPage); - - expect(await copyImage(owner)).toBe("uploading"); - // The copy's placeholder waits, as for any image on its way. - expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ - "pending", - ]); - }); - - test("an image the strategy cannot show is unavailable", async () => { - const { owner } = await createHarness(); - - expect( - await owner.mutation(copyAsset, { - ...protocol, - strategyPublicId, - sourceAssetPublicId: "missing-image", - targetAssetPublicId: `missing-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`, - }), - ).toBe("unavailable"); - }); - - test("only an editor of the strategy can copy, and never onto itself", async () => { - const { owner, other } = await createHarness(); - - await expect(copyImage(other)).rejects.toThrow(); - // A collaborator who may only view cannot copy; an editor can. - for (const role of ["viewer", "editor"] as const) { - await owner.mutation(createShare, { - ...protocol, - targetType: "strategy", - targetPublicId: strategyPublicId, - token: `as-${role}`, - role, - }); - } - await other.mutation(redeemShare, { ...protocol, token: "as-viewer" }); - await expect(copyImage(other)).rejects.toThrow(); - await other.mutation(redeemShare, { ...protocol, token: "as-editor" }); - expect(await copyImage(other)).toBe("copied"); - - await expect( - owner.mutation(copyAsset, { - ...protocol, - strategyPublicId, - sourceAssetPublicId: original, - targetAssetPublicId: original, - }), - ).rejects.toThrow(); - }); - - test("deleting the original keeps the copy's bytes; deleting both frees them", async () => { - vi.useFakeTimers(); - const fetchMock = vi.fn( - async (_input: RequestInfo | URL, _init?: RequestInit) => - new Response(null, { status: 204 }), - ); - vi.stubGlobal("fetch", fetchMock); - const { t, owner } = await createHarness(); - await copyImage(owner); - await addImage(owner, copy, secondPage); - - await owner.mutation(applyBatch, { - ...protocol, - strategyPublicId, - clientId: "remove-original", - ops: [ - { - opId: "remove-original", - type: "element.delete", - elementPublicId: original, - pagePublicId: firstPage, - expectedElementRevision: 1, - }, - ], - }); - vi.setSystemTime(Date.now() + 31 * 24 * 60 * 60 * 1000); - await t.mutation(purgeOldTombstones, {}); - await t.finishAllScheduledFunctions(vi.runAllTimers); - - expect(fetchMock).not.toHaveBeenCalled(); - expect((await images(owner))[copy]).toEqual(["active", url]); - - const revision = await t.run(async (ctx) => { - const strategy = await ctx.db - .query("strategies") - .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) - .unique(); - return strategy!.revision; - }); - await owner.mutation(deleteStrategy, { - ...protocol, - strategyPublicId, - expectedRevision: revision, - }); - await t.finishAllScheduledFunctions(vi.runAllTimers); - - expect( - fetchMock.mock.calls.map((call) => new URL(String(call[0])).pathname), - ).toEqual([`/copy-bucket/strategies/${strategyPublicId}/${original}.png`]); - }); -}); diff --git a/convex/images.ts b/convex/images.ts index 1438a1ae..945b30ec 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -7,7 +7,6 @@ import { isAssetReferenced, } from "./lib/assetReferences"; import { - copyActiveAssetToStrategy, getActiveAssetForStrategy, inferFileExtension, getViewerAssetForStrategy, @@ -24,7 +23,6 @@ import { internalAction, internalMutation, internalQuery, - mutation, query, type MutationCtx, type QueryCtx, @@ -385,87 +383,6 @@ export const createR2UploadIntent = internalMutation({ }, }); -/// Gives image `targetAssetPublicId` the picture the strategy already shows -/// as `sourceAssetPublicId`, for a placed image copied to another page: a -/// placed image's id is also its image's id, so the copy needs a row of its -/// own. The row points at the same stored bytes, as a duplicated strategy's -/// images do (`copyActiveAssetToStrategy`), so nothing is uploaded twice. -/// -/// The copy's content may reach the server first and leave a placeholder; -/// the copied row replaces it, as it replaces a failed upload's. Copying -/// again is harmless. "uploading" means an upload is still on its way (the -/// source's, so there is nothing to copy yet, or one under the target's id); -/// "unavailable" means the strategy cannot show the source either. -/// -/// The row is not swept on a timer: the copy's content may wait in a -/// device's outbox for days and still needs it. It goes as any image does, -/// once content that showed it is purged, or with the strategy; a copy whose -/// content never arrives keeps its row until then. -export const copyAsset = mutation({ - args: { - ...cloudProtocolArgs, - strategyPublicId: v.string(), - sourceAssetPublicId: v.string(), - targetAssetPublicId: v.string(), - }, - returns: v.union( - v.literal("copied"), - v.literal("uploading"), - v.literal("unavailable"), - ), - handler: async (ctx, args) => { - assertSupportedCloudProtocol(args.clientProtocolVersion); - if (args.sourceAssetPublicId === args.targetAssetPublicId) { - throw invalidPayloadError("An image cannot be copied onto itself."); - } - const strategy = await getStrategyByPublicId(ctx, args.strategyPublicId); - const { user } = await assertStrategyRole(ctx, strategy, "editor"); - - const target = await getViewerAssetForStrategy( - ctx, - strategy._id, - args.targetAssetPublicId, - ); - if (target !== null && !isUploadPlaceholder(target)) { - const status = inferUploadStatus(target); - if (status === "active") { - // Copied before, or another image entirely: only a row with the - // source's bytes is this copy. - const source = await getActiveAssetForStrategy( - ctx, - strategy._id, - args.sourceAssetPublicId, - ); - if ( - source !== null && - (source.objectKey !== target.objectKey || - source.storageId !== target.storageId) - ) { - throw conflictError("That image id already shows another image."); - } - return "copied"; - } - if (status === "pending") return "uploading"; - // A failed upload under the target's id: the copied row, being newer, - // is the one readers see, and the stale-upload sweep removes the - // failed one. - } - const copied = await copyActiveAssetToStrategy(ctx, { - sourceStrategyId: strategy._id, - sourceAssetPublicId: args.sourceAssetPublicId, - targetStrategyId: strategy._id, - targetAssetPublicId: args.targetAssetPublicId, - userId: user._id, - now: Date.now(), - }); - if (copied !== "copied") return copied; - if (target !== null && isUploadPlaceholder(target)) { - await ctx.db.delete(target._id); - } - return copied; - }, -}); - export const completeUpload = action({ args: { ...cloudProtocolArgs, diff --git a/lib/collab/generated/convex_models.dart b/lib/collab/generated/convex_models.dart index 9b121823..8d05c6ab 100644 --- a/lib/collab/generated/convex_models.dart +++ b/lib/collab/generated/convex_models.dart @@ -221,25 +221,6 @@ enum ImagesCompleteUploadArgsProvider { } } -enum ImagesCopyAssetResult { - copied('copied'), - unavailable('unavailable'), - uploading('uploading'); - - const ImagesCopyAssetResult(this.wireName); - final String wireName; - - static ImagesCopyAssetResult fromWireName(String wireName, String path) { - for (final value in values) { - if (value.wireName == wireName) return value; - } - throw ConvexDecodingException( - path, - 'unknown ImagesCopyAssetResult $wireName', - ); - } -} - enum ImagesGenerateUploadUrlResultProvider { r2('r2'); @@ -5429,27 +5410,6 @@ ImagesCompleteUploadResult decodeImagesCompleteUploadResult( 'images.js:completeUpload.returns', ); -ConvexObject encodeImagesCopyAssetArgs({ - required double clientProtocolVersion, - required String sourceAssetPublicId, - required String strategyPublicId, - required String targetAssetPublicId, -}) => ConvexObject({ - 'clientProtocolVersion': _encodeNumber( - clientProtocolVersion, - 'images.js:copyAsset.args.clientProtocolVersion', - ), - 'sourceAssetPublicId': ConvexString(sourceAssetPublicId), - 'strategyPublicId': ConvexString(strategyPublicId), - 'targetAssetPublicId': ConvexString(targetAssetPublicId), -}); - -ImagesCopyAssetResult decodeImagesCopyAssetResult(ConvexValue value) => - ImagesCopyAssetResult.fromWireName( - _decodeString(value, 'images.js:copyAsset.returns'), - 'images.js:copyAsset.returns', - ); - ConvexObject encodeImagesDeleteAssetRefArgs({ required String assetPublicId, required double clientProtocolVersion, diff --git a/lib/collab/generated/icarus_convex_api.dart b/lib/collab/generated/icarus_convex_api.dart index 12597b9f..a4facdd0 100644 --- a/lib/collab/generated/icarus_convex_api.dart +++ b/lib/collab/generated/icarus_convex_api.dart @@ -408,12 +408,6 @@ abstract interface class ImagesModule { ConvexOptional uploadId = const ConvexOptional.absent(), ConvexOptional width = const ConvexOptional.absent(), }); - Future copyAsset({ - required double clientProtocolVersion, - required String sourceAssetPublicId, - required String strategyPublicId, - required String targetAssetPublicId, - }); Future deleteAssetRef({ required String assetPublicId, required double clientProtocolVersion, @@ -484,25 +478,6 @@ final class _ImagesModule implements ImagesModule { ); } - @override - Future copyAsset({ - required double clientProtocolVersion, - required String sourceAssetPublicId, - required String strategyPublicId, - required String targetAssetPublicId, - }) { - final args = encodeImagesCopyAssetArgs( - clientProtocolVersion: clientProtocolVersion, - sourceAssetPublicId: sourceAssetPublicId, - strategyPublicId: strategyPublicId, - targetAssetPublicId: targetAssetPublicId, - ); - return _invoke( - () => _transport.mutation('images:copyAsset', args), - decodeImagesCopyAssetResult, - ); - } - @override Future deleteAssetRef({ required String assetPublicId, From 69bd2b48b6fbe85edfd5788d6d37467d51ba6930 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:06:40 -0400 Subject: [PATCH 10/11] Copy an image to another cloud page by its picture, not a copy of it A copied image now names its original's picture (assetId), so copying needs no call to the server first, and an image still uploading copies like any other: its copy shows the picture once the upload lands. Co-Authored-By: Claude Opus 5.5 --- lib/collab/cloud_media_models.dart | 13 -- lib/collab/convex_strategy_repository.dart | 21 --- lib/providers/strategy_provider.dart | 56 ++------ .../adjacent_page_copy_menu.dart | 10 -- test/strategy_page_session_provider_test.dart | 132 +++--------------- 5 files changed, 28 insertions(+), 204 deletions(-) diff --git a/lib/collab/cloud_media_models.dart b/lib/collab/cloud_media_models.dart index fde82618..6bb26a37 100644 --- a/lib/collab/cloud_media_models.dart +++ b/lib/collab/cloud_media_models.dart @@ -142,19 +142,6 @@ class CloudMediaUploadJob { } } -/// What became of giving a copied image the picture of the image it was -/// copied from (images:copyAsset). -enum CloudImageCopyResult { - /// The copy shows the original's picture. - copied, - - /// The original's upload has not finished, so there is nothing to copy yet. - uploading, - - /// The strategy cannot show the original either. - unavailable, -} - class CloudImageUploadIntent { const CloudImageUploadIntent({ required this.provider, diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 0cdd785b..89c6330b 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -231,27 +231,6 @@ class ConvexStrategyRepository { )); } - /// Gives image [targetAssetPublicId] the picture the strategy shows as - /// [sourceAssetPublicId], sharing its stored bytes: for a placed image - /// copied to another page, whose id is also its image's id. - Future copyImageAsset({ - required String strategyPublicId, - required String sourceAssetPublicId, - required String targetAssetPublicId, - }) async { - final result = await _api.images.copyAsset( - clientProtocolVersion: currentCloudProtocolVersion.toDouble(), - strategyPublicId: strategyPublicId, - sourceAssetPublicId: sourceAssetPublicId, - targetAssetPublicId: targetAssetPublicId, - ); - return switch (result) { - ImagesCopyAssetResult.copied => CloudImageCopyResult.copied, - ImagesCopyAssetResult.uploading => CloudImageCopyResult.uploading, - ImagesCopyAssetResult.unavailable => CloudImageCopyResult.unavailable, - }; - } - Future generateImageUploadUrl({ required String strategyPublicId, required String assetPublicId, diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index ca9bcbee..ab4ec2c0 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -75,12 +75,6 @@ enum PageCopyResult { /// There was nothing to copy, or no page to copy it to. unavailable, - - /// The image's upload has not finished, so it cannot be copied yet. - imageUploading, - - /// The server cannot show the image, so it was not copied. - imageUnavailable, } class StrategyProvider extends Notifier { @@ -1060,46 +1054,6 @@ class StrategyProvider extends Notifier { 255) { return PageCopyResult.unavailable; } - // An image's id also names its picture on the server, so the copy gets - // the original's picture under its own id first. It shares the stored - // bytes, so nothing is uploaded again. - if (element.kind == 'image') { - final CloudImageCopyResult picture; - try { - picture = - await ref.read(convexStrategyRepositoryProvider).copyImageAsset( - strategyPublicId: strategyId, - sourceAssetPublicId: widgetId, - targetAssetPublicId: copyId, - ); - } catch (error) { - log('Could not copy the picture of image $widgetId: $error'); - return PageCopyResult.unreachable; - } - switch (picture) { - case CloudImageCopyResult.uploading: - return PageCopyResult.imageUploading; - case CloudImageCopyResult.unavailable: - // An image this device placed may not have reached the server yet. - // Its upload only goes once the image itself is saved to send - // (referenceDurable); one whose save failed never will. - final stillUploading = ref - .read(cloudMediaUploadQueueProvider) - .jobsForStrategy(strategyId) - .any( - (job) => - job.assetPublicId == widgetId && - job.referenceDurable && - job.state != CloudMediaJobState.failed, - ); - return stillUploading - ? PageCopyResult.imageUploading - : PageCopyResult.imageUnavailable; - case CloudImageCopyResult.copied: - break; - } - if (state.strategyId != strategyId) return PageCopyResult.unavailable; - } // The canvas never draws the copy: its page shows it from the server. final queued = await ref.read(strategyOpQueueProvider.notifier).enqueueOffCanvas( @@ -1109,7 +1063,15 @@ class StrategyProvider extends Notifier { pagePublicId: targetPageId, payload: cloudElementPayload( kind: element.kind, - data: {...element.data, 'id': copyId}, + data: { + ...element.data, + 'id': copyId, + // A copied image shows its original's picture: nothing + // is copied or uploaded, and nothing waits on an upload + // still under way (see PlacedImage.assetId). + if (element.kind == 'image') + 'assetId': element.data['assetId'] ?? widgetId, + }, ), sortIndex: 1 + onTarget.values.fold(-1, max), ), diff --git a/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart b/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart index c9fbf8e4..ee18d4a2 100644 --- a/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart +++ b/lib/widgets/draggable_widgets/adjacent_page_copy_menu.dart @@ -39,16 +39,6 @@ List buildAdjacentPageCopyMenuItems( 'copied.', backgroundColor: Settings.tacticalVioletTheme.destructive, ); - case PageCopyResult.imageUploading: - Settings.showToast( - message: 'The image is still uploading. Copy it again in a moment.', - backgroundColor: Settings.tacticalVioletTheme.primary, - ); - case PageCopyResult.imageUnavailable: - Settings.showToast( - message: "The cloud doesn't have this image, so it wasn't copied.", - backgroundColor: Settings.tacticalVioletTheme.destructive, - ); case PageCopyResult.copied || PageCopyResult.unavailable: break; } diff --git a/test/strategy_page_session_provider_test.dart b/test/strategy_page_session_provider_test.dart index d51db593..525ed1ac 100644 --- a/test/strategy_page_session_provider_test.dart +++ b/test/strategy_page_session_provider_test.dart @@ -11524,13 +11524,10 @@ void main() { }); group('an image', () { - /// Opens page 2 with a placed image on it, whose picture the server - /// copies with [result] (null: the call fails, as when offline). - Future<(ProviderContainer, _PageReader)> openWithImage( - CloudImageCopyResult? result, - ) async { - final (container, _, reader) = await open(); - reader.imageCopy = result; + /// Opens page 2 with a placed image on it, showing picture + /// [assetId] when given (it is itself a copy), else its own. + Future openWithImage({String? assetId}) async { + final (container, _, _) = await open(); container.read(placedImageProvider.notifier).fromHive([ PlacedImage( id: 'image', @@ -11538,10 +11535,11 @@ void main() { aspectRatio: 1.5, scale: 100, fileExtension: '.png', + assetId: assetId, ), ]); await _settle(); - return (container, reader); + return container; } Iterable copies(ProviderContainer container) => @@ -11554,9 +11552,9 @@ void main() { direction: PageTransitionDirection.forward, ); - test('gets its picture copied under the new id, then is sent', () async { - final (container, reader) = - await openWithImage(CloudImageCopyResult.copied); + test('is copied showing its picture, however far its upload has got', + () async { + final container = await openWithImage(); expect( container .read(strategyProvider.notifier) @@ -11564,97 +11562,27 @@ void main() { [PageTransitionDirection.forward, PageTransitionDirection.backward], ); - // Nothing is queued until the picture is copied. - final picture = reader.imageCopyGate = Completer(); - final copied = copy(container); - await _settle(); - expect(copies(container), isEmpty); - picture.complete(); - expect(await copied, PageCopyResult.copied); + expect(await copy(container), PageCopyResult.copied); final add = copies(container).single; expect(add.payload['kind'], 'image'); final data = cloudPayloadData(add.payload); expect(data['id'], add.elementPublicId); - expect(data['aspectRatio'], 1.5); expect(pageCopyRoot(add.elementPublicId), 'image'); - expect(reader.imageCopies, [('image', add.elementPublicId)]); + // The copy shows the original's picture. + expect(data['assetId'], 'image'); + expect(data['aspectRatio'], 1.5); await _settle(); }); - test('still uploading is not copied yet', () async { - final (container, _) = - await openWithImage(CloudImageCopyResult.uploading); - - expect(await copy(container), PageCopyResult.imageUploading); - expect(copies(container), isEmpty); - }); - - /// Opens the image, unknown to the server, with this device's upload - /// of it in [state]; [saved] false: the image's own save failed, so - /// its upload never goes. - Future openUploading( - CloudMediaJobState state, { - bool saved = true, - }) async { - final (container, _) = - await openWithImage(CloudImageCopyResult.unavailable); - container.read(cloudMediaUploadQueueProvider.notifier).state = - CloudMediaUploadQueueState( - jobs: [ - CloudMediaUploadJob( - jobId: 'image', - accountId: 'account-a', - strategyPublicId: 'cloud-strategy', - assetPublicId: 'image', - fileExtension: '.png', - mimeType: 'image/png', - state: state, - attempts: 0, - updatedAt: DateTime.utc(2026), - referenceDurable: saved, - ), - ], - isProcessing: false, - ); - return container; - } + test("a copy's copy shows the first picture", () async { + final container = await openWithImage(assetId: 'first-image'); - test('this device is still uploading is not copied yet', () async { - for (final state in [ - CloudMediaJobState.pendingUpload, - CloudMediaJobState.pendingAttach, - ]) { - final container = await openUploading(state); - expect(await copy(container), PageCopyResult.imageUploading); - expect(copies(container), isEmpty); - } - }); + expect(await copy(container), PageCopyResult.copied); - test('whose upload failed or will never go is not called uploading', - () async { - for (final container in [ - await openUploading(CloudMediaJobState.failed), - await openUploading(CloudMediaJobState.pendingUpload, saved: false), - ]) { - expect(await copy(container), PageCopyResult.imageUnavailable); - expect(copies(container), isEmpty); - } - }); - - test('the cloud cannot show is not copied', () async { - final (container, _) = - await openWithImage(CloudImageCopyResult.unavailable); - - expect(await copy(container), PageCopyResult.imageUnavailable); - expect(copies(container), isEmpty); - }); - - test('whose picture cannot be copied right now is not copied', () async { - final (container, _) = await openWithImage(null); - - expect(await copy(container), PageCopyResult.unreachable); - expect(copies(container), isEmpty); + expect(cloudPayloadData(copies(container).single.payload)['assetId'], + 'first-image'); + await _settle(); }); }); }); @@ -11672,28 +11600,6 @@ class _PageReader extends Fake implements ConvexStrategyRepository { /// While set, a read waits for it. Completer? gate; - /// What copying an image's picture returns; null: the call fails. - CloudImageCopyResult? imageCopy = CloudImageCopyResult.copied; - - /// While set, copying an image's picture waits for it. - Completer? imageCopyGate; - - /// The pictures copied, as (source, target) image ids. - final List<(String, String)> imageCopies = []; - - @override - Future copyImageAsset({ - required String strategyPublicId, - required String sourceAssetPublicId, - required String targetAssetPublicId, - }) async { - await imageCopyGate?.future; - final result = imageCopy; - if (result == null) throw const SocketException('offline'); - imageCopies.add((sourceAssetPublicId, targetAssetPublicId)); - return result; - } - @override Future fetchPageSnapshot({ required String strategyPublicId, From 9f4b327e7a09e03c7e0ea5cee4bf7a44d8fded68 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:40:11 -0400 Subject: [PATCH 11/11] Ask the server for images' picture ids This client keeps PlacedImage.assetId, so it reads snapshots and op results with it (acceptsPictureIds); older clients get them without. Co-Authored-By: Claude Opus 5.5 --- lib/collab/convex_strategy_repository.dart | 6 +++ test/collab/referenced_asset_ids_test.dart | 47 ++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 89c6330b..db86f107 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -163,6 +163,7 @@ class ConvexStrategyRepository { strategyPublicId: strategyPublicId, pagePublicId: pagePublicId, shareToken: _optional(shareToken), + acceptsPictureIds: const ConvexOptional.present(true), ) .fetch(), ); @@ -179,6 +180,8 @@ class ConvexStrategyRepository { strategyPublicId: strategyPublicId, pagePublicId: pagePublicId, shareToken: _optional(shareToken), + // This client keeps an image's picture id (PlacedImage.assetId). + acceptsPictureIds: const ConvexOptional.present(true), ) .watch() .map(_pageSnapshot); @@ -226,6 +229,7 @@ class ConvexStrategyRepository { // This client checks image references apart, so it can take a // snapshot without the pages in the server's trash. acceptsTrashedPagesLeftOut: const ConvexOptional.present(true), + acceptsPictureIds: const ConvexOptional.present(true), ) .fetch(), )); @@ -351,6 +355,8 @@ class ConvexStrategyRepository { accountSubject: _optional(accountSubject), // This client restores deleted pages, so it sends such a delete again. checkTrashedPageDeletes: const ConvexOptional.present(true), + // This client keeps an image's picture id (PlacedImage.assetId). + acceptsPictureIds: const ConvexOptional.present(true), ); return result.results.map(_opAck).toList(growable: false); } diff --git a/test/collab/referenced_asset_ids_test.dart b/test/collab/referenced_asset_ids_test.dart index c9ebafd5..1324d8f7 100644 --- a/test/collab/referenced_asset_ids_test.dart +++ b/test/collab/referenced_asset_ids_test.dart @@ -1,4 +1,5 @@ import 'package:flutter_test/flutter_test.dart'; +import 'package:icarus/collab/collab_models.dart'; import 'package:icarus/collab/convex_strategy_repository.dart'; import 'package:icarus/collab/generated/generated.dart'; import 'package:icarus/collab/transport/convex_transport.dart'; @@ -36,6 +37,46 @@ void main() { isTrue); }); + test("reads and writes as a client that keeps images' picture ids", () async { + final transport = _RecordingTransport(); + final repository = ConvexStrategyRepository(IcarusConvexApi(transport)); + + await expectLater( + repository.fetchFullSnapshot('strategy-a'), throwsStateError); + await expectLater( + repository.fetchPageSnapshot( + strategyPublicId: 'strategy-a', + pagePublicId: 'page-1', + ), + throwsStateError, + ); + await expectLater( + repository.applyBatch( + strategyPublicId: 'strategy-a', + clientId: 'client-a', + ops: const [ + ElementDeleteOp( + opId: 'op-1', + pagePublicId: 'page-1', + elementPublicId: 'image-1', + expectedElementRevision: 1, + ), + ], + ), + throwsStateError, + ); + + expect(transport.calls.map((call) => call.$1), [ + 'strategy:getFullSnapshot', + 'page:getSnapshot', + 'ops:applyBatch', + ]); + for (final (name, args) in transport.calls) { + expect((args.value['acceptsPictureIds'] as ConvexBoolean?)?.value, isTrue, + reason: name); + } + }); + test('cannot tell if any batch cannot', () async { final transport = _ReferencesTransport({'image-3'}, nullBatch: 1); final repository = ConvexStrategyRepository(IcarusConvexApi(transport)); @@ -83,6 +124,12 @@ final class _RecordingTransport implements ConvexTransport { throw StateError('not answered in this test'); } + @override + Future mutation(String name, ConvexObject args) async { + calls.add((name, args)); + throw StateError('not answered in this test'); + } + @override dynamic noSuchMethod(Invocation invocation) => throw UnimplementedError(); }