From af3b592defd036f1b006fbe167a9b1c57cde56f7 Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Thu, 30 Jul 2026 13:23:17 +0200 Subject: [PATCH 1/6] fix(assets): honour the asset model's cover image on its assets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AssetModel.image` / `AssetModel.imageExpiration` shipped as columns with the asset-models feature but nothing ever read or wrote them: the settings form had no image field, and an asset linked to a model still rendered the grey placeholder. Users had to pick and upload the same photo once per asset. Upload once on the model; every asset of that model that has no image of its own points at that single storage object. - `updateAssetModelImage` mirrors `updateKitImage` (multipart parse -> resize -> 108px thumbnail -> sign -> persist) and fans the result out to inheriting assets. `refreshExpiredAssetModelImages` mirrors `refreshExpiredKitImages`. - Model images live in the `assets` bucket on purpose. Inheriting assets store the model's URL in `Asset.mainImage`, and the existing re-sign / thumbnail paths resolve it with `extractStoragePath(url, "assets")` — a separate bucket would break every inherited image after 72h. - Inheritance is applied where the link is made (`createAsset`, `updateAsset`) and re-applied when the model's image changes, so every surface that already reads `Asset.mainImage` — asset index, detail, booking rows, scanner, reports, PDFs, the companion app — shows it with no change to those call sites. - Ownership is decided by storage path, not URL string: signed URLs get re-signed per asset over time, so an exact-URL match would lose track of the inheritance. An image the user uploaded for a specific asset always wins. - Unlinking or deleting a model clears the images its assets inherited, so no asset is left showing the photo of a model it no longer belongs to. - Extracted `getThumbnailStoragePath` so the upload path and the lazy `generate-thumbnail` route derive the same path from one helper. Storage holds one image plus one thumbnail per model, no matter how many assets share it. No migration — both columns already exist. Reported in support chat: "Would there be a way to have an asset's model determine what image is used? It would help reduce having to upload and store the same image over and over again." --- .../app/components/asset-model/form.tsx | 64 ++- .../asset-model/service.server.test.ts | 361 +++++++++++- .../app/modules/asset-model/service.server.ts | 534 +++++++++++++++++- .../app/modules/asset/service.server.test.ts | 179 ++++++ .../app/modules/asset/service.server.ts | 79 ++- ...tings.asset-models.$assetModelId_.edit.tsx | 32 +- .../_layout+/settings.asset-models.index.tsx | 35 +- .../_layout+/settings.asset-models.new.tsx | 27 +- .../routes/api+/asset.generate-thumbnail.ts | 19 +- apps/webapp/app/utils/storage.server.ts | 31 +- 10 files changed, 1323 insertions(+), 38 deletions(-) diff --git a/apps/webapp/app/components/asset-model/form.tsx b/apps/webapp/app/components/asset-model/form.tsx index f40c9f7aac..e2dd153530 100644 --- a/apps/webapp/app/components/asset-model/form.tsx +++ b/apps/webapp/app/components/asset-model/form.tsx @@ -13,13 +13,16 @@ */ import { useEffect } from "react"; import type { AssetModel } from "@prisma/client"; +import { useAtom, useAtomValue } from "jotai"; import { useActionData, useLoaderData } from "react-router"; import { useZorm } from "react-zorm"; import z from "zod"; +import { assetImageValidateFileAtom, fileErrorAtom } from "~/atoms/file"; import { useAutoFocus } from "~/hooks/use-auto-focus"; import { useDisabled } from "~/hooks/use-disabled"; import useFetcherWithReset from "~/hooks/use-fetcher-with-reset"; import type { action } from "~/routes/_layout+/settings.asset-models.new"; +import { ACCEPT_SUPPORTED_IMAGES } from "~/utils/constants"; import { getValidationErrors } from "~/utils/http"; import type { DataOrErrorResponse } from "~/utils/http.server"; import { zodFieldIsRequired } from "~/utils/zod"; @@ -27,6 +30,7 @@ import { Form } from "../custom-form"; import DynamicSelect from "../dynamic-select/dynamic-select"; import FormRow from "../forms/form-row"; import Input from "../forms/input"; +import ImageWithPreview from "../image-with-preview/image-with-preview"; import { Button } from "../shared/button"; import { Card } from "../shared/card"; @@ -47,7 +51,7 @@ type AssetModelFormProps = { /** Pre-filled values for edit mode */ assetModel?: Pick< AssetModel, - "name" | "description" | "defaultCategoryId" | "defaultValuation" + "name" | "description" | "defaultCategoryId" | "defaultValuation" | "image" >; /** The API URL to submit the form to (used in inline/dialog mode). */ apiUrl?: string; @@ -213,9 +217,29 @@ function FullPageForm({ actionData?.error ); + // Client-side file guard (type + 8MB cap) shared with the asset/kit image + // inputs, so all three surfaces reject the same files with the same copy. + const [, validateFile] = useAtom(assetImageValidateFileAtom); + const fileError = useAtomValue(fileErrorAtom); + + /** + * The image upload has no zod field (a File can't be parsed by the text + * schema), so its errors arrive either as the client-side file guard's + * message or as the server's `field: "image"` ShelfError. + */ + const imageError = + (actionData?.error?.additionalData?.field === "image" + ? actionData?.error?.message + : undefined) ?? fileError; + return ( -
+ {/* -- Top action bar (visible on md+) -- */}
@@ -323,6 +347,42 @@ function FullPageForm({
+ {/* -- Image -- */} + +
+ {assetModel?.image ? ( + + ) : null} +

+ Accepts PNG, JPG, JPEG, or WebP (max.8 MB) +

+ +

+ Accepts PNG, JPG, JPEG, or WebP (max.8 MB) +

+
+ + {/* -- Bottom action bar -- */}
diff --git a/apps/webapp/app/modules/asset-model/service.server.test.ts b/apps/webapp/app/modules/asset-model/service.server.test.ts index ab214c82f8..4ee980e738 100644 --- a/apps/webapp/app/modules/asset-model/service.server.test.ts +++ b/apps/webapp/app/modules/asset-model/service.server.test.ts @@ -3,10 +3,14 @@ import { createAssetModel as createAssetModelFactory } from "@factories"; import { db } from "~/database/db.server"; import { ShelfError } from "~/utils/error"; import { + clearInheritedAssetModelImages, createAssetModel, createAssetModelsIfNotExists, getAssetModels, getAssetModel, + getInheritableAssetModelImage, + isAssetModelImageUrl, + propagateAssetModelImageToAssets, updateAssetModel, deleteAssetModel, bulkDeleteAssetModels, @@ -24,9 +28,30 @@ vitest.mock("~/database/db.server", () => ({ deleteMany: vitest.fn(), count: vitest.fn(), }, + asset: { + findMany: vitest.fn(), + updateMany: vitest.fn(), + }, }, })); +// why: Supabase storage is a network boundary — stub the signing call so the +// thumbnail derivation can be asserted without hitting it. getThumbnailStoragePath +// is pure, so it keeps its real implementation. +vitest.mock("~/utils/storage.server", async () => { + const actual = await vitest.importActual>( + "~/utils/storage.server" + ); + return { + ...actual, + createSignedUrl: vitest.fn( + ({ filename }: { filename: string }) => + `https://xyz.supabase.co/storage/v1/object/sign/assets/${filename}?token=signed` + ), + parseFileFormData: vitest.fn(), + }; +}); + describe("createAssetModel", () => { beforeEach(() => { vitest.clearAllMocks(); @@ -314,6 +339,8 @@ describe("updateAssetModel", () => { describe("deleteAssetModel", () => { beforeEach(() => { vitest.clearAllMocks(); + // @ts-expect-error mock setup — no inheriting assets unless a test says so + db.asset.findMany.mockResolvedValue([]); }); it("deletes an asset model scoped to organization", async () => { @@ -443,9 +470,16 @@ describe("createAssetModelsIfNotExists", () => { describe("bulkDeleteAssetModels", () => { beforeEach(() => { vitest.clearAllMocks(); + // @ts-expect-error mock setup — no inheriting assets unless a test says so + db.asset.findMany.mockResolvedValue([]); }); it("deletes specific asset models by IDs", async () => { + // @ts-expect-error mock setup + db.assetModel.findMany.mockResolvedValue([ + { id: "model-1" }, + { id: "model-2" }, + ]); // @ts-expect-error mock setup db.assetModel.deleteMany.mockResolvedValue({ count: 2 }); @@ -454,6 +488,13 @@ describe("bulkDeleteAssetModels", () => { organizationId: "org-123", }); + expect(db.assetModel.findMany).toHaveBeenCalledWith({ + where: { + id: { in: ["model-1", "model-2"] }, + organizationId: "org-123", + }, + select: { id: true }, + }); expect(db.assetModel.deleteMany).toHaveBeenCalledWith({ where: { id: { in: ["model-1", "model-2"] }, @@ -464,15 +505,331 @@ describe("bulkDeleteAssetModels", () => { it("deletes all asset models when ALL_SELECTED key is present", async () => { // @ts-expect-error mock setup - db.assetModel.deleteMany.mockResolvedValue({ count: 5 }); + db.assetModel.findMany.mockResolvedValue([ + { id: "model-1" }, + { id: "model-2" }, + { id: "model-3" }, + ]); + // @ts-expect-error mock setup + db.assetModel.deleteMany.mockResolvedValue({ count: 3 }); await bulkDeleteAssetModels({ assetModelIds: ["all-selected"], organizationId: "org-123", }); - expect(db.assetModel.deleteMany).toHaveBeenCalledWith({ + // The id set is resolved from the org-wide filter first, so the + // inherited-image cleanup and the delete cover exactly the same models. + expect(db.assetModel.findMany).toHaveBeenCalledWith({ where: { organizationId: "org-123" }, + select: { id: true }, }); + expect(db.assetModel.deleteMany).toHaveBeenCalledWith({ + where: { + id: { in: ["model-1", "model-2", "model-3"] }, + organizationId: "org-123", + }, + }); + }); + + it("clears inherited images from the deleted models' assets before deleting", async () => { + // @ts-expect-error mock setup + db.assetModel.findMany.mockResolvedValue([{ id: "model-1" }]); + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { + id: "asset-inheriting", + mainImage: MODEL_IMAGE_URL, + assetModelId: "model-1", + }, + { + id: "asset-own-image", + mainImage: OWN_ASSET_IMAGE_URL, + assetModelId: "model-1", + }, + ]); + // @ts-expect-error mock setup + db.asset.updateMany.mockResolvedValue({ count: 1 }); + // @ts-expect-error mock setup + db.assetModel.deleteMany.mockResolvedValue({ count: 1 }); + + await bulkDeleteAssetModels({ + assetModelIds: ["model-1"], + organizationId: "org-123", + }); + + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["asset-inheriting"] }, organizationId: "org-123" }, + data: { + mainImage: null, + mainImageExpiration: null, + thumbnailImage: null, + }, + }); + // why: after the delete, ON DELETE SET NULL has erased the link that + // identifies inheriting assets — the cleanup must run first. + expect( + vitest.mocked(db.asset.updateMany).mock.invocationCallOrder[0] + ).toBeLessThan( + vitest.mocked(db.assetModel.deleteMany).mock.invocationCallOrder[0] + ); + }); +}); + +describe("clearInheritedAssetModelImages", () => { + beforeEach(() => { + vitest.clearAllMocks(); + }); + + it("clears only the assets that were showing their model's image", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { + id: "asset-inheriting", + mainImage: MODEL_IMAGE_URL, + assetModelId: "model-1", + }, + { + id: "asset-own-image", + mainImage: OWN_ASSET_IMAGE_URL, + assetModelId: "model-1", + }, + { id: "asset-no-image", mainImage: null, assetModelId: "model-1" }, + ]); + // @ts-expect-error mock setup + db.asset.updateMany.mockResolvedValue({ count: 1 }); + + const count = await clearInheritedAssetModelImages({ + assetModelIds: ["model-1"], + organizationId: "org-123", + }); + + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["asset-inheriting"] }, organizationId: "org-123" }, + data: { + mainImage: null, + mainImageExpiration: null, + thumbnailImage: null, + }, + }); + expect(count).toBe(1); + }); + + it("writes nothing when no asset was inheriting", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { + id: "asset-own-image", + mainImage: OWN_ASSET_IMAGE_URL, + assetModelId: "model-1", + }, + ]); + + const count = await clearInheritedAssetModelImages({ + assetModelIds: ["model-1"], + organizationId: "org-123", + }); + + expect(db.asset.updateMany).not.toHaveBeenCalled(); + expect(count).toBe(0); + }); + + it("short-circuits on an empty model list", async () => { + const count = await clearInheritedAssetModelImages({ + assetModelIds: [], + organizationId: "org-123", + }); + + expect(db.asset.findMany).not.toHaveBeenCalled(); + expect(count).toBe(0); + }); +}); + +/* ====================================================================== */ +/* Cover image */ +/* ====================================================================== */ + +/** A signed URL for an image stored under model `model-1`'s folder. */ +const MODEL_IMAGE_URL = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-123/asset-models/model-1/image-1700000000000.png?token=abc"; +/** A re-signed URL for the SAME object (different token — refresh happened). */ +const MODEL_IMAGE_URL_RESIGNED = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-123/asset-models/model-1/image-1700000000000.png?token=zzz"; +/** The shared 108px thumbnail sitting next to that model image. */ +const MODEL_THUMBNAIL_URL = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-123/asset-models/model-1/image-1700000000000-thumbnail.png?token=abc"; +/** An image the user uploaded for one specific asset. */ +const OWN_ASSET_IMAGE_URL = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-123/asset-abc/main-image-1700000000000?token=abc"; + +describe("isAssetModelImageUrl", () => { + it("recognises an asset-model image by its storage path", () => { + expect(isAssetModelImageUrl(MODEL_IMAGE_URL)).toBe(true); + expect(isAssetModelImageUrl(MODEL_IMAGE_URL, "model-1")).toBe(true); + }); + + it("still recognises the image after the signed URL was re-signed", () => { + // why: refreshExpiredAssetImages rewrites the token on the asset row, so an + // exact-URL comparison would lose track of the inheritance. The path is + // the stable part. + expect(isAssetModelImageUrl(MODEL_IMAGE_URL_RESIGNED, "model-1")).toBe( + true + ); + }); + + it("does not claim another model's image", () => { + expect(isAssetModelImageUrl(MODEL_IMAGE_URL, "model-2")).toBe(false); + }); + + it("does not claim an image the user uploaded for the asset itself", () => { + expect(isAssetModelImageUrl(OWN_ASSET_IMAGE_URL)).toBe(false); + expect(isAssetModelImageUrl(OWN_ASSET_IMAGE_URL, "model-1")).toBe(false); + }); + + it("treats a missing image as not inherited", () => { + expect(isAssetModelImageUrl(null)).toBe(false); + expect(isAssetModelImageUrl(undefined)).toBe(false); + expect(isAssetModelImageUrl("")).toBe(false); + }); +}); + +describe("getInheritableAssetModelImage", () => { + beforeEach(() => { + vitest.clearAllMocks(); + }); + + it("returns the model's image plus its derived shared thumbnail", async () => { + const imageExpiration = new Date("2026-08-01T00:00:00.000Z"); + // @ts-expect-error mock setup + db.assetModel.findFirst.mockResolvedValue({ + image: MODEL_IMAGE_URL, + imageExpiration, + }); + + const result = await getInheritableAssetModelImage({ + assetModelId: "model-1", + organizationId: "org-123", + }); + + expect(db.assetModel.findFirst).toHaveBeenCalledWith({ + where: { id: "model-1", organizationId: "org-123" }, + select: { image: true, imageExpiration: true }, + }); + expect(result).toEqual({ + image: MODEL_IMAGE_URL, + imageExpiration, + // Derived from the image path by the same rule the upload wrote it with, + // so a fresh asset renders at list size without a generate-thumbnail hop. + thumbnailImage: + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-123/asset-models/model-1/image-1700000000000-thumbnail.png?token=signed", + }); + }); + + it("returns null when the model has no image", async () => { + // @ts-expect-error mock setup + db.assetModel.findFirst.mockResolvedValue({ + image: null, + imageExpiration: null, + }); + + await expect( + getInheritableAssetModelImage({ + assetModelId: "model-1", + organizationId: "org-123", + }) + ).resolves.toBeNull(); + }); + + it("returns null for a model outside the organization", async () => { + // @ts-expect-error mock setup + db.assetModel.findFirst.mockResolvedValue(null); + + await expect( + getInheritableAssetModelImage({ + assetModelId: "foreign-model", + organizationId: "org-123", + }) + ).resolves.toBeNull(); + }); +}); + +describe("propagateAssetModelImageToAssets", () => { + const imageExpiration = new Date("2026-08-01T00:00:00.000Z"); + + beforeEach(() => { + vitest.clearAllMocks(); + }); + + it("re-stamps assets with no image and assets already inheriting, but never an asset with its own image", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-no-image", mainImage: null }, + { id: "asset-inheriting", mainImage: MODEL_IMAGE_URL_RESIGNED }, + { id: "asset-own-image", mainImage: OWN_ASSET_IMAGE_URL }, + ]); + // @ts-expect-error mock setup + db.asset.updateMany.mockResolvedValue({ count: 2 }); + + const count = await propagateAssetModelImageToAssets({ + assetModelId: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + imageExpiration, + thumbnailImage: MODEL_THUMBNAIL_URL, + }); + + expect(db.asset.findMany).toHaveBeenCalledWith({ + where: { assetModelId: "model-1", organizationId: "org-123" }, + select: { id: true, mainImage: true }, + }); + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { + id: { in: ["asset-no-image", "asset-inheriting"] }, + organizationId: "org-123", + }, + data: { + mainImage: MODEL_IMAGE_URL, + mainImageExpiration: imageExpiration, + // why: the shared thumbnail is stamped too, so N inheriting assets + // don't each fire /api/asset/generate-thumbnail to build the one + // object that already exists. + thumbnailImage: MODEL_THUMBNAIL_URL, + }, + }); + expect(count).toBe(2); + }); + + it("writes nothing when every asset has its own image", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-own-image", mainImage: OWN_ASSET_IMAGE_URL }, + ]); + + const count = await propagateAssetModelImageToAssets({ + assetModelId: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + imageExpiration, + thumbnailImage: MODEL_THUMBNAIL_URL, + }); + + expect(db.asset.updateMany).not.toHaveBeenCalled(); + expect(count).toBe(0); + }); + + it("writes nothing when the model has no assets", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([]); + + const count = await propagateAssetModelImageToAssets({ + assetModelId: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + imageExpiration, + thumbnailImage: MODEL_THUMBNAIL_URL, + }); + + expect(db.asset.updateMany).not.toHaveBeenCalled(); + expect(count).toBe(0); }); }); diff --git a/apps/webapp/app/modules/asset-model/service.server.ts b/apps/webapp/app/modules/asset-model/service.server.ts index 034d428a56..3695ed261d 100644 --- a/apps/webapp/app/modules/asset-model/service.server.ts +++ b/apps/webapp/app/modules/asset-model/service.server.ts @@ -1,12 +1,83 @@ import type { AssetModel, Organization, Prisma, User } from "@prisma/client"; +import { extractStoragePath } from "~/components/assets/asset-image/utils"; import { db } from "~/database/db.server"; +import { ASSET_MAX_IMAGE_UPLOAD_SIZE } from "~/utils/constants"; +import { dateTimeInUnix } from "~/utils/date-time-in-unix"; import type { ErrorLabel } from "~/utils/error"; -import { ShelfError, maybeUniqueConstraintViolation } from "~/utils/error"; +import { + ShelfError, + isLikeShelfError, + maybeUniqueConstraintViolation, +} from "~/utils/error"; import { ALL_SELECTED_KEY } from "~/utils/list"; +import { Logger } from "~/utils/logger"; +import { threeDaysFromNow } from "~/utils/one-week-from-now"; +import { + createSignedUrl, + getThumbnailStoragePath, + parseFileFormData, +} from "~/utils/storage.server"; import type { CreateAssetFromContentImportPayload } from "../asset/types"; const label: ErrorLabel = "Asset Model"; +/** + * Storage bucket that holds asset-model cover images. + * + * Deliberately the SAME bucket assets use (not a dedicated `asset-models` + * bucket): an asset that inherits its model's cover image stores the model's + * URL in `Asset.mainImage`, and every existing re-sign / thumbnail path + * (`refreshExpiredAssetImages`, `api+/asset.refresh-main-image`, + * `api+/asset.generate-thumbnail`) resolves that URL with + * `extractStoragePath(url, "assets")`. A separate bucket would make those + * paths unresolvable and every inherited image would break after 72h. + */ +const ASSET_MODEL_IMAGE_BUCKET = "assets"; + +/** + * Path segment that marks a storage object as belonging to an asset model + * rather than to a single asset. Asset-model images live at + * `/asset-models//image-`, while per-asset images + * live at `//main-image-` — so the segment is what + * tells "this asset is displaying its model's shared image" apart from "this + * asset has its own uploaded image". + */ +const ASSET_MODEL_IMAGE_PATH_SEGMENT = "asset-models"; + +/** + * Whether a stored image URL points at an asset model's shared cover image. + * + * Used as the ownership test before overwriting or clearing an asset's + * `mainImage`: an asset whose image lives under the model's storage folder is + * *inheriting* it and may be re-stamped, whereas an asset with its own + * uploaded image must never be touched. + * + * Matches on the storage PATH, not the URL string, because signed URLs are + * re-signed per asset over time (`refreshExpiredAssetImages` writes a fresh + * token onto the asset row) — the path is the only stable part. + * + * @param imageUrl - The asset's stored `mainImage` URL (or null) + * @param assetModelId - Optional: require the image to belong to THIS model + * @returns true when the URL resolves to an asset-model image path + */ +export function isAssetModelImageUrl( + imageUrl: string | null | undefined, + assetModelId?: AssetModel["id"] +): boolean { + if (!imageUrl) { + return false; + } + + const path = extractStoragePath(imageUrl, ASSET_MODEL_IMAGE_BUCKET); + if (!path) { + return false; + } + + return assetModelId + ? path.includes(`/${ASSET_MODEL_IMAGE_PATH_SEGMENT}/${assetModelId}/`) + : path.includes(`/${ASSET_MODEL_IMAGE_PATH_SEGMENT}/`); +} + /** * Creates a new asset model (template/grouping entity for assets). * Asset models provide default values when creating new assets from them. @@ -170,6 +241,436 @@ export async function updateAssetModel({ } } +/** The image fields an inheriting asset copies from its model. */ +type InheritableAssetModelImage = { + /** Signed URL of the model's full-size cover image */ + image: string; + /** When that signed URL expires */ + imageExpiration: Date | null; + /** Signed URL of the model's shared 108px thumbnail, when resolvable */ + thumbnailImage: string | null; +}; + +/** + * Reads the cover image an asset should inherit when it is linked to a model + * and has no image of its own. + * + * Narrow, org-scoped read (two columns) so the create/update asset paths don't + * pull a whole `AssetModel` row just to resolve an image. + * + * The thumbnail isn't a column on `AssetModel` — it doesn't need to be. Its + * storage path is derived from the image path by the same + * {@link getThumbnailStoragePath} rule the upload wrote it with, so it is + * re-signed on demand here. Stamping it means an inheriting asset renders + * immediately at list sizes instead of firing a `generate-thumbnail` request on + * first view. + * + * @param params.assetModelId - The model the asset is being linked to + * @param params.organizationId - Org scope; a foreign model resolves to null + * @returns The model's signed image fields, or null when it has no image + */ +export async function getInheritableAssetModelImage({ + assetModelId, + organizationId, +}: { + assetModelId: AssetModel["id"]; + organizationId: Organization["id"]; +}): Promise { + const assetModel = await db.assetModel.findFirst({ + where: { id: assetModelId, organizationId }, + select: { image: true, imageExpiration: true }, + }); + + if (!assetModel?.image) { + return null; + } + + return { + image: assetModel.image, + imageExpiration: assetModel.imageExpiration, + thumbnailImage: await signAssetModelThumbnail(assetModel.image), + }; +} + +/** + * Re-signs the shared thumbnail that sits next to a model's cover image. + * + * Degrades to null (never throws) — a missing thumbnail just means the client + * regenerates it lazily, which is the pre-existing behaviour for any asset + * without one. + * + * @param imageUrl - Signed URL of the model's full-size image + * @returns A signed thumbnail URL, or null when the path can't be resolved + */ +async function signAssetModelThumbnail( + imageUrl: string +): Promise { + const imagePath = extractStoragePath(imageUrl, ASSET_MODEL_IMAGE_BUCKET); + if (!imagePath) { + return null; + } + + try { + return await createSignedUrl({ + filename: getThumbnailStoragePath(imagePath), + bucketName: ASSET_MODEL_IMAGE_BUCKET, + }); + } catch { + Logger.info( + `Failed to sign the thumbnail for asset-model image ${imagePath}; the client will regenerate it` + ); + return null; + } +} + +/** + * Uploads (or replaces) an asset model's cover image and propagates it to the + * model's assets that don't have an image of their own. + * + * This is the write half of the "upload once, show on every asset of this + * model" contract the customer asked for: the file is stored ONCE, and each + * inheriting asset simply stores a signed URL pointing at that single object. + * + * Mirrors `updateKitImage` (`~/modules/kit/service.server`) — same multipart + * parse → resize → sign → persist shape — with two deliberate differences: + * the bucket is `assets` (see {@link ASSET_MODEL_IMAGE_BUCKET}) and the write + * fans out to inheriting assets via {@link propagateAssetModelImageToAssets}. + * + * No-ops when the submitted form carries no file, so a plain "Save" on the + * model form never clears an existing image. + * + * @param params.request - The raw (un-consumed) multipart request + * @param params.assetModelId - Model receiving the image + * @param params.userId - Uploading user; also the storage path prefix + * @param params.organizationId - Org scope for both the model and asset writes + * @returns The signed URL that was stored, or null when no file was submitted + * @throws {ShelfError} If parsing, uploading, signing or persisting fails + */ +export async function updateAssetModelImage({ + request, + assetModelId, + userId, + organizationId, +}: { + request: Request; + assetModelId: AssetModel["id"]; + userId: User["id"]; + organizationId: Organization["id"]; +}) { + try { + const fileData = await parseFileFormData({ + request, + bucketName: ASSET_MODEL_IMAGE_BUCKET, + newFileName: `${userId}/${ASSET_MODEL_IMAGE_PATH_SEGMENT}/${assetModelId}/image-${dateTimeInUnix( + Date.now() + )}`, + resizeOptions: { + width: 800, + withoutEnlargement: true, + }, + /** + * Generate the 108px thumbnail up front. Inheriting assets are stamped + * with it, so N assets sharing this model don't each fire + * `/api/asset/generate-thumbnail` on first render to produce the one + * shared object. + */ + generateThumbnail: true, + thumbnailSize: 108, + maxFileSize: ASSET_MAX_IMAGE_UPLOAD_SIZE, + }); + + const uploaded = fileData.get("image") as string; + /** No file submitted — leave any existing image untouched. */ + if (!uploaded) { + return null; + } + + /** + * With `generateThumbnail`, the parser returns a JSON blob carrying both + * paths; without it, a bare path string. Handle both, same as + * `updateAssetMainImage`. + */ + const { imagePath, thumbnailPath } = parseUploadedImagePaths(uploaded); + + const [image, thumbnailImage] = await Promise.all([ + createSignedUrl({ + filename: imagePath, + bucketName: ASSET_MODEL_IMAGE_BUCKET, + }), + thumbnailPath + ? createSignedUrl({ + filename: thumbnailPath, + bucketName: ASSET_MODEL_IMAGE_BUCKET, + }).catch(() => null) + : Promise.resolve(null), + ]); + + /** + * `createSignedUrl` mints a 72h URL (see the invariant comment in + * `~/utils/storage.server`), and this expiration is copied onto every + * inheriting asset — a shorter value would make `refreshExpiredAssetImages` + * re-sign each of them days before the URL actually lapses. + */ + const imageExpiration = threeDaysFromNow(); + + await db.assetModel.update({ + where: { id: assetModelId, organizationId }, + data: { image, imageExpiration }, + }); + + await propagateAssetModelImageToAssets({ + assetModelId, + organizationId, + image, + imageExpiration, + thumbnailImage, + }); + + return image; + } catch (cause) { + throw new ShelfError({ + cause, + message: isLikeShelfError(cause) + ? cause.message + : "Something went wrong while updating the image for this asset model.", + additionalData: { assetModelId, userId, field: "image" }, + label, + }); + } +} + +/** + * Unwraps what `parseFileFormData` returned for the uploaded image. + * + * With `generateThumbnail: true` the value is a JSON blob carrying both paths; + * otherwise it is a bare storage path. Mirrors the unwrap in + * `updateAssetMainImage` (`~/modules/asset/service.server`). + * + * @param uploaded - The raw value read off the parsed form data + * @returns The full-size image path and, when present, its thumbnail path + */ +function parseUploadedImagePaths(uploaded: string): { + imagePath: string; + thumbnailPath: string | null; +} { + try { + const parsed = JSON.parse(uploaded) as { + originalPath?: string; + thumbnailPath?: string; + }; + + if (parsed.originalPath) { + return { + imagePath: parsed.originalPath, + thumbnailPath: parsed.thumbnailPath ?? null, + }; + } + } catch { + // Not JSON — the parser returned a bare path. + } + + return { imagePath: uploaded, thumbnailPath: null }; +} + +/** + * Points every "inheriting" asset of a model at the model's current image. + * + * An asset inherits when it has no image at all (`mainImage` is null) or when + * the image it shows is the model's own shared object (see + * {@link isAssetModelImageUrl}). Assets with their own uploaded image are left + * alone — an explicit per-asset upload always wins. + * + * All inheriting assets point at the SAME two storage objects (the image and + * its thumbnail), so storage holds one image + one thumbnail per model no + * matter how many assets share it. + * + * @param params.assetModelId - Model whose image changed + * @param params.organizationId - Org scope for the write + * @param params.image - Freshly-signed model image URL + * @param params.imageExpiration - Expiration of that signed URL + * @param params.thumbnailImage - Signed URL of the shared thumbnail, if any + * @returns Number of assets that were re-stamped + */ +export async function propagateAssetModelImageToAssets({ + assetModelId, + organizationId, + image, + imageExpiration, + thumbnailImage, +}: { + assetModelId: AssetModel["id"]; + organizationId: Organization["id"]; + image: string; + imageExpiration: Date; + thumbnailImage: string | null; +}) { + const candidates = await db.asset.findMany({ + where: { assetModelId, organizationId }, + select: { id: true, mainImage: true }, + }); + + const inheritingAssetIds = candidates + .filter( + (asset) => + asset.mainImage === null || + isAssetModelImageUrl(asset.mainImage, assetModelId) + ) + .map((asset) => asset.id); + + if (inheritingAssetIds.length === 0) { + return 0; + } + + const { count } = await db.asset.updateMany({ + where: { id: { in: inheritingAssetIds }, organizationId }, + data: { + mainImage: image, + mainImageExpiration: imageExpiration, + thumbnailImage, + }, + }); + + return count; +} + +/** + * Clears inherited cover images from the assets of the given models. + * + * Called just BEFORE a model is deleted, while its rows still exist to identify + * — `Asset.assetModelId` is `ON DELETE SET NULL`, so after the delete there is + * no way to tell which assets were showing that model's photo. Without this an + * asset would keep displaying the picture of a model that no longer exists: + * exactly the two-signals-to-reconcile state the unlink path already avoids. + * + * Assets with their own uploaded image are untouched. + * + * @param params.assetModelIds - Models about to be deleted + * @param params.organizationId - Org scope for the write + * @returns Number of assets whose inherited image was cleared + */ +export async function clearInheritedAssetModelImages({ + assetModelIds, + organizationId, +}: { + assetModelIds: AssetModel["id"][]; + organizationId: Organization["id"]; +}) { + if (assetModelIds.length === 0) { + return 0; + } + + const candidates = await db.asset.findMany({ + where: { assetModelId: { in: assetModelIds }, organizationId }, + select: { id: true, mainImage: true, assetModelId: true }, + }); + + const inheritingAssetIds = candidates + .filter((asset) => + isAssetModelImageUrl(asset.mainImage, asset.assetModelId ?? undefined) + ) + .map((asset) => asset.id); + + if (inheritingAssetIds.length === 0) { + return 0; + } + + const { count } = await db.asset.updateMany({ + where: { id: { in: inheritingAssetIds }, organizationId }, + data: { + mainImage: null, + mainImageExpiration: null, + thumbnailImage: null, + }, + }); + + return count; +} + +/** + * Re-signs expired Supabase signed image URLs for a set of asset models, in + * place. + * + * Mirrors `refreshExpiredKitImages` (`~/modules/kit/service.server`): the + * settings list and edit form need a URL that still resolves, and signed URLs + * live for 72h. Failures are logged and swallowed — a stale URL degrades to + * the client-side broken-image fallback rather than failing the page. + * + * @param assetModels - Rows carrying `id`, `organizationId`, `image` and + * `imageExpiration` + * @returns The same array with fresh `image`/`imageExpiration` where refreshed + */ +export async function refreshExpiredAssetModelImages< + T extends { + id: string; + organizationId: string; + image: string | null; + imageExpiration: Date | null; + }, +>(assetModels: T[]): Promise { + const now = new Date(); + const expired = assetModels.filter( + (model) => + model.image && + model.imageExpiration && + new Date(model.imageExpiration) < now + ); + + if (expired.length === 0) { + return assetModels; + } + + const results = await Promise.allSettled( + expired.map(async (model) => { + const imagePath = extractStoragePath( + model.image!, + ASSET_MODEL_IMAGE_BUCKET + ); + if (!imagePath) { + return null; + } + + const image = await createSignedUrl({ + filename: imagePath, + bucketName: ASSET_MODEL_IMAGE_BUCKET, + }); + const imageExpiration = threeDaysFromNow(); + + await db.assetModel.update({ + where: { id: model.id, organizationId: model.organizationId }, + data: { image, imageExpiration }, + }); + + return { id: model.id, image, imageExpiration }; + }) + ); + + const refreshed = new Map(); + results.forEach((result, index) => { + if (result.status === "fulfilled" && result.value) { + refreshed.set(result.value.id, { + image: result.value.image, + imageExpiration: result.value.imageExpiration, + }); + return; + } + + if (result.status === "rejected") { + Logger.info( + `Failed to refresh image for asset model ${expired[index].id}, proceeding with stale URL` + ); + } + }); + + if (refreshed.size === 0) { + return assetModels; + } + + return assetModels.map((model) => { + const fresh = refreshed.get(model.id); + return fresh ? { ...model, ...fresh } : model; + }); +} + /** * Deletes an asset model by ID, scoped to the given organization. * Assets referencing this model will have their assetModelId set to null. @@ -179,6 +680,15 @@ export async function deleteAssetModel({ organizationId, }: Pick & { organizationId: Organization["id"] }) { try { + /** + * Runs BEFORE the delete: once the model row is gone, `ON DELETE SET NULL` + * has erased the link that identifies which assets were inheriting from it. + */ + await clearInheritedAssetModelImages({ + assetModelIds: [id], + organizationId, + }); + const result = await db.assetModel.deleteMany({ where: { id, organizationId }, }); @@ -335,7 +845,27 @@ export async function bulkDeleteAssetModels({ where = { id: { in: assetModelIds }, organizationId }; } - return await db.assetModel.deleteMany({ where }); + /** + * Resolve the ids first so the inherited-image cleanup and the delete cover + * exactly the same set — `where` can be filter-driven ("select all" with an + * active search), and the cleanup needs the links intact to run. + */ + const matchingModels = await db.assetModel.findMany({ + where, + select: { id: true }, + }); + + await clearInheritedAssetModelImages({ + assetModelIds: matchingModels.map((model) => model.id), + organizationId, + }); + + return await db.assetModel.deleteMany({ + where: { + id: { in: matchingModels.map((model) => model.id) }, + organizationId, + }, + }); } catch (cause) { throw new ShelfError({ cause, diff --git a/apps/webapp/app/modules/asset/service.server.test.ts b/apps/webapp/app/modules/asset/service.server.test.ts index d95112219a..7cea396569 100644 --- a/apps/webapp/app/modules/asset/service.server.test.ts +++ b/apps/webapp/app/modules/asset/service.server.test.ts @@ -119,6 +119,17 @@ vitest.mock("~/database/db.server", () => ({ findUnique: vitest.fn().mockResolvedValue({ user: null }), findFirst: vitest.fn().mockResolvedValue({ user: null }), }, + // why: the model-image inheritance path reads the linked model's image + // (getInheritableAssetModelImage). Default: a model with no image, so the + // existing suites see no behaviour change. + assetModel: { + findFirst: vitest.fn().mockResolvedValue(null), + findFirstOrThrow: vitest.fn().mockResolvedValue({ + id: "am-1", + defaultCategoryId: null, + defaultValuation: null, + }), + }, assetCustomFieldValue: { findMany: vitest.fn().mockResolvedValue([]), }, @@ -1391,6 +1402,174 @@ describe("updateAsset custom-field writes", () => { }); }); +/* ====================================================================== */ +/* Asset-model cover-image inheritance */ +/* ====================================================================== */ + +/** + * An asset linked to an AssetModel that carries a cover image shows that + * image instead of the grey placeholder — the model's file is uploaded and + * stored once, and each asset points at that single storage object. + */ +describe("updateAsset asset-model cover image", () => { + /** Signed URL of model `am-1`'s shared cover image. */ + const MODEL_IMAGE_URL = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-1/asset-models/am-1/image-1700000000000.png?token=abc"; + /** An image the user uploaded for this specific asset. */ + const OWN_IMAGE_URL = + "https://xyz.supabase.co/storage/v1/object/sign/assets/user-1/asset-1/main-image-1700000000000.png?token=abc"; + + beforeEach(async () => { + vitest.clearAllMocks(); + // why: clearAllMocks keeps implementations AND unconsumed `...Once` queues, + // so sibling suites leak into these tests — one pins extractStoragePath to a + // fixed path, others leave queued findUnique values behind. These tests hinge + // on the real path parsing (model folder vs per-asset folder) and on their + // own asset row, so reset the mocks this suite drives and re-arm them. + (db.asset.findUnique as ReturnType).mockReset(); + (db.assetModel.findFirst as ReturnType).mockReset(); + (db.asset.update as ReturnType).mockReset(); + + const actualImageUtils = await vitest.importActual< + Record unknown> + >("~/components/assets/asset-image/utils"); + (extractStoragePath as ReturnType).mockImplementation( + actualImageUtils.extractStoragePath + ); + + (db.asset.update as ReturnType).mockResolvedValue({ + id: "asset-1", + title: "Asset 1", + category: null, + valuation: null, + }); + // INDIVIDUAL so the model link is allowed; no image of its own by default. + (db.asset.findUnique as ReturnType).mockResolvedValue({ + type: "INDIVIDUAL", + mainImage: null, + }); + (db.assetModel.findFirst as ReturnType).mockResolvedValue( + { + id: "am-1", + } + ); + (createSignedUrl as ReturnType).mockResolvedValue( + "https://signed-thumbnail" + ); + }); + + it("stamps the model's image onto an asset that has none of its own", async () => { + (db.assetModel.findFirst as ReturnType).mockResolvedValue( + { + image: MODEL_IMAGE_URL, + imageExpiration: new Date("2026-08-01T00:00:00.000Z"), + } + ); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: "am-1", + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + expect(data.mainImage).toBe(MODEL_IMAGE_URL); + expect(data.thumbnailImage).toBe("https://signed-thumbnail"); + }); + + it("never overwrites an image the user uploaded for the asset itself", async () => { + (db.asset.findUnique as ReturnType).mockResolvedValue({ + type: "INDIVIDUAL", + mainImage: OWN_IMAGE_URL, + }); + (db.assetModel.findFirst as ReturnType).mockResolvedValue( + { + image: MODEL_IMAGE_URL, + imageExpiration: new Date("2026-08-01T00:00:00.000Z"), + } + ); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: "am-1", + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + // `undefined` = field untouched by this update. + expect(data.mainImage).toBeUndefined(); + }); + + // why: regression — re-linking an inheriting asset to a model that has NO + // image used to leave the previous model's photo on the row, so the asset + // showed model A's picture while linked to model B. + it("clears a previously inherited image when the new model has none", async () => { + (db.asset.findUnique as ReturnType).mockResolvedValue({ + type: "INDIVIDUAL", + mainImage: MODEL_IMAGE_URL, + }); + (db.assetModel.findFirst as ReturnType).mockResolvedValue( + { + image: null, + imageExpiration: null, + } + ); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: "am-2", + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + expect(data.mainImage).toBeNull(); + expect(data.mainImageExpiration).toBeNull(); + expect(data.thumbnailImage).toBeNull(); + }); + + it("drops the inherited image when the model is unlinked", async () => { + (db.asset.findUnique as ReturnType).mockResolvedValue({ + mainImage: MODEL_IMAGE_URL, + }); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: null, + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + expect(data.assetModel).toEqual({ disconnect: true }); + expect(data.mainImage).toBeNull(); + }); + + it("leaves the asset's own image alone when the model is unlinked", async () => { + (db.asset.findUnique as ReturnType).mockResolvedValue({ + mainImage: OWN_IMAGE_URL, + }); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: null, + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + expect(data.assetModel).toEqual({ disconnect: true }); + expect(data.mainImage).toBeUndefined(); + }); +}); + describe("updateAsset newLocationQuantity", () => { beforeEach(() => { vitest.clearAllMocks(); diff --git a/apps/webapp/app/modules/asset/service.server.ts b/apps/webapp/app/modules/asset/service.server.ts index a41774ed24..46ed797bf9 100644 --- a/apps/webapp/app/modules/asset/service.server.ts +++ b/apps/webapp/app/modules/asset/service.server.ts @@ -167,6 +167,8 @@ import type { Column } from "../asset-index-settings/helpers"; import { createAssetModelsIfNotExists, getAssetModel, + getInheritableAssetModelImage, + isAssetModelImageUrl, } from "../asset-model/service.server"; import { cancelAssetReminderScheduler } from "../asset-reminder/scheduler.server"; import { lockAssetForQuantityUpdate } from "../consumption-log/quantity-lock.server"; @@ -1479,6 +1481,28 @@ export async function createAsset({ }, }, }); + + /** + * Inherit the model's cover image when this asset has none of its own. + * The model's image is uploaded and stored ONCE — the asset just points + * at the same storage object — so a workspace with 100 units of the + * same model uploads one file instead of 100 identical ones. + * An explicitly-supplied `mainImage` always wins. + */ + if (!mainImage) { + const inherited = await getInheritableAssetModelImage({ + assetModelId, + organizationId, + }); + + if (inherited) { + Object.assign(data, { + mainImage: inherited.image, + mainImageExpiration: inherited.imageExpiration, + thumbnailImage: inherited.thumbnailImage, + }); + } + } } // Placement can't be set inline in the asset create (the AssetLocation @@ -2203,6 +2227,31 @@ export async function updateAsset({ disconnect: true, }, }); + + /** + * Drop an inherited cover image alongside the link. Without this the + * asset would keep showing a model image it no longer belongs to — two + * signals to reconcile ("no model" + "the model's photo"). An image the + * user uploaded for THIS asset is left untouched. + * + * Note the edit route always sends `assetModelId: assetModelId || null`, + * so this branch runs on every save of a model-less asset; the ownership + * test makes it a no-op there. + */ + if (mainImage === undefined) { + const currentAsset = await db.asset.findUnique({ + where: { id, organizationId }, + select: { mainImage: true }, + }); + + if (isAssetModelImageUrl(currentAsset?.mainImage)) { + Object.assign(data, { + mainImage: null, + mainImageExpiration: null, + thumbnailImage: null, + }); + } + } } else if (assetModelId) { // Org-scope guard before the connect — Prisma's FK only enforces // that the AssetModel row exists, not that it belongs to the @@ -2220,7 +2269,7 @@ export async function updateAsset({ // org-scoped index lookup, only on the link branch. const currentAsset = await db.asset.findUnique({ where: { id, organizationId }, - select: { type: true }, + select: { type: true, mainImage: true }, }); if (currentAsset && currentAsset.type === AssetType.QUANTITY_TRACKED) { throw new ShelfError({ @@ -2242,6 +2291,34 @@ export async function updateAsset({ }, }, }); + + /** + * Reconcile the inherited cover image with the newly-linked model. Same + * one-upload-many-assets contract as `createAsset`; an image the user + * uploaded for this asset always wins, as does an image being set in this + * very update. + * + * This ASSIGNS unconditionally (rather than only when the new model has + * an image) so that re-linking an inheriting asset to a model with no + * image clears the old model's photo instead of leaving the asset showing + * model A's picture while linked to model B. + */ + const hasOwnImage = + currentAsset?.mainImage != null && + !isAssetModelImageUrl(currentAsset.mainImage); + + if (mainImage === undefined && !hasOwnImage) { + const inherited = await getInheritableAssetModelImage({ + assetModelId, + organizationId, + }); + + Object.assign(data, { + mainImage: inherited?.image ?? null, + mainImageExpiration: inherited?.imageExpiration ?? null, + thumbnailImage: inherited?.thumbnailImage ?? null, + }); + } } /** Connect the new location id */ diff --git a/apps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsx b/apps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsx index 9fc60ada13..d25ec2ac34 100644 --- a/apps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsx +++ b/apps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsx @@ -16,7 +16,9 @@ import AssetModelForm, { import { getCategoriesForCreateAndEdit } from "~/modules/asset/service.server"; import { getAssetModel, + refreshExpiredAssetModelImages, updateAssetModel, + updateAssetModelImage, } from "~/modules/asset-model/service.server"; import { appendToMetaTitle } from "~/utils/append-to-meta-title"; import { sendNotification } from "~/utils/emitter/send-notification.server"; @@ -59,9 +61,17 @@ export async function loader({ context, request, params }: LoaderFunctionArgs) { const header = { title }; + /** + * Supabase signed URLs live for 72h, so an older model image would render + * broken in the form's preview. Re-sign before shipping it to the client. + */ + const [refreshedAssetModel] = await refreshExpiredAssetModelImages([ + assetModel, + ]); + return payload({ header, - assetModel, + assetModel: refreshedAssetModel, categories, totalCategories, currency: currentOrganization?.currency, @@ -93,8 +103,14 @@ export async function action({ context, request, params }: LoaderFunctionArgs) { action: PermissionAction.update, }); + /** + * Multipart form (optional cover image) — clone so the text fields and the + * streaming file parser each get their own read of the body. + */ + const clonedRequest = request.clone(); + const parsedPayload = parseData( - await request.formData(), + await clonedRequest.formData(), AssetModelFormSchema, { additionalData: { userId, id, organizationId } } ); @@ -105,6 +121,18 @@ export async function action({ context, request, params }: LoaderFunctionArgs) { organizationId, }); + /** + * Stores a newly-picked image and re-points every inheriting asset at it. + * No-ops when the user didn't pick a file, so a plain Save keeps the + * current image. + */ + await updateAssetModelImage({ + request, + assetModelId: id, + userId: authSession.userId, + organizationId, + }); + sendNotification({ title: "Asset model updated", message: "Your asset model has been updated successfully", diff --git a/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx b/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx index 08dd98c195..dccf38d5d5 100644 --- a/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx +++ b/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx @@ -10,6 +10,7 @@ import type { LoaderFunctionArgs, MetaFunction } from "react-router"; import { data } from "react-router"; import AssetModelQuickActions from "~/components/asset-model/asset-model-quick-actions"; import AssetModelBulkActionsDropdown from "~/components/asset-model/bulk-actions-dropdown"; +import ImageWithPreview from "~/components/image-with-preview/image-with-preview"; import type { HeaderData } from "~/components/layout/header/types"; import LineBreakText from "~/components/layout/line-break-text"; import { List } from "~/components/list"; @@ -17,7 +18,10 @@ import { Badge } from "~/components/shared/badge"; import { Button } from "~/components/shared/button"; import { Th, Td } from "~/components/table"; import { useUserRoleHelper } from "~/hooks/user-user-role-helper"; -import { getAssetModels } from "~/modules/asset-model/service.server"; +import { + getAssetModels, + refreshExpiredAssetModelImages, +} from "~/modules/asset-model/service.server"; import { appendToMetaTitle } from "~/utils/append-to-meta-title"; import { setCookie, @@ -58,6 +62,14 @@ export async function loader({ context, request }: LoaderFunctionArgs) { }); const totalPages = Math.ceil(totalAssetModels / perPage); + /** + * Supabase signed URLs expire after 72h, so re-sign any that lapsed before + * shipping the rows to the client — otherwise the thumbnails render broken. + * Same treatment the kit and asset lists give their images. + */ + const refreshedAssetModels = + await refreshExpiredAssetModelImages(assetModels); + const header: HeaderData = { title: "Asset Models", subHeading: @@ -71,7 +83,7 @@ export async function loader({ context, request }: LoaderFunctionArgs) { return data( payload({ header, - items: assetModels, + items: refreshedAssetModels, search, page, totalItems: totalAssetModels, @@ -137,7 +149,7 @@ export default function AssetModelsIndexPage() { const AssetModelItem = ({ item, }: { - item: Pick & { + item: Pick & { _count: { assets: number; }; @@ -146,7 +158,22 @@ const AssetModelItem = ({ }) => ( <> - {item.name} + {/* + Image + name in one cell, matching the kit and asset list rows: the + picture is the fastest way to confirm you're looking at the right + model, and it's the same picture its assets now show. + */} +
+ {item.image ? ( + + ) : null} + {item.name} +
{item.description ? ( diff --git a/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx b/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx index 81f06141e3..53450f9835 100644 --- a/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx +++ b/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx @@ -12,7 +12,10 @@ import AssetModelForm, { AssetModelFormSchema, } from "~/components/asset-model/form"; import { getCategoriesForCreateAndEdit } from "~/modules/asset/service.server"; -import { createAssetModel } from "~/modules/asset-model/service.server"; +import { + createAssetModel, + updateAssetModelImage, +} from "~/modules/asset-model/service.server"; import { appendToMetaTitle } from "~/utils/append-to-meta-title"; import { sendNotification } from "~/utils/emitter/send-notification.server"; import { makeShelfError } from "~/utils/error"; @@ -74,8 +77,17 @@ export async function action({ context, request }: LoaderFunctionArgs) { action: PermissionAction.create, }); + /** + * The form is multipart (it carries an optional cover image), so the + * request body has to be read twice: once here for the text fields and + * once by `updateAssetModelImage`'s streaming file parser. Cloning is the + * same pattern the kit routes use — a body stream can only be consumed + * once. + */ + const clonedRequest = request.clone(); + const parsedData = parseData( - await request.formData(), + await clonedRequest.formData(), AssetModelFormSchema, { additionalData: { userId, organizationId }, @@ -88,6 +100,17 @@ export async function action({ context, request }: LoaderFunctionArgs) { organizationId, }); + /** + * Runs after the create so the image is stored under the new model's id. + * No-ops when the user didn't pick a file. + */ + await updateAssetModelImage({ + request, + assetModelId: assetModel.id, + userId: authSession.userId, + organizationId, + }); + sendNotification({ title: "Asset model created", message: "Your asset model has been created successfully", diff --git a/apps/webapp/app/routes/api+/asset.generate-thumbnail.ts b/apps/webapp/app/routes/api+/asset.generate-thumbnail.ts index cd788e3c59..4eac274f2e 100644 --- a/apps/webapp/app/routes/api+/asset.generate-thumbnail.ts +++ b/apps/webapp/app/routes/api+/asset.generate-thumbnail.ts @@ -12,7 +12,11 @@ import { PermissionEntity, } from "~/utils/permissions/permission.data"; import { requirePermission } from "~/utils/roles.server"; -import { createSignedUrl, uploadFile } from "~/utils/storage.server"; +import { + createSignedUrl, + getThumbnailStoragePath, + uploadFile, +} from "~/utils/storage.server"; const THUMBNAIL_SIZE = 108; @@ -171,17 +175,8 @@ export async function loader({ request, context }: LoaderFunctionArgs) { yield new Uint8Array(buffer); }; - // Generate thumbnail filename - let thumbnailPath: string; - - // Check if the file has an extension - if (originalPath.includes(".")) { - // File has extension, replace before the extension - thumbnailPath = originalPath.replace(/(\.[^.]+)$/, "-thumbnail$1"); - } else { - // File has no extension, just append -thumbnail - thumbnailPath = `${originalPath}-thumbnail`; - } + // Generate thumbnail filename — same convention the upload path writes to + const thumbnailPath = getThumbnailStoragePath(originalPath); // Create and upload thumbnail const uploadedPath = await uploadFile(createAsyncIterable(), { diff --git a/apps/webapp/app/utils/storage.server.ts b/apps/webapp/app/utils/storage.server.ts index c0a318acab..563dbe4c76 100644 --- a/apps/webapp/app/utils/storage.server.ts +++ b/apps/webapp/app/utils/storage.server.ts @@ -256,6 +256,25 @@ export async function createSignedUrl({ } } +/** + * Derives the storage path of an image's 108px thumbnail from the path of the + * full-size image: `-thumbnail` goes before the extension, or is appended when + * the path has none. + * + * Single source of truth for the convention, shared by the upload path (which + * creates the object) and the lazy `api+/asset.generate-thumbnail` route (which + * recreates it on demand) — the two must agree or a thumbnail is uploaded to + * one path and read from another. + * + * @param filename - Storage path of the full-size image (no bucket prefix) + * @returns Storage path its thumbnail lives at + */ +export function getThumbnailStoragePath(filename: string): string { + return filename.includes(".") + ? filename.replace(/(\.[^.]+)$/, "-thumbnail$1") + : `${filename}-thumbnail`; +} + export async function uploadFile( fileData: AsyncIterable, { @@ -287,17 +306,7 @@ export async function uploadFile( // If thumbnail generation is requested if (generateThumbnail) { - // Generate a thumbnail filename - let thumbFilename: string; - - // Check if the file has an extension - if (filename.includes(".")) { - // File has extension, add '-thumbnail' before the extension - thumbFilename = filename.replace(/(\.[^.]+)$/, "-thumbnail$1"); - } else { - // File has no extension, just append '-thumbnail' - thumbFilename = `${filename}-thumbnail`; - } + const thumbFilename = getThumbnailStoragePath(filename); // Create thumbnail version with Sharp const thumbnailFile = await cropImage( From 881821f99dbd51098c433bf0cc8a4a99b3847202 Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Thu, 30 Jul 2026 13:43:20 +0200 Subject: [PATCH 2/6] fix(assets): guard model-image writes against concurrent image changes Addresses the Codex review on #2774. - `propagateAssetModelImageToAssets` and `clearInheritedAssetModelImages` decided ownership in application code against a prior read, then wrote by id. An image uploaded for a specific asset in that gap was overwritten by the model cover, breaking the per-asset-image-wins contract. Both now carry the observed `mainImage` in each predicate (grouped by value, so the common case is still a single query), the same optimistic-concurrency shape `refreshExpiredAssetImages` uses. - `refreshExpiredAssetModelImages` re-signed the expired URL and wrote it back guarded only by model + organization, so it could resurrect a superseded cover over one that had just been replaced and propagated. Now guarded on the image it read; a zero-count update is discarded and the next load re-reads. - A failure in the image step of asset-model creation left the model row committed while the action reported an error, so the user retried and got a duplicate. The row is now rolled back before the error surfaces. --- .../asset-model/service.server.test.ts | 132 +++++++++++++++++- .../app/modules/asset-model/service.server.ts | 121 +++++++++++----- .../_layout+/settings.asset-models.new.tsx | 27 +++- 3 files changed, 234 insertions(+), 46 deletions(-) diff --git a/apps/webapp/app/modules/asset-model/service.server.test.ts b/apps/webapp/app/modules/asset-model/service.server.test.ts index 4ee980e738..cc5cab8f69 100644 --- a/apps/webapp/app/modules/asset-model/service.server.test.ts +++ b/apps/webapp/app/modules/asset-model/service.server.test.ts @@ -11,6 +11,7 @@ import { getInheritableAssetModelImage, isAssetModelImageUrl, propagateAssetModelImageToAssets, + refreshExpiredAssetModelImages, updateAssetModel, deleteAssetModel, bulkDeleteAssetModels, @@ -25,6 +26,7 @@ vitest.mock("~/database/db.server", () => ({ findMany: vitest.fn(), findFirstOrThrow: vitest.fn(), update: vitest.fn(), + updateMany: vitest.fn(), deleteMany: vitest.fn(), count: vitest.fn(), }, @@ -559,7 +561,13 @@ describe("bulkDeleteAssetModels", () => { }); expect(db.asset.updateMany).toHaveBeenCalledWith({ - where: { id: { in: ["asset-inheriting"] }, organizationId: "org-123" }, + where: { + id: { in: ["asset-inheriting"] }, + organizationId: "org-123", + // Guarded on the image the row was observed to have, so a concurrent + // per-asset upload is never clobbered. + mainImage: MODEL_IMAGE_URL, + }, data: { mainImage: null, mainImageExpiration: null, @@ -576,6 +584,63 @@ describe("bulkDeleteAssetModels", () => { }); }); +describe("refreshExpiredAssetModelImages", () => { + const expiredModel = { + id: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + imageExpiration: new Date("2020-01-01T00:00:00.000Z"), + }; + + beforeEach(() => { + vitest.clearAllMocks(); + }); + + it("leaves an unexpired image alone", async () => { + const fresh = { + ...expiredModel, + imageExpiration: new Date("2999-01-01T00:00:00.000Z"), + }; + + await expect(refreshExpiredAssetModelImages([fresh])).resolves.toEqual([ + fresh, + ]); + expect(db.assetModel.updateMany).not.toHaveBeenCalled(); + }); + + it("re-signs an expired image, guarded on the image it read", async () => { + // @ts-expect-error mock setup + db.assetModel.updateMany.mockResolvedValue({ count: 1 }); + + const [refreshed] = await refreshExpiredAssetModelImages([expiredModel]); + + expect(db.assetModel.updateMany).toHaveBeenCalledWith({ + where: { + id: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + }, + data: { + image: expect.stringContaining("-models/model-1/"), + imageExpiration: expect.any(Date), + }, + }); + expect(refreshed.image).not.toBe(MODEL_IMAGE_URL); + }); + + // why: a cover replaced (and propagated to this model's assets) between the + // read and this write must not be overwritten with a re-signed URL for the + // superseded object. + it("discards the refresh when a newer cover already won", async () => { + // @ts-expect-error mock setup + db.assetModel.updateMany.mockResolvedValue({ count: 0 }); + + const [refreshed] = await refreshExpiredAssetModelImages([expiredModel]); + + expect(refreshed.image).toBe(MODEL_IMAGE_URL); + }); +}); + describe("clearInheritedAssetModelImages", () => { beforeEach(() => { vitest.clearAllMocks(); @@ -605,7 +670,13 @@ describe("clearInheritedAssetModelImages", () => { }); expect(db.asset.updateMany).toHaveBeenCalledWith({ - where: { id: { in: ["asset-inheriting"] }, organizationId: "org-123" }, + where: { + id: { in: ["asset-inheriting"] }, + organizationId: "org-123", + // Guarded on the image the row was observed to have, so a concurrent + // per-asset upload is never clobbered. + mainImage: MODEL_IMAGE_URL, + }, data: { mainImage: null, mainImageExpiration: null, @@ -767,8 +838,10 @@ describe("propagateAssetModelImageToAssets", () => { { id: "asset-inheriting", mainImage: MODEL_IMAGE_URL_RESIGNED }, { id: "asset-own-image", mainImage: OWN_ASSET_IMAGE_URL }, ]); + // The two inheriting rows have different observed images, so they land in + // two grouped, individually-guarded writes of one row each. // @ts-expect-error mock setup - db.asset.updateMany.mockResolvedValue({ count: 2 }); + db.asset.updateMany.mockResolvedValue({ count: 1 }); const count = await propagateAssetModelImageToAssets({ assetModelId: "model-1", @@ -782,10 +855,14 @@ describe("propagateAssetModelImageToAssets", () => { where: { assetModelId: "model-1", organizationId: "org-123" }, select: { id: true, mainImage: true }, }); + // Two writes: the rows are grouped by the image each was observed to have, + // and every predicate carries that value so a concurrent per-asset upload + // can't be clobbered. expect(db.asset.updateMany).toHaveBeenCalledWith({ where: { - id: { in: ["asset-no-image", "asset-inheriting"] }, + id: { in: ["asset-no-image"] }, organizationId: "org-123", + mainImage: null, }, data: { mainImage: MODEL_IMAGE_URL, @@ -796,6 +873,25 @@ describe("propagateAssetModelImageToAssets", () => { thumbnailImage: MODEL_THUMBNAIL_URL, }, }); + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { + id: { in: ["asset-inheriting"] }, + organizationId: "org-123", + mainImage: MODEL_IMAGE_URL_RESIGNED, + }, + data: { + mainImage: MODEL_IMAGE_URL, + mainImageExpiration: imageExpiration, + thumbnailImage: MODEL_THUMBNAIL_URL, + }, + }); + expect(db.asset.updateMany).not.toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + id: { in: expect.arrayContaining(["asset-own-image"]) }, + }), + }) + ); expect(count).toBe(2); }); @@ -832,4 +928,32 @@ describe("propagateAssetModelImageToAssets", () => { expect(db.asset.updateMany).not.toHaveBeenCalled(); expect(count).toBe(0); }); + + // why: the inheriting/own-image decision is made in app code against a prior + // read. Without the observed image in the predicate, an upload landing in that + // gap would be overwritten by the model cover. + it("reports no change for an asset whose image moved between the read and the write", async () => { + // @ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-racing", mainImage: null }, + ]); + // Zero rows matched: the row no longer holds the image we observed. + // @ts-expect-error mock setup + db.asset.updateMany.mockResolvedValue({ count: 0 }); + + const count = await propagateAssetModelImageToAssets({ + assetModelId: "model-1", + organizationId: "org-123", + image: MODEL_IMAGE_URL, + imageExpiration, + thumbnailImage: MODEL_THUMBNAIL_URL, + }); + + expect(db.asset.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ mainImage: null }), + }) + ); + expect(count).toBe(0); + }); }); diff --git a/apps/webapp/app/modules/asset-model/service.server.ts b/apps/webapp/app/modules/asset-model/service.server.ts index 3695ed261d..1198506a33 100644 --- a/apps/webapp/app/modules/asset-model/service.server.ts +++ b/apps/webapp/app/modules/asset-model/service.server.ts @@ -509,28 +509,74 @@ export async function propagateAssetModelImageToAssets({ select: { id: true, mainImage: true }, }); - const inheritingAssetIds = candidates - .filter( - (asset) => - asset.mainImage === null || - isAssetModelImageUrl(asset.mainImage, assetModelId) - ) - .map((asset) => asset.id); + const inheriting = candidates.filter( + (asset) => + asset.mainImage === null || + isAssetModelImageUrl(asset.mainImage, assetModelId) + ); + + return writeGuardedByObservedImage(inheriting, organizationId, { + mainImage: image, + mainImageExpiration: imageExpiration, + thumbnailImage, + }); +} - if (inheritingAssetIds.length === 0) { +/** + * Applies an image write to assets, guarded on the `mainImage` each row was + * observed to have. + * + * The ownership decision ("is this asset inheriting, or does it have its own + * image?") happens in application code against a prior read, so a plain + * `updateMany` by id would clobber an image uploaded in the gap between that + * read and this write — the exact case the per-asset-image-wins contract must + * not lose. Carrying the observed value in the predicate makes each row's write + * conditional: a row that changed underneath simply matches zero rows and keeps + * whatever it now holds. Same optimistic-concurrency shape + * `refreshExpiredAssetImages` uses for its deferred re-signed URLs. + * + * Rows are grouped by observed value, so the common case (all null, or all + * showing the same model URL) is a single query. + * + * @param assets - Rows to write, each carrying the `mainImage` just read + * @param organizationId - Org scope for the write + * @param data - The image fields to set + * @returns Number of rows that actually changed + */ +async function writeGuardedByObservedImage( + assets: { id: string; mainImage: string | null }[], + organizationId: Organization["id"], + data: { + mainImage: string | null; + mainImageExpiration: Date | null; + thumbnailImage: string | null; + } +) { + if (assets.length === 0) { return 0; } - const { count } = await db.asset.updateMany({ - where: { id: { in: inheritingAssetIds }, organizationId }, - data: { - mainImage: image, - mainImageExpiration: imageExpiration, - thumbnailImage, - }, + /** observed `mainImage` → ids of the assets that had it */ + const idsByObservedImage = new Map(); + assets.forEach((asset) => { + const ids = idsByObservedImage.get(asset.mainImage); + if (ids) { + ids.push(asset.id); + return; + } + idsByObservedImage.set(asset.mainImage, [asset.id]); }); - return count; + const results = await Promise.all( + [...idsByObservedImage.entries()].map(([observedImage, ids]) => + db.asset.updateMany({ + where: { id: { in: ids }, organizationId, mainImage: observedImage }, + data, + }) + ) + ); + + return results.reduce((total, result) => total + result.count, 0); } /** @@ -564,26 +610,15 @@ export async function clearInheritedAssetModelImages({ select: { id: true, mainImage: true, assetModelId: true }, }); - const inheritingAssetIds = candidates - .filter((asset) => - isAssetModelImageUrl(asset.mainImage, asset.assetModelId ?? undefined) - ) - .map((asset) => asset.id); - - if (inheritingAssetIds.length === 0) { - return 0; - } + const inheriting = candidates.filter((asset) => + isAssetModelImageUrl(asset.mainImage, asset.assetModelId ?? undefined) + ); - const { count } = await db.asset.updateMany({ - where: { id: { in: inheritingAssetIds }, organizationId }, - data: { - mainImage: null, - mainImageExpiration: null, - thumbnailImage: null, - }, + return writeGuardedByObservedImage(inheriting, organizationId, { + mainImage: null, + mainImageExpiration: null, + thumbnailImage: null, }); - - return count; } /** @@ -635,11 +670,25 @@ export async function refreshExpiredAssetModelImages< }); const imageExpiration = threeDaysFromNow(); - await db.assetModel.update({ - where: { id: model.id, organizationId: model.organizationId }, + /** + * Guarded on the image we read, so a cover replaced (and propagated to + * this model's assets) between that read and this write is not overwritten + * with a re-signed URL for the superseded object. Zero rows matched means + * a newer cover won — drop this refresh and let the next load re-read. + */ + const { count } = await db.assetModel.updateMany({ + where: { + id: model.id, + organizationId: model.organizationId, + image: model.image, + }, data: { image, imageExpiration }, }); + if (count === 0) { + return null; + } + return { id: model.id, image, imageExpiration }; }) ); diff --git a/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx b/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx index 53450f9835..569927a59d 100644 --- a/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx +++ b/apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx @@ -14,6 +14,7 @@ import AssetModelForm, { import { getCategoriesForCreateAndEdit } from "~/modules/asset/service.server"; import { createAssetModel, + deleteAssetModel, updateAssetModelImage, } from "~/modules/asset-model/service.server"; import { appendToMetaTitle } from "~/utils/append-to-meta-title"; @@ -103,13 +104,27 @@ export async function action({ context, request }: LoaderFunctionArgs) { /** * Runs after the create so the image is stored under the new model's id. * No-ops when the user didn't pick a file. + * + * A failure here (file rejected, storage or signing error) would otherwise + * leave the model committed while the action reports an error — the user + * sees "creation failed", retries, and ends up with a duplicate. Roll the + * row back so the failure the user is told about is the truth. */ - await updateAssetModelImage({ - request, - assetModelId: assetModel.id, - userId: authSession.userId, - organizationId, - }); + try { + await updateAssetModelImage({ + request, + assetModelId: assetModel.id, + userId: authSession.userId, + organizationId, + }); + } catch (cause) { + await deleteAssetModel({ id: assetModel.id, organizationId }).catch( + () => { + /* Best-effort rollback; the original failure is what matters. */ + } + ); + throw cause; + } sendNotification({ title: "Asset model created", From f615666af947afba96d527d3603b80fd7c85e6bb Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Thu, 30 Jul 2026 13:56:33 +0200 Subject: [PATCH 3/6] fix(assets): address review on the asset-model cover image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #2774. - `ImageWithPreview` was rendered with `withPreview` but no `imageUrl` on the model form and the models list. `withPreview` adds role="button", tabIndex and an "Open preview for …" label unconditionally, while the open handler no-ops without a full-size URL — both surfaces exposed a focusable control that did nothing. Pass `imageUrl` at both call sites. - The asset edit route resends the current `assetModelId` on every save, so an unrelated metadata edit re-read the model and re-signed its thumbnail each time. Both reconcile paths are now gated on the link actually changing; a model whose image changed is already propagated to its assets. - An asset-model image with no expiration would have been copied onto inheriting assets as `mainImageExpiration: null`, which `refreshExpiredAssetImages` skips, so the URL would lapse and never be re-signed. Treat an unknown expiration as already elapsed instead, which self-heals on the next read. --- .../app/components/asset-model/form.tsx | 4 +++ .../app/modules/asset-model/service.server.ts | 9 +++++- .../app/modules/asset/service.server.test.ts | 32 +++++++++++++++++++ .../app/modules/asset/service.server.ts | 24 ++++++++++---- .../_layout+/settings.asset-models.index.tsx | 3 ++ 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/apps/webapp/app/components/asset-model/form.tsx b/apps/webapp/app/components/asset-model/form.tsx index e2dd153530..fe30ebb4ff 100644 --- a/apps/webapp/app/components/asset-model/form.tsx +++ b/apps/webapp/app/components/asset-model/form.tsx @@ -355,7 +355,11 @@ function FullPageForm({ >
{assetModel?.image ? ( + // `imageUrl` is required alongside `withPreview`: the preview + // trigger is keyboard-focusable and labelled "Open preview for …", + // but its handler no-ops without a full-size URL — a dead control. { it("drops the inherited image when the model is unlinked", async () => { (db.asset.findUnique as ReturnType).mockResolvedValue({ mainImage: MODEL_IMAGE_URL, + assetModelId: "am-1", }); await updateAsset({ @@ -1554,6 +1555,7 @@ describe("updateAsset asset-model cover image", () => { it("leaves the asset's own image alone when the model is unlinked", async () => { (db.asset.findUnique as ReturnType).mockResolvedValue({ mainImage: OWN_IMAGE_URL, + assetModelId: "am-1", }); await updateAsset({ @@ -1568,6 +1570,36 @@ describe("updateAsset asset-model cover image", () => { expect(data.assetModel).toEqual({ disconnect: true }); expect(data.mainImage).toBeUndefined(); }); + + // why: the edit route resends the asset's current assetModelId on every save, + // so an unrelated metadata edit would otherwise re-read the model and re-sign + // its thumbnail every time. + it("does not touch the image when a save keeps the same model", async () => { + (db.asset.findUnique as ReturnType).mockResolvedValue({ + type: "INDIVIDUAL", + mainImage: MODEL_IMAGE_URL, + assetModelId: "am-1", + }); + + await updateAsset({ + id: "asset-1", + userId: "user-1", + organizationId: "org-1", + assetModelId: "am-1", + } as any); + + const { data } = (db.asset.update as ReturnType).mock + .calls[0][0]; + expect(data.mainImage).toBeUndefined(); + // The org-scope IDOR guard still reads the model (select: { id }), but the + // image read never happens — so neither does the signed-URL request. + expect(db.assetModel.findFirst).not.toHaveBeenCalledWith( + expect.objectContaining({ + select: { image: true, imageExpiration: true }, + }) + ); + expect(createSignedUrl).not.toHaveBeenCalled(); + }); }); describe("updateAsset newLocationQuantity", () => { diff --git a/apps/webapp/app/modules/asset/service.server.ts b/apps/webapp/app/modules/asset/service.server.ts index 46ed797bf9..14eac1e727 100644 --- a/apps/webapp/app/modules/asset/service.server.ts +++ b/apps/webapp/app/modules/asset/service.server.ts @@ -2234,17 +2234,20 @@ export async function updateAsset({ * signals to reconcile ("no model" + "the model's photo"). An image the * user uploaded for THIS asset is left untouched. * - * Note the edit route always sends `assetModelId: assetModelId || null`, - * so this branch runs on every save of a model-less asset; the ownership - * test makes it a no-op there. + * The edit route always sends `assetModelId: assetModelId || null`, so + * this branch also runs on every save of an already-model-less asset. Only + * a save that actually removes a link needs reconciling, so the read is + * skipped when there was no link to begin with. */ if (mainImage === undefined) { const currentAsset = await db.asset.findUnique({ where: { id, organizationId }, - select: { mainImage: true }, + select: { mainImage: true, assetModelId: true }, }); - if (isAssetModelImageUrl(currentAsset?.mainImage)) { + const isRemovingLink = currentAsset?.assetModelId != null; + + if (isRemovingLink && isAssetModelImageUrl(currentAsset.mainImage)) { Object.assign(data, { mainImage: null, mainImageExpiration: null, @@ -2269,7 +2272,7 @@ export async function updateAsset({ // org-scoped index lookup, only on the link branch. const currentAsset = await db.asset.findUnique({ where: { id, organizationId }, - select: { type: true, mainImage: true }, + select: { type: true, mainImage: true, assetModelId: true }, }); if (currentAsset && currentAsset.type === AssetType.QUANTITY_TRACKED) { throw new ShelfError({ @@ -2302,12 +2305,19 @@ export async function updateAsset({ * an image) so that re-linking an inheriting asset to a model with no * image clears the old model's photo instead of leaving the asset showing * model A's picture while linked to model B. + * + * Gated on the link actually changing: the edit route resends the current + * `assetModelId` on every save, so without this an unrelated metadata edit + * would cost a model read plus a Supabase signed-URL request each time. A + * model whose image changed has already been propagated to its assets by + * `propagateAssetModelImageToAssets`, so there is nothing to catch up on. */ + const isChangingModel = currentAsset?.assetModelId !== assetModelId; const hasOwnImage = currentAsset?.mainImage != null && !isAssetModelImageUrl(currentAsset.mainImage); - if (mainImage === undefined && !hasOwnImage) { + if (isChangingModel && mainImage === undefined && !hasOwnImage) { const inherited = await getInheritableAssetModelImage({ assetModelId, organizationId, diff --git a/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx b/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx index dccf38d5d5..040c974d23 100644 --- a/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx +++ b/apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx @@ -165,7 +165,10 @@ const AssetModelItem = ({ */}
{item.image ? ( + // `imageUrl` is required alongside `withPreview` — see the note at the + // matching call site in components/asset-model/form.tsx. Date: Thu, 30 Jul 2026 14:05:56 +0200 Subject: [PATCH 4/6] test(assets): document the mocks in the cover-image suite Every mock needs a `// why:` per the repo's testing conventions. Each override in this suite encodes a different starting state (own image vs inherited, linked vs unlinked), so the comments say which case the fixture is standing in for rather than repeating that the database is mocked. --- .../app/modules/asset/service.server.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/apps/webapp/app/modules/asset/service.server.test.ts b/apps/webapp/app/modules/asset/service.server.test.ts index 5c8b4eba3d..23e94e2644 100644 --- a/apps/webapp/app/modules/asset/service.server.test.ts +++ b/apps/webapp/app/modules/asset/service.server.test.ts @@ -1459,6 +1459,8 @@ describe("updateAsset asset-model cover image", () => { }); it("stamps the model's image onto an asset that has none of its own", async () => { + // why: stands in for the model row being linked to — the source of the + // image this asset should inherit. (db.assetModel.findFirst as ReturnType).mockResolvedValue( { image: MODEL_IMAGE_URL, @@ -1480,10 +1482,14 @@ describe("updateAsset asset-model cover image", () => { }); it("never overwrites an image the user uploaded for the asset itself", async () => { + // why: the persisted row is what makes this case distinct — an image stored + // under the asset's own folder, which must survive the model link. (db.asset.findUnique as ReturnType).mockResolvedValue({ type: "INDIVIDUAL", mainImage: OWN_IMAGE_URL, }); + // why: a model that does have an image, so the test proves the asset's own + // image wins rather than that there was nothing to inherit. (db.assetModel.findFirst as ReturnType).mockResolvedValue( { image: MODEL_IMAGE_URL, @@ -1508,10 +1514,14 @@ describe("updateAsset asset-model cover image", () => { // image used to leave the previous model's photo on the row, so the asset // showed model A's picture while linked to model B. it("clears a previously inherited image when the new model has none", async () => { + // why: an asset currently inheriting model A's image — the starting state + // the reconcile has to clear when it is relinked. (db.asset.findUnique as ReturnType).mockResolvedValue({ type: "INDIVIDUAL", mainImage: MODEL_IMAGE_URL, }); + // why: the newly-linked model has no image, which is the case that used to + // leave model A's photo behind. (db.assetModel.findFirst as ReturnType).mockResolvedValue( { image: null, @@ -1534,6 +1544,8 @@ describe("updateAsset asset-model cover image", () => { }); it("drops the inherited image when the model is unlinked", async () => { + // why: an asset that HAS a link and is showing that model's image; both + // fields are read to decide whether the unlink needs reconciling. (db.asset.findUnique as ReturnType).mockResolvedValue({ mainImage: MODEL_IMAGE_URL, assetModelId: "am-1", @@ -1553,6 +1565,8 @@ describe("updateAsset asset-model cover image", () => { }); it("leaves the asset's own image alone when the model is unlinked", async () => { + // why: same linked starting state as above but with the asset's own image, + // so the ownership test is what decides the outcome. (db.asset.findUnique as ReturnType).mockResolvedValue({ mainImage: OWN_IMAGE_URL, assetModelId: "am-1", @@ -1575,6 +1589,9 @@ describe("updateAsset asset-model cover image", () => { // so an unrelated metadata edit would otherwise re-read the model and re-sign // its thumbnail every time. it("does not touch the image when a save keeps the same model", async () => { + // why: stands in for the persisted row this scenario needs — an asset + // already linked to `am-1` and already showing that model's image, which is + // what makes the incoming `assetModelId` an unchanged link. (db.asset.findUnique as ReturnType).mockResolvedValue({ type: "INDIVIDUAL", mainImage: MODEL_IMAGE_URL, From 28ec0d9143be169eb15fdf1aca2f96d167b8f695 Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Thu, 30 Jul 2026 15:12:24 +0200 Subject: [PATCH 5/6] fix(assets): add the cover image to the inline create-asset-model dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The image field only reached the full-page settings form. `AssetModelForm` has a second, compact branch used by the "Create new asset model" shortcut inside the asset form's model picker, and that branch had no file input and no multipart encoding — so a model created from the asset form (the most discoverable path) could never get an image, and its assets had nothing to inherit. Both halves of the original report reproduce from that one gap. Both branches now carry the same field, the same shared 8 MB/type file guard and the same server-error surface. The dialog already posted to `/settings/asset-models/new`, which handles the upload, so only the form needed changing. --- .../app/components/asset-model/form.tsx | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/apps/webapp/app/components/asset-model/form.tsx b/apps/webapp/app/components/asset-model/form.tsx index fe30ebb4ff..bf58073598 100644 --- a/apps/webapp/app/components/asset-model/form.tsx +++ b/apps/webapp/app/components/asset-model/form.tsx @@ -109,6 +109,22 @@ export default function AssetModelForm({ const nameError = fetcherValidationErrors?.name?.message || zo.errors.name()?.message; + // Client-side file guard (type + 8MB cap), shared with the asset, kit and + // full-page model image inputs so every surface rejects the same files with + // the same copy. + const [, validateFile] = useAtom(assetImageValidateFileAtom); + const fileError = useAtomValue(fileErrorAtom); + + /** + * The image has no zod field (a File can't be parsed by the text schema), so + * its errors arrive either from the client-side guard or as the server's + * `field: "image"` ShelfError on the fetcher. + */ + const inlineImageError = + (fetcher.data?.error?.additionalData?.field === "image" + ? fetcher.data.error.message + : undefined) ?? fileError; + /* ------------------------------------------------------------------ */ /* Inline / dialog mode */ /* ------------------------------------------------------------------ */ @@ -120,6 +136,13 @@ export default function AssetModelForm({ className="w-full rounded border border-gray-200 bg-white px-6 py-5" ref={zo.ref} action={apiUrl} + /** + * Multipart so this dialog can carry the cover image too. Without it the + * file input silently posts nothing and a model created from the asset + * form would have no image — leaving its assets nothing to inherit, + * which reads as "the feature doesn't work". + */ + encType="multipart/form-data" >
+ {/* Same cover-image field as the settings form, in a compact layout. */} +
+ +

+ Optional. Shown on every asset of this model that has no image of + its own. PNG, JPG, JPEG or WebP, max. 8 MB. +

+
+
{onCancel ? ( From 87fe98f140155ca2ec9bfb027565584a95942947 Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Thu, 30 Jul 2026 15:27:34 +0200 Subject: [PATCH 6/6] fix(assets): scope the model image file error and describe the field Addresses the CodeRabbit review on the inline dialog. - `fileErrorAtom` is module-scoped, and the create-model dialog opens inside the asset form, so rejecting a file in one input surfaced the error on the other and picking a valid file cleared an error the user still needed. The rule is extracted to a pure `validateSelectedFile`, and asset-model images now use a scoped error atom built by `createScopedValidateFile`. The shared factory and its three existing consumers keep the current behaviour. - The image inputs had no relationship to the text stating accepted formats and size, so assistive tech never announced it. Both inputs now carry `aria-describedby`; the full-page form duplicates the text per breakpoint, so it references both ids (the display:none copy is out of the a11y tree). --- apps/webapp/app/atoms/file.ts | 136 +++++++++++++----- .../app/components/asset-model/form.tsx | 44 ++++-- 2 files changed, 134 insertions(+), 46 deletions(-) diff --git a/apps/webapp/app/atoms/file.ts b/apps/webapp/app/atoms/file.ts index 9193cca967..2b6788f122 100644 --- a/apps/webapp/app/atoms/file.ts +++ b/apps/webapp/app/atoms/file.ts @@ -9,47 +9,99 @@ import { verifyAccept } from "~/utils/verify-file-accept"; export const fileErrorAtom = atom(undefined); -export const createValidateFileAtom = (options: { +/** Size + type limits and the copy shown when a file violates them. */ +type ValidateFileOptions = { maxSize: number; sizeErrorMessage: string; allowedTypesErrorMessage: string; -}) => +}; + +/** + * Validates the picked file and normalises the input in place. + * + * Clears the input on a rejected type or size (so an invalid file is never + * submitted) and rewrites the `FileList` when the filename had to be sanitised + * — a raw filename breaks the content-disposition header on upload. + * + * Pure with respect to state: it returns the message instead of writing it, so + * the same rule can back both the shared and the scoped atoms below. + * + * @param event - Change event from the file input + * @param options - Size/type limits and their messages + * @returns The validation message, or undefined when the file is acceptable + */ +function validateSelectedFile( + event: ChangeEvent, + options: ValidateFileOptions +): string | undefined { + const file = event?.target?.files?.[0]; + if (!file) { + return undefined; + } + + const allowedType = verifyAccept(file.type, event.target.accept); + const allowedSize = file.size < options.maxSize; + + if (!allowedType) { + event.target.value = ""; + return options.allowedTypesErrorMessage; + } + + if (!allowedSize) { + /** Clean the field */ + event.target.value = ""; + return options.sizeErrorMessage; + } + + // Sanitize the filename to prevent content-disposition header issues + if (event.target.files) { + const sanitizedFile = sanitizeFile(file); + + // If the filename was changed, we need to update the file input + if (sanitizedFile.name !== file.name) { + // Create a new DataTransfer to replace the file in the input + const dataTransfer = new DataTransfer(); + dataTransfer.items.add(sanitizedFile); + event.target.files = dataTransfer.files; + } + } + + return undefined; +} + +export const createValidateFileAtom = (options: ValidateFileOptions) => atom(null, (_get, set, event: ChangeEvent) => { - set(fileErrorAtom, () => { - const file = event?.target?.files?.[0]; - if (file) { - const allowedType = verifyAccept(file.type, event.target.accept); - const allowedSize = file.size < options.maxSize; - - if (!allowedType) { - event.target.value = ""; - return options.allowedTypesErrorMessage; - } - - if (!allowedSize) { - /** Clean the field */ - event.target.value = ""; - return options.sizeErrorMessage; - } - - // Sanitize the filename to prevent content-disposition header issues - if (event.target.files) { - const sanitizedFile = sanitizeFile(file); - - // If the filename was changed, we need to update the file input - if (sanitizedFile.name !== file.name) { - // Create a new DataTransfer to replace the file in the input - const dataTransfer = new DataTransfer(); - dataTransfer.items.add(sanitizedFile); - event.target.files = dataTransfer.files; - } - } - - return undefined; - } - }); + set(fileErrorAtom, () => validateSelectedFile(event, options)); }); +/** + * Builds a validator with its OWN error atom, for file inputs that can be on + * screen at the same time as another one. + * + * {@link fileErrorAtom} is module-scoped, so every consumer of + * {@link createValidateFileAtom} shares one error slot. That is fine while only + * one such form is mounted (asset, kit and audit forms never coexist), but the + * inline "create asset model" dialog opens *inside* the asset form — with a + * shared slot, rejecting a file in the dialog would also light up the asset's + * own image field, and picking a valid one would clear an error the user still + * needs to see. + * + * @param options - Same size/type limits as the shared factory + * @returns `errorAtom` to read the message from, `validateAtom` to pass to `onChange` + */ +export const createScopedValidateFile = (options: ValidateFileOptions) => { + const errorAtom = atom(undefined); + + const validateAtom = atom( + null, + (_get, set, event: ChangeEvent) => { + set(errorAtom, () => validateSelectedFile(event, options)); + } + ); + + return { errorAtom, validateAtom }; +}; + // Default instance with 4MB limit export const defaultValidateFileAtom = createValidateFileAtom({ maxSize: DEFAULT_MAX_IMAGE_UPLOAD_SIZE, // 4MB @@ -70,3 +122,17 @@ export const auditImageValidateFileAtom = createValidateFileAtom({ sizeErrorMessage: "Max file size is 4MB", allowedTypesErrorMessage: "Allowed file types are: PNG, JPG or JPEG", }); + +/** + * Asset-model cover image — same 8MB limit as asset images, but scoped, because + * the inline create-model dialog is rendered inside the asset form and would + * otherwise share its error slot. + */ +export const { + errorAtom: assetModelImageErrorAtom, + validateAtom: assetModelImageValidateFileAtom, +} = createScopedValidateFile({ + maxSize: ASSET_MAX_IMAGE_UPLOAD_SIZE, // 8MB + sizeErrorMessage: "Max file size is 8MB", + allowedTypesErrorMessage: "Allowed file types are: PNG, JPG, JPEG, or WebP", +}); diff --git a/apps/webapp/app/components/asset-model/form.tsx b/apps/webapp/app/components/asset-model/form.tsx index bf58073598..3d7466690d 100644 --- a/apps/webapp/app/components/asset-model/form.tsx +++ b/apps/webapp/app/components/asset-model/form.tsx @@ -17,7 +17,10 @@ import { useAtom, useAtomValue } from "jotai"; import { useActionData, useLoaderData } from "react-router"; import { useZorm } from "react-zorm"; import z from "zod"; -import { assetImageValidateFileAtom, fileErrorAtom } from "~/atoms/file"; +import { + assetModelImageErrorAtom, + assetModelImageValidateFileAtom, +} from "~/atoms/file"; import { useAutoFocus } from "~/hooks/use-auto-focus"; import { useDisabled } from "~/hooks/use-disabled"; import useFetcherWithReset from "~/hooks/use-fetcher-with-reset"; @@ -34,6 +37,10 @@ import ImageWithPreview from "../image-with-preview/image-with-preview"; import { Button } from "../shared/button"; import { Card } from "../shared/card"; +/** Links each image input to the

stating its accepted formats and size. */ +const INLINE_IMAGE_HELP_ID = "asset-model-image-help-inline"; +const PAGE_IMAGE_HELP_ID = "asset-model-image-help"; + /** Zod schema for creating/editing an asset model. */ export const AssetModelFormSchema = z.object({ name: z.string().min(2, "Name is required"), @@ -109,11 +116,11 @@ export default function AssetModelForm({ const nameError = fetcherValidationErrors?.name?.message || zo.errors.name()?.message; - // Client-side file guard (type + 8MB cap), shared with the asset, kit and - // full-page model image inputs so every surface rejects the same files with - // the same copy. - const [, validateFile] = useAtom(assetImageValidateFileAtom); - const fileError = useAtomValue(fileErrorAtom); + // Client-side file guard (type + 8MB cap). Deliberately SCOPED, not the shared + // `fileErrorAtom`: this dialog opens inside the asset form, whose own image + // input would otherwise surface — and clear — this field's error. + const [, validateFile] = useAtom(assetModelImageValidateFileAtom); + const fileError = useAtomValue(assetModelImageErrorAtom); /** * The image has no zod field (a File can't be parsed by the text schema), so @@ -172,6 +179,10 @@ export default function AssetModelForm({

and adds no + // describedby of its own, so this is the only link between the field + // and its format/size requirements for assistive tech. + aria-describedby={INLINE_IMAGE_HELP_ID} disabled={disabled} accept={ACCEPT_SUPPORTED_IMAGES} name="image" @@ -180,7 +191,10 @@ export default function AssetModelForm({ error={inlineImageError} inputClassName="border-0 shadow-none p-0 rounded-none" /> -

+

Optional. Shown on every asset of this model that has no image of its own. PNG, JPG, JPEG or WebP, max. 8 MB.

@@ -260,8 +274,8 @@ function FullPageForm({ // Client-side file guard (type + 8MB cap) shared with the asset/kit image // inputs, so all three surfaces reject the same files with the same copy. - const [, validateFile] = useAtom(assetImageValidateFileAtom); - const fileError = useAtomValue(fileErrorAtom); + const [, validateFile] = useAtom(assetModelImageValidateFileAtom); + const fileError = useAtomValue(assetModelImageErrorAtom); /** * The image upload has no zod field (a File can't be parsed by the text @@ -407,7 +421,7 @@ function FullPageForm({ withPreview /> ) : null} -

+

Accepts PNG, JPG, JPEG, or WebP (max.8 MB)

and + * adds no describedby of its own, so nothing is being overridden. + */ + aria-describedby={`${PAGE_IMAGE_HELP_ID} ${PAGE_IMAGE_HELP_ID}-sm`} error={imageError} className="mt-2" inputClassName="border-0 shadow-none p-0 rounded-none" /> -

+

Accepts PNG, JPG, JPEG, or WebP (max.8 MB)