From 09301ab3cc8511c29c2b6445d73f84de49705607 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 15:40:43 -0400 Subject: [PATCH 1/4] Let an image show another image's picture An image element may name the picture it shows (`assetId`); without one, its own id names it, as before. A copy of an image can then be a new element showing its original's picture, with nothing copied or uploaded, and nothing to wait for while the original is still uploading. References, upload placeholders and cleanup all read the picture through collectAssetIdFromElementPayload, so they follow. For builds from before the field, page and strategy snapshots also list each picture under the id of every image showing it, and a whole write that leaves the field out keeps it: an image never changes picture. Co-Authored-By: Claude Opus 5.5 --- convex/lib/imageAssets.ts | 46 +++++++- convex/ops.ts | 6 +- convex/page.ts | 6 +- convex/pictureId.test.ts | 229 ++++++++++++++++++++++++++++++++++++++ convex/strategy.ts | 6 +- 5 files changed, 287 insertions(+), 6 deletions(-) create mode 100644 convex/pictureId.test.ts diff --git a/convex/lib/imageAssets.ts b/convex/lib/imageAssets.ts index 38ad835a..e680a99a 100644 --- a/convex/lib/imageAssets.ts +++ b/convex/lib/imageAssets.ts @@ -46,10 +46,54 @@ export function inferFileExtension( return match?.[1]?.toLowerCase() ?? ""; } +/// The picture an image element shows: its `assetId` when it has one, else +/// its own id. A copy of an image is a new element showing its original's +/// picture, so it needs no picture of its own, and is never waiting on an +/// upload its original is still making. Only images have an `assetId`. export function collectAssetIdFromElementPayload( payload: Doc<"elements">["payload"], ): string | null { - return typeof payload.data.id === "string" ? payload.data.id : null; + const { assetId, id } = payload.data; + if (typeof assetId === "string" && assetId.length > 0) return assetId; + return typeof id === "string" ? id : null; +} + +/// [next] keeping the picture [current] shows, when it leaves the picture +/// out. An image never changes picture, and builds from before pictures had +/// their own id write an image's whole payload without the field: moving a +/// copy there must not cut it off from its picture. +export function keepPictureId( + current: Doc<"elements">["payload"], + next: Doc<"elements">["payload"], +): Doc<"elements">["payload"] { + const assetId = current.data.assetId; + if (typeof assetId !== "string" || "assetId" in next.data) return next; + return { ...next, data: { ...next.data, assetId } }; +} + +/// [assets] as builds from before pictures had their own id can read them: +/// they look an image's picture up under the image's own id. Each live image +/// showing another id's picture gets that picture under its own id too. +export function withPictureAliases( + assets: T[], + elements: Doc<"elements">[], +): T[] { + const byId = new Map(assets.map((asset) => [asset.publicId, asset])); + const aliases: T[] = []; + for (const element of elements) { + if (element.deleted || element.elementType !== "image") continue; + const id = element.payload.data.id; + const pictureId = collectAssetIdFromElementPayload(element.payload); + if (typeof id !== "string" || pictureId === null || pictureId === id) { + continue; + } + const picture = byId.get(pictureId); + if (picture === undefined || byId.has(id)) continue; + const alias = { ...picture, publicId: id }; + byId.set(id, alias); + aliases.push(alias); + } + return [...assets, ...aliases]; } /// The images a lineup group shows: links[*].images[*].id, across every diff --git a/convex/ops.ts b/convex/ops.ts index b0ecad83..a3a6a266 100644 --- a/convex/ops.ts +++ b/convex/ops.ts @@ -8,6 +8,7 @@ import { } from "./lib/strategyAgentSummary"; import { expectAssets, + keepPictureId, referencedAssetIds, staleUploadAgeMs, } from "./lib/imageAssets"; @@ -1231,7 +1232,9 @@ async function applyElementOp( throw errorWithCode("MISSING_PAGE_PUBLIC_ID", "Missing pagePublicId"); } const page = await requireTargetPage(ctx, strategy, op.pagePublicId); - const payload = assertElementPayload(op.payload); + const sent = assertElementPayload(op.payload); + const payload = + existing === null ? sent : keepPictureId(existing.payload, sent); if (existing !== null) { if (existing.strategyId !== strategy._id) { return rejected("element_strategy_mismatch"); @@ -1379,6 +1382,7 @@ async function applyElementOp( checkRevision = true; } } + payload = keepPictureId(existing.payload, payload); setIfChanged(patch, "payload", existing.payload, payload); setIfChanged(patch, "payloadKind", existing.payloadKind, payload.kind); setIfChanged( diff --git a/convex/page.ts b/convex/page.ts index 535a5421..93a33977 100644 --- a/convex/page.ts +++ b/convex/page.ts @@ -8,6 +8,7 @@ import { collectReferencedAssetIds, getViewerAssetForStrategy, serializeAssetForViewer, + withPictureAliases, } from "./lib/imageAssets"; import { serializeElement, @@ -76,12 +77,13 @@ export const getSnapshot = query({ .map((lineup) => serializeLineup(strategy.publicId, page.publicId, lineup), ), - assets: ( + assets: withPictureAliases( await Promise.all( assets .filter((asset): asset is Doc<"imageAssets"> => asset !== null) .map((asset) => serializeAssetForViewer(ctx, asset)), - ) + ), + elements, ).sort((left, right) => left.publicId.localeCompare(right.publicId)), }; }, diff --git a/convex/pictureId.test.ts b/convex/pictureId.test.ts new file mode 100644 index 00000000..e514ea72 --- /dev/null +++ b/convex/pictureId.test.ts @@ -0,0 +1,229 @@ +import { + convexTest, + type TestConvexForDataModel, + type TestConvexForDataModelAndIdentity, +} from "convex-test"; +import { makeFunctionReference } from "convex/server"; +import { beforeAll, describe, expect, test } 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 applyBatch = makeFunctionReference<"mutation">("ops:applyBatch"); +const getPageSnapshot = makeFunctionReference<"query">("page:getSnapshot"); +const getFullSnapshot = makeFunctionReference<"query">( + "strategy:getFullSnapshot", +); + +type Harness = TestConvexForDataModel; +type RootHarness = TestConvexForDataModelAndIdentity; +type Row = Record; + +const protocol = { clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION }; +const strategy = "picture-strategy"; +const page = "picture-page"; +const pictureUrl = `https://media.picture.test/strategies/${strategy}/original.png`; + +beforeAll(() => { + process.env.R2_ACCOUNT_ID = "picture-account"; + process.env.R2_BUCKET = "picture-bucket"; + process.env.R2_ACCESS_KEY_ID = "picture-access-key"; + process.env.R2_SECRET_ACCESS_KEY = "picture-secret"; + process.env.R2_PUBLIC_BASE_URL = "https://media.picture.test"; + process.env.R2_S3_ENDPOINT = "https://picture.r2.test"; +}); + +/// A strategy with an uploaded image, "original", and a copy of it, "copy", +/// that shows the original's picture. +async function seed(): Promise<{ t: RootHarness; owner: Harness }> { + const t = convexTest(schema, modules); + await t.run(markAssetReferencesReady); + const owner = t.withIdentity({ + issuer: "https://picture.test", + subject: "owner", + tokenIdentifier: "picture|owner", + name: "Owner", + }); + await owner.mutation(ensureCurrentUser, protocol); + await owner.mutation(createStrategy, { + ...protocol, + publicId: strategy, + name: "Pictures", + mapData: "ascent", + initialPagePublicId: page, + initialPageName: "Page 1", + initialPageIsAttack: true, + }); + await t.run(async (ctx) => { + const row = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategy)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: "original", + provider: "r2", + strategyId: row!._id, + objectKey: `strategies/${strategy}/original.png`, + uploadStatus: "active", + fileExtension: ".png", + mimeType: "image/png", + width: 64, + height: 32, + byteSize: 100, + uploadedAt: now, + createdAt: now, + updatedAt: now, + }); + }); + await send(owner, "seed", [ + addImage("add-original", { id: "original", scale: 1 }), + addImage("add-copy", { id: "copy", assetId: "original", scale: 1 }), + ]); + return { t, owner }; +} + +function addImage(opId: string, data: Record) { + return { + opId, + type: "element.add", + elementPublicId: data.id, + pagePublicId: page, + payload: { kind: "image", payloadVersion: 1, data }, + sortIndex: 0, + }; +} + +async function send(owner: Harness, clientId: string, ops: unknown[]) { + const { results } = (await owner.mutation(applyBatch, { + ...protocol, + strategyPublicId: strategy, + clientId, + ops, + })) as { results: Row[] }; + return results; +} + +async function copyRow(t: RootHarness) { + return await t.run(async (ctx) => + (await ctx.db.query("elements").collect()).find( + (row) => row.publicId === "copy", + ), + ); +} + +describe("an image showing another image's picture", () => { + test("shows the picture, under its own id too for builds that look it up there", async () => { + const { owner } = await seed(); + + const pageSnapshot = (await owner.query(getPageSnapshot, { + ...protocol, + strategyPublicId: strategy, + pagePublicId: page, + })) as { assets: Row[] }; + const full = (await owner.query(getFullSnapshot, { + ...protocol, + strategyPublicId: strategy, + })) as { assets: Row[] }; + + for (const assets of [pageSnapshot.assets, full.assets]) { + expect(assets.map((asset) => [asset.publicId, asset.url])).toEqual([ + ["copy", pictureUrl], + ["original", pictureUrl], + ]); + } + }); + + test("keeps the picture referenced after the original is deleted", async () => { + const { t, owner } = await seed(); + await send(owner, "delete", [ + { + opId: "delete-original", + type: "element.delete", + elementPublicId: "original", + pagePublicId: page, + expectedElementRevision: 1, + }, + ]); + + const references = await t.run(async (ctx) => { + const copy = (await ctx.db.query("elements").collect()).find( + (row) => row.publicId === "copy", + )!; + return (await ctx.db.query("assetReferences").collect()) + .filter((ref) => ref.elementId === copy._id) + .map((ref) => [ref.assetPublicId, ref.deleted]); + }); + expect(references).toEqual([["original", false]]); + const pageSnapshot = (await owner.query(getPageSnapshot, { + ...protocol, + strategyPublicId: strategy, + pagePublicId: page, + })) as { assets: Row[] }; + expect(pageSnapshot.assets.map((asset) => asset.publicId)).toEqual([ + "copy", + "original", + ]); + }); + + test("keeps its picture when an older build writes it whole without it", async () => { + const { t, owner } = await seed(); + + // An older build moves the copy, sending the payload it knows. + const [moved] = await send(owner, "old-build", [ + { + opId: "move-copy", + type: "element.patch", + elementPublicId: "copy", + pagePublicId: page, + payload: { + kind: "image", + payloadVersion: 1, + data: { id: "copy", scale: 2 }, + }, + expectedElementRevision: 1, + }, + ]); + + expect(moved).toMatchObject({ status: "applied" }); + expect((await copyRow(t))!.payload.data).toEqual({ + id: "copy", + assetId: "original", + scale: 2, + }); + }); + + test("keeps its picture when an older build restores it without it", async () => { + const { t, owner } = await seed(); + await send(owner, "delete", [ + { + opId: "delete-copy", + type: "element.delete", + elementPublicId: "copy", + pagePublicId: page, + expectedElementRevision: 1, + }, + ]); + + // Undo on an older build adds the copy back as it knows it. + const [restored] = await send(owner, "old-build", [ + { + ...addImage("restore-copy", { id: "copy", scale: 1 }), + expectedElementRevision: 2, + }, + ]); + + expect(restored).toMatchObject({ status: "applied" }); + const copy = (await copyRow(t))!; + expect(copy.deleted).toBe(false); + expect(copy.payload.data.assetId).toBe("original"); + }); +}); diff --git a/convex/strategy.ts b/convex/strategy.ts index a07e18d2..89e3b3d3 100644 --- a/convex/strategy.ts +++ b/convex/strategy.ts @@ -19,6 +19,7 @@ import { collectReferencedAssetIds, getViewerAssetForStrategy, serializeAssetForViewer, + withPictureAliases, } from "./lib/imageAssets"; import { serializeElement, @@ -157,12 +158,13 @@ export const getFullSnapshot = query({ lineup, ), ), - assets: ( + assets: withPictureAliases( await Promise.all( assets .filter((asset): asset is Doc<"imageAssets"> => asset !== null) .map((asset) => serializeAssetForViewer(ctx, asset)), - ) + ), + visibleElements, ).sort((left, right) => left.publicId.localeCompare(right.publicId)), }; }, From 366567721653346ae1cfef6544f94fc88d08dc1b Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:35:58 -0400 Subject: [PATCH 2/4] Serve old builds pictures by image id, and keep legacy pictures findable Three gaps for builds and rows from before picture ids: - Image payloads go without assetId to clients that don't ask for it (acceptsPictureIds on the snapshots and applyBatch): such a client keeps no assetId, so a copy would read to it as an unsaved change. - images:getAssetUrl answers for an image's own id with the picture it shows, as old builds ask when a picture's address expires. - A picture from before upload statuses is looked up among its own strategy's rows first: copies keep pictures' ids across strategies, so twenty copies could fill a lookup of every strategy's rows. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 18 +++++ convex/images.ts | 11 ++- convex/lib/imageAssets.ts | 64 ++++++++++++--- convex/lib/snapshotSerialization.ts | 8 +- convex/ops.ts | 22 +++++- convex/page.ts | 12 ++- convex/pictureId.test.ts | 88 +++++++++++++++++++++ convex/strategy.ts | 6 ++ lib/collab/generated/convex_models.dart | 9 +++ lib/collab/generated/icarus_convex_api.dart | 9 +++ 10 files changed, 226 insertions(+), 21 deletions(-) diff --git a/convex/function_spec.json b/convex/function_spec.json index 93232fbf..1ac6bcdf 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -48407,6 +48407,12 @@ "args": { "type": "object", "value": { + "acceptsPictureIds": { + "fieldType": { + "type": "boolean" + }, + "optional": true + }, "accountSubject": { "fieldType": { "type": "string" @@ -144197,6 +144203,12 @@ "args": { "type": "object", "value": { + "acceptsPictureIds": { + "fieldType": { + "type": "boolean" + }, + "optional": true + }, "clientProtocolVersion": { "fieldType": { "type": "number" @@ -169716,6 +169728,12 @@ "args": { "type": "object", "value": { + "acceptsPictureIds": { + "fieldType": { + "type": "boolean" + }, + "optional": true + }, "acceptsTrashedPagesLeftOut": { "fieldType": { "type": "boolean" diff --git a/convex/images.ts b/convex/images.ts index 945b30ec..cd77db50 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -13,6 +13,7 @@ import { inferProvider, inferUploadStatus, isUploadPlaceholder, + pictureShownByImage, serializeAssetForViewer, staleUploadAgeMs, type Provider, @@ -799,16 +800,18 @@ export const getAssetUrl = query({ const strategy = await getStrategyByPublicId(ctx, args.strategyPublicId); await assertStrategyReadable(ctx, strategy, args.shareToken); - if ( - !(await strategyReferencesAsset(ctx, strategy._id, args.assetPublicId)) - ) { + // Builds from before pictures had their own id ask by the image's id. + const pictureId = + (await pictureShownByImage(ctx, strategy._id, args.assetPublicId)) ?? + args.assetPublicId; + if (!(await strategyReferencesAsset(ctx, strategy._id, pictureId))) { throw notFoundError("Asset", args.assetPublicId); } const asset = await getActiveAssetForStrategy( ctx, strategy._id, - args.assetPublicId, + pictureId, ); if (asset === null) { throw notFoundError("Asset", args.assetPublicId); diff --git a/convex/lib/imageAssets.ts b/convex/lib/imageAssets.ts index e680a99a..4ca44222 100644 --- a/convex/lib/imageAssets.ts +++ b/convex/lib/imageAssets.ts @@ -185,18 +185,58 @@ export async function getActiveAssetForStrategy( return strategyAsset; } - const legacyCandidates = await ctx.db - .query("imageAssets") - .withIndex("by_publicId", (q) => q.eq("publicId", assetPublicId)) - .order("desc") - .take(20); - return ( - legacyCandidates.find( - (asset) => - (asset.strategyId === undefined || asset.strategyId === strategyId) && - isVisibleAsset(asset), - ) ?? null - ); + // Rows from before upload statuses, this strategy's then those of no + // strategy. Each is read on its own: copies into other strategies keep + // their pictures' ids, so a read of every strategy's rows could fill its + // limit with theirs. + for (const owner of [strategyId, undefined]) { + const legacyCandidates = await ctx.db + .query("imageAssets") + .withIndex("by_strategyId_and_publicId", (q) => + q.eq("strategyId", owner).eq("publicId", assetPublicId), + ) + .order("desc") + .take(20); + const visible = legacyCandidates.find(isVisibleAsset); + if (visible !== undefined) return visible; + } + return null; +} + +/// The picture image [itemPublicId] of the strategy shows, when the image +/// shows another's (see collectAssetIdFromElementPayload): builds from +/// before pictures had their own id ask for a picture by the image's id. +export async function pictureShownByImage( + ctx: AnyCtx, + strategyId: Id<"strategies">, + itemPublicId: string, +): Promise { + const element = await ctx.db + .query("elements") + .withIndex("by_publicId", (q) => q.eq("publicId", itemPublicId)) + .first(); + if ( + element === null || + element.deleted || + element.strategyId !== strategyId || + element.elementType !== "image" + ) { + return null; + } + const pictureId = collectAssetIdFromElementPayload(element.payload); + return pictureId === itemPublicId ? null : pictureId; +} + +/// [payload] as builds from before pictures had their own id hold it: they +/// keep no `assetId`, so one sent to them reads as a change they made. +/// They find the picture by the image's own id (withPictureAliases), and a +/// write of theirs keeps the stored picture (keepPictureId). +export function withoutPictureId( + payload: Doc<"elements">["payload"], +): Doc<"elements">["payload"] { + if (!("assetId" in payload.data)) return payload; + const { assetId: _assetId, ...data } = payload.data; + return { ...payload, data }; } /// A row recording that a strategy's content shows an image whose upload has diff --git a/convex/lib/snapshotSerialization.ts b/convex/lib/snapshotSerialization.ts index 3946cbde..efbbfffa 100644 --- a/convex/lib/snapshotSerialization.ts +++ b/convex/lib/snapshotSerialization.ts @@ -1,4 +1,5 @@ import type { Doc } from "../_generated/dataModel"; +import { withoutPictureId } from "./imageAssets"; export function serializeStrategyHeader( strategy: Doc<"strategies">, @@ -45,17 +46,22 @@ export function serializePageContent(pageContent: Doc<"pageContents">) { }; } +/// [element] as a client sees it. One that doesn't [acceptsPictureIds] +/// gets its payload without the picture id (see withoutPictureId). export function serializeElement( strategyPublicId: string, pagePublicId: string, element: Doc<"elements">, + acceptsPictureIds: boolean, ) { return { publicId: element.publicId, strategyPublicId, pagePublicId, elementType: element.elementType, - payload: element.payload, + payload: acceptsPictureIds + ? element.payload + : withoutPictureId(element.payload), sortIndex: element.sortIndex, revision: element.revision, deleted: element.deleted, diff --git a/convex/ops.ts b/convex/ops.ts index a3a6a266..1909d42f 100644 --- a/convex/ops.ts +++ b/convex/ops.ts @@ -10,6 +10,7 @@ import { expectAssets, keepPictureId, referencedAssetIds, + withoutPictureId, staleUploadAgeMs, } from "./lib/imageAssets"; import { @@ -785,6 +786,7 @@ function isRejectionReason( function toPublicResult( op: StrategyOp, result: OperationResult, + acceptsPictureIds: boolean, ): PublicOperationResult { if (result.status === "failed") { return { @@ -828,7 +830,10 @@ function toPublicResult( current: { type: currentTargetForOp(op), revision: result.latestRevision, - value: result.latestPayload, + value: + op.entityType === "element" && !acceptsPictureIds + ? withoutPictureId(result.latestPayload as ElementPayload) + : result.latestPayload, } as Infer, }), }; @@ -1780,6 +1785,11 @@ export const applyBatch = mutation({ // once the page is back. Older clients get the no-op a deleted page's // purged content gave them (see refuseDeleteOffLivePage). checkTrashedPageDeletes: v.optional(v.boolean()), + // Set by clients that keep an image's picture id (assetId, see + // collectAssetIdFromElementPayload). Older clients get image payloads + // without it, as they would write them, and find pictures under each + // image's own id (withPictureAliases). + acceptsPictureIds: v.optional(v.boolean()), // Sent by clients on protocol 4, which stored lineups as origin, // landing and link rows. Ignored: accepting them lets such a client // reach the protocol gate (CLIENT_UPGRADE_REQUIRED) instead of failing @@ -1849,7 +1859,9 @@ export const applyBatch = mutation({ latestPayload: latest?.payload, } : noop(existingEvent.appliedRevision); - results.push(toPublicResult(op, replayResult)); + results.push( + toPublicResult(op, replayResult, args.acceptsPictureIds === true), + ); continue; } @@ -1955,7 +1967,11 @@ export const applyBatch = mutation({ acceptedStrategyBatchBaseRevision = originalExpectedRevision; } - const publicResult = toPublicResult(op, result); + const publicResult = toPublicResult( + op, + result, + args.acceptsPictureIds === true, + ); await ctx.db.insert("operationEvents", { strategyId: strategy._id, diff --git a/convex/page.ts b/convex/page.ts index 93a33977..c4bb4ecb 100644 --- a/convex/page.ts +++ b/convex/page.ts @@ -28,6 +28,11 @@ export const getSnapshot = query({ strategyPublicId: v.string(), pagePublicId: v.string(), shareToken: v.optional(v.string()), + // Set by clients that keep an image's picture id (assetId, see + // collectAssetIdFromElementPayload). Older clients get image payloads + // without it, as they would write them, and find pictures under each + // image's own id (withPictureAliases). + acceptsPictureIds: v.optional(v.boolean()), }, returns: pageSnapshotValidator, handler: async (ctx, args) => { @@ -70,7 +75,12 @@ export const getSnapshot = query({ elements: elements .sort((left, right) => left.sortIndex - right.sortIndex) .map((element) => - serializeElement(strategy.publicId, page.publicId, element), + serializeElement( + strategy.publicId, + page.publicId, + element, + args.acceptsPictureIds === true, + ), ), lineups: lineups .sort((left, right) => left.sortIndex - right.sortIndex) diff --git a/convex/pictureId.test.ts b/convex/pictureId.test.ts index e514ea72..f017df22 100644 --- a/convex/pictureId.test.ts +++ b/convex/pictureId.test.ts @@ -10,6 +10,7 @@ import { markAssetReferencesReady } from "./lib/assetReferences"; import { CURRENT_CLOUD_PROTOCOL_VERSION } from "./lib/cloudProtocol"; import schema from "./schema"; import { modules } from "./test.setup"; +import { getActiveAssetForStrategy } from "./lib/imageAssets"; const ensureCurrentUser = makeFunctionReference<"mutation">( "users:ensureCurrentUser", @@ -22,6 +23,7 @@ const getPageSnapshot = makeFunctionReference<"query">("page:getSnapshot"); const getFullSnapshot = makeFunctionReference<"query">( "strategy:getFullSnapshot", ); +const getAssetUrl = makeFunctionReference<"query">("images:getAssetUrl"); type Harness = TestConvexForDataModel; type RootHarness = TestConvexForDataModelAndIdentity; @@ -226,4 +228,90 @@ describe("an image showing another image's picture", () => { expect(copy.deleted).toBe(false); expect(copy.payload.data.assetId).toBe("original"); }); + + test("is sent to builds that keep picture ids with it, and to others without", async () => { + const { owner } = await seed(); + const copyData = async (acceptsPictureIds?: boolean) => { + const snapshot = (await owner.query(getPageSnapshot, { + ...protocol, + strategyPublicId: strategy, + pagePublicId: page, + ...(acceptsPictureIds === undefined ? {} : { acceptsPictureIds }), + })) as { elements: Row[] }; + return snapshot.elements.find((row) => row.publicId === "copy")!.payload + .data; + }; + + expect(await copyData(true)).toEqual({ + id: "copy", + assetId: "original", + scale: 1, + }); + // An older build holds the copy as it would write it, so it reads as + // unchanged; it finds the picture under the copy's id. + expect(await copyData()).toEqual({ id: "copy", scale: 1 }); + }); + + test("gives its picture's address to builds that ask by its own id", async () => { + const { owner } = await seed(); + + for (const assetPublicId of ["original", "copy"]) { + expect( + await owner.query(getAssetUrl, { + strategyPublicId: strategy, + assetPublicId, + }), + ).toEqual({ url: pictureUrl }); + } + }); +}); + +test("a picture from before upload statuses is found beside copies in other strategies", async () => { + const { t, owner } = await seed(); + const otherStrategy = "picture-other"; + await owner.mutation(createStrategy, { + ...protocol, + publicId: otherStrategy, + name: "Other", + mapData: "ascent", + initialPagePublicId: "other-page", + initialPageName: "Page 1", + initialPageIsAttack: true, + }); + const found = await t.run(async (ctx) => { + const strategyId = async (publicId: string) => + (await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", publicId)) + .unique())!._id; + const mine = await strategyId(strategy); + const other = await strategyId(otherStrategy); + const now = Date.now(); + // This strategy's picture, stored in Convex before upload statuses. + const storageId = await ctx.storage.store(new Blob(["picture"])); + await ctx.db.insert("imageAssets", { + publicId: "legacy-picture", + strategyId: mine, + storageId, + fileExtension: ".png", + createdAt: now, + updatedAt: now, + }); + // Twenty newer rows of the same picture, copied into another strategy. + for (let index = 0; index < 20; index += 1) { + await ctx.db.insert("imageAssets", { + publicId: "legacy-picture", + strategyId: other, + storageId, + uploadStatus: "deleted", + fileExtension: ".png", + createdAt: now + index + 1, + updatedAt: now + index + 1, + }); + } + const found = await getActiveAssetForStrategy(ctx, mine, "legacy-picture"); + return found?.strategyId === mine && found.uploadStatus === undefined; + }); + + expect(found).toBe(true); }); diff --git a/convex/strategy.ts b/convex/strategy.ts index 89e3b3d3..abf62504 100644 --- a/convex/strategy.ts +++ b/convex/strategy.ts @@ -82,6 +82,11 @@ export const getFullSnapshot = query({ // the trash's pages out cannot make them drop one. Older clients are // refused while the strategy holds trashed pages (see below). acceptsTrashedPagesLeftOut: v.optional(v.boolean()), + // Set by clients that keep an image's picture id (assetId, see + // collectAssetIdFromElementPayload). Older clients get image payloads + // without it, as they would write them, and find pictures under each + // image's own id (withPictureAliases). + acceptsPictureIds: v.optional(v.boolean()), }, returns: fullStrategySnapshotValidator, handler: async (ctx, args) => { @@ -147,6 +152,7 @@ export const getFullSnapshot = query({ strategy.publicId, pagePublicIds.get(element.pageId)!, element, + args.acceptsPictureIds === true, ), ), lineups: visibleLineups diff --git a/lib/collab/generated/convex_models.dart b/lib/collab/generated/convex_models.dart index 8d05c6ab..345e9413 100644 --- a/lib/collab/generated/convex_models.dart +++ b/lib/collab/generated/convex_models.dart @@ -5635,6 +5635,7 @@ List decodeLineupsListForStrategyResult( .toList(growable: false); ConvexObject encodeOpsApplyBatchArgs({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional accountSubject = const ConvexOptional.absent(), ConvexOptional checkLineupEndDeletes = const ConvexOptional.absent(), ConvexOptional checkLineupLinkEnds = const ConvexOptional.absent(), @@ -5644,6 +5645,8 @@ ConvexObject encodeOpsApplyBatchArgs({ required List ops, required String strategyPublicId, }) => ConvexObject({ + if (acceptsPictureIds.isPresent) + 'acceptsPictureIds': ConvexBoolean(acceptsPictureIds.value), if (accountSubject.isPresent) 'accountSubject': ConvexString(accountSubject.value), if (checkLineupEndDeletes.isPresent) @@ -5673,11 +5676,14 @@ OpsApplyBatchResult decodeOpsApplyBatchResult(ConvexValue value) => OpsApplyBatchResult.decode(value, 'ops.js:applyBatch.returns'); ConvexObject encodePageGetSnapshotArgs({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), required double clientProtocolVersion, required String pagePublicId, ConvexOptional shareToken = const ConvexOptional.absent(), required String strategyPublicId, }) => ConvexObject({ + if (acceptsPictureIds.isPresent) + 'acceptsPictureIds': ConvexBoolean(acceptsPictureIds.value), 'clientProtocolVersion': _encodeNumber( clientProtocolVersion, 'page.js:getSnapshot.args.clientProtocolVersion', @@ -6173,12 +6179,15 @@ ConvexValue decodeStrategiesUpdateResult(ConvexValue value) => _decodeRaw(value, 'strategies.js:update.returns', _validatePagesAddResult); ConvexObject encodeStrategyGetFullSnapshotArgs({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional acceptsTrashedPagesLeftOut = const ConvexOptional.absent(), required double clientProtocolVersion, ConvexOptional shareToken = const ConvexOptional.absent(), required String strategyPublicId, }) => ConvexObject({ + if (acceptsPictureIds.isPresent) + 'acceptsPictureIds': ConvexBoolean(acceptsPictureIds.value), if (acceptsTrashedPagesLeftOut.isPresent) 'acceptsTrashedPagesLeftOut': ConvexBoolean( acceptsTrashedPagesLeftOut.value, diff --git a/lib/collab/generated/icarus_convex_api.dart b/lib/collab/generated/icarus_convex_api.dart index a4facdd0..72fb03fe 100644 --- a/lib/collab/generated/icarus_convex_api.dart +++ b/lib/collab/generated/icarus_convex_api.dart @@ -725,6 +725,7 @@ final class _LineupsModule implements LineupsModule { abstract interface class OpsModule { Future applyBatch({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional accountSubject = const ConvexOptional.absent(), ConvexOptional checkLineupEndDeletes = const ConvexOptional.absent(), ConvexOptional checkLineupLinkEnds = const ConvexOptional.absent(), @@ -742,6 +743,7 @@ final class _OpsModule implements OpsModule { final ConvexTransport _transport; @override Future applyBatch({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional accountSubject = const ConvexOptional.absent(), ConvexOptional checkLineupEndDeletes = const ConvexOptional.absent(), ConvexOptional checkLineupLinkEnds = const ConvexOptional.absent(), @@ -753,6 +755,7 @@ final class _OpsModule implements OpsModule { required String strategyPublicId, }) { final args = encodeOpsApplyBatchArgs( + acceptsPictureIds: acceptsPictureIds, accountSubject: accountSubject, checkLineupEndDeletes: checkLineupEndDeletes, checkLineupLinkEnds: checkLineupLinkEnds, @@ -771,6 +774,7 @@ final class _OpsModule implements OpsModule { abstract interface class PageModule { ConvexQuery getSnapshot({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), required double clientProtocolVersion, required String pagePublicId, ConvexOptional shareToken = const ConvexOptional.absent(), @@ -783,12 +787,14 @@ final class _PageModule implements PageModule { final ConvexTransport _transport; @override ConvexQuery getSnapshot({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), required double clientProtocolVersion, required String pagePublicId, ConvexOptional shareToken = const ConvexOptional.absent(), required String strategyPublicId, }) { final args = encodePageGetSnapshotArgs( + acceptsPictureIds: acceptsPictureIds, clientProtocolVersion: clientProtocolVersion, pagePublicId: pagePublicId, shareToken: shareToken, @@ -1417,6 +1423,7 @@ final class _StrategiesModule implements StrategiesModule { abstract interface class StrategyModule { ConvexQuery getFullSnapshot({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional acceptsTrashedPagesLeftOut = const ConvexOptional.absent(), required double clientProtocolVersion, @@ -1435,6 +1442,7 @@ final class _StrategyModule implements StrategyModule { final ConvexTransport _transport; @override ConvexQuery getFullSnapshot({ + ConvexOptional acceptsPictureIds = const ConvexOptional.absent(), ConvexOptional acceptsTrashedPagesLeftOut = const ConvexOptional.absent(), required double clientProtocolVersion, @@ -1442,6 +1450,7 @@ final class _StrategyModule implements StrategyModule { required String strategyPublicId, }) { final args = encodeStrategyGetFullSnapshotArgs( + acceptsPictureIds: acceptsPictureIds, acceptsTrashedPagesLeftOut: acceptsTrashedPagesLeftOut, clientProtocolVersion: clientProtocolVersion, shareToken: shareToken, From 70668dc4b41590c927c51c573826f807e6ce1224 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:43:14 -0400 Subject: [PATCH 3/4] Read ten legacy rows per owner, so a picture lookup stays at 22 rows Co-Authored-By: Claude Opus 5.5 --- convex/lib/imageAssets.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/convex/lib/imageAssets.ts b/convex/lib/imageAssets.ts index 4ca44222..0615517e 100644 --- a/convex/lib/imageAssets.ts +++ b/convex/lib/imageAssets.ts @@ -188,7 +188,8 @@ export async function getActiveAssetForStrategy( // Rows from before upload statuses, this strategy's then those of no // strategy. Each is read on its own: copies into other strategies keep // their pictures' ids, so a read of every strategy's rows could fill its - // limit with theirs. + // limit with theirs. Ten each keeps the whole lookup to 22 rows, as a + // copy's budget counts it (convex/lib/contentCopy.ts). for (const owner of [strategyId, undefined]) { const legacyCandidates = await ctx.db .query("imageAssets") @@ -196,7 +197,7 @@ export async function getActiveAssetForStrategy( q.eq("strategyId", owner).eq("publicId", assetPublicId), ) .order("desc") - .take(20); + .take(10); const visible = legacyCandidates.find(isVisibleAsset); if (visible !== undefined) return visible; } From 01df59e2c0c557ca4f7c51b6734d4fd568a3d54d Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:59:14 -0400 Subject: [PATCH 4/4] Read legacy pictures by status, so failed attempts can't crowd them out Co-Authored-By: Claude Opus 5.5 --- convex/lib/imageAssets.ts | 32 +++++++++++++++++++++----------- convex/pictureId.test.ts | 14 +++++++++++++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/convex/lib/imageAssets.ts b/convex/lib/imageAssets.ts index 0615517e..3d7a4cf7 100644 --- a/convex/lib/imageAssets.ts +++ b/convex/lib/imageAssets.ts @@ -185,20 +185,30 @@ export async function getActiveAssetForStrategy( return strategyAsset; } - // Rows from before upload statuses, this strategy's then those of no - // strategy. Each is read on its own: copies into other strategies keep - // their pictures' ids, so a read of every strategy's rows could fill its - // limit with theirs. Ten each keeps the whole lookup to 22 rows, as a - // copy's budget counts it (convex/lib/contentCopy.ts). - for (const owner of [strategyId, undefined]) { - const legacyCandidates = await ctx.db + // The other rows that can still show the picture: this strategy's from + // before upload statuses, then rows of no strategy, active or from before + // statuses. Each set is read from its own index range, so neither copies + // in other strategies (which keep pictures' ids) nor failed upload + // attempts can crowd it out. Five each, with the active row and an upload + // placeholder, stays within the 22 rows a copy's budget charges for + // (convex/lib/contentCopy.ts). + const ranges = [ + [strategyId, undefined], + [undefined, "active"], + [undefined, undefined], + ] as const; + for (const [owner, uploadStatus] of ranges) { + const candidates = await ctx.db .query("imageAssets") - .withIndex("by_strategyId_and_publicId", (q) => - q.eq("strategyId", owner).eq("publicId", assetPublicId), + .withIndex("by_strategyId_and_publicId_and_uploadStatus", (q) => + q + .eq("strategyId", owner) + .eq("publicId", assetPublicId) + .eq("uploadStatus", uploadStatus), ) .order("desc") - .take(10); - const visible = legacyCandidates.find(isVisibleAsset); + .take(5); + const visible = candidates.find(isVisibleAsset); if (visible !== undefined) return visible; } return null; diff --git a/convex/pictureId.test.ts b/convex/pictureId.test.ts index f017df22..5ae8d363 100644 --- a/convex/pictureId.test.ts +++ b/convex/pictureId.test.ts @@ -266,7 +266,7 @@ describe("an image showing another image's picture", () => { }); }); -test("a picture from before upload statuses is found beside copies in other strategies", async () => { +test("a picture from before upload statuses is found beside copies elsewhere and failed attempts", async () => { const { t, owner } = await seed(); const otherStrategy = "picture-other"; await owner.mutation(createStrategy, { @@ -309,6 +309,18 @@ test("a picture from before upload statuses is found beside copies in other stra updatedAt: now + index + 1, }); } + // Ten newer failed attempts at the same picture in this strategy. + for (let index = 0; index < 10; index += 1) { + await ctx.db.insert("imageAssets", { + publicId: "legacy-picture", + provider: "r2", + strategyId: mine, + uploadStatus: "failed", + fileExtension: ".png", + createdAt: now + 100 + index, + updatedAt: now + 100 + index, + }); + } const found = await getActiveAssetForStrategy(ctx, mine, "legacy-picture"); return found?.strategyId === mine && found.uploadStatus === undefined; });