fix(assets): honour the asset model's cover image on its assets - #2774
fix(assets): honour the asset model's cover image on its assets#2774carlosvirreira wants to merge 7 commits into
Conversation
`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."
🩺 React Doctor — webapp✅ No new findings on the files changed by this PR. Run locally with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3b592def
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAsset-model cover images now support multipart upload, signed URL refresh, previews, inheritance by linked assets, preservation of user-uploaded asset images, and cleanup before model deletion. Thumbnail path generation is centralized for upload and thumbnail-generation flows. ChangesAsset-model image lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant AssetModelRoute
participant AssetModelService
participant Storage
participant Assets
Editor->>AssetModelRoute: Submit multipart asset-model form
AssetModelRoute->>AssetModelService: Parse and persist image
AssetModelService->>Storage: Upload and sign image
AssetModelService->>Assets: Propagate inherited image fields
Assets-->>AssetModelService: Return guarded update count
AssetModelRoute-->>Editor: Return refreshed asset-model data
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/webapp/app/components/asset-model/form.tsx (1)
365-382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssociate the "Accepts PNG, JPG…" hint with the file input.
The helper text is rendered as sibling
<p>elements with noid/aria-describedbylink, so screen-reader users never hear the format/size constraint. Also worth collapsing the duplicated lg/mobile copy into one element with responsive classes.As per coding guidelines, UI implementations must satisfy WCAG 2.1 AA requirements, including
aria-describedbyfor helper/error text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/asset-model/form.tsx` around lines 365 - 382, Update the file input in the form component around validateFile to reference a uniquely identified helper-text element through aria-describedby, and assign that id to a single shared format/size hint. Replace the duplicated desktop/mobile <p> elements with one responsive element while preserving their visibility behavior and ensure the existing imageError remains associated as needed.Source: Coding guidelines
apps/webapp/app/modules/asset/service.server.ts (1)
1484-1505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
createAsset's new image-inheritance branch.The
updateAsset asset-model cover imagesuite added inapps/webapp/app/modules/asset/service.server.test.ts(lines 1405-1572) only exercisesupdateAsset. There's no equivalent case forcreateAssetstampingmainImage/mainImageExpiration/thumbnailImagefromgetInheritableAssetModelImagewhen a new asset is linked to a model at creation time. The PR objectives explicitly call out "tests covering inheritance, propagation, deletion cleanup, relinking" — the creation-time inheritance path is the one gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/asset/service.server.ts` around lines 1484 - 1505, Add coverage in the asset service tests for createAsset’s image-inheritance branch: mock getInheritableAssetModelImage to return inherited image fields, create an asset linked to the model without an explicit mainImage, and assert mainImage, mainImageExpiration, and thumbnailImage are persisted from the inherited result. Keep the explicit-mainImage precedence behavior covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 356-364: Add the corresponding imageUrl prop to both
ImageWithPreview call sites: use assetModel.image in
apps/webapp/app/components/asset-model/form.tsx lines 356-364 and item.image in
apps/webapp/app/routes/_layout+/settings.asset-models.index.tsx lines 167-174,
alongside each existing thumbnailUrl prop.
In `@apps/webapp/app/modules/asset-model/service.server.ts`:
- Around line 288-292: The inherited asset model response should assign a
72-hour expiration when assetModel.imageExpiration is null or missing. Update
the return mapping near signAssetModelThumbnail so imageExpiration uses the
existing value when present, otherwise derives a new expiration 72 hours from
the current time, ensuring legacy images become refreshable.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 2230-2254: Gate the inherited-image unlink logic around the
visible currentAsset lookup and mainImage reset, and the corresponding
relink/reconciliation path, on whether the model association changed:
previousAsset.assetModelId !== assetModelId. Preserve reconciliation when
linking or unlinking a different model, but skip both paths when metadata saves
retain the same assetModelId to avoid re-fetching or re-signing the image.
---
Nitpick comments:
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 365-382: Update the file input in the form component around
validateFile to reference a uniquely identified helper-text element through
aria-describedby, and assign that id to a single shared format/size hint.
Replace the duplicated desktop/mobile <p> elements with one responsive element
while preserving their visibility behavior and ensure the existing imageError
remains associated as needed.
In `@apps/webapp/app/modules/asset/service.server.ts`:
- Around line 1484-1505: Add coverage in the asset service tests for
createAsset’s image-inheritance branch: mock getInheritableAssetModelImage to
return inherited image fields, create an asset linked to the model without an
explicit mainImage, and assert mainImage, mainImageExpiration, and
thumbnailImage are persisted from the inherited result. Keep the
explicit-mainImage precedence behavior covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb7a3147-74e9-4fcf-80f2-b16494d36ce1
📒 Files selected for processing (10)
apps/webapp/app/components/asset-model/form.tsxapps/webapp/app/modules/asset-model/service.server.test.tsapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/_layout+/settings.asset-models.$assetModelId_.edit.tsxapps/webapp/app/routes/_layout+/settings.asset-models.index.tsxapps/webapp/app/routes/_layout+/settings.asset-models.new.tsxapps/webapp/app/routes/api+/asset.generate-thumbnail.tsapps/webapp/app/utils/storage.server.ts
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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/app/routes/_layout+/settings.asset-models.new.tsx (1)
113-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed rollback failure.
If
deleteAssetModelalso fails, the orphaned model row disappears silently with no trace to reconcile later. ALogger.errorin the catch keeps the original error as the thrown one while leaving a breadcrumb.🔍 Proposed change
} catch (cause) { - await deleteAssetModel({ id: assetModel.id, organizationId }).catch( - () => { - /* Best-effort rollback; the original failure is what matters. */ - } - ); + await deleteAssetModel({ id: assetModel.id, organizationId }).catch( + (rollbackCause) => { + /* Best-effort rollback; the original failure is what matters. */ + Logger.error( + makeShelfError(rollbackCause, { + userId, + assetModelId: assetModel.id, + message: + "Failed to roll back asset model after image processing error", + }) + ); + } + ); throw cause; }Add the import:
-import { makeShelfError } from "~/utils/error"; +import { makeShelfError } from "~/utils/error"; +import { Logger } from "~/utils/logger";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/routes/_layout`+/settings.asset-models.new.tsx around lines 113 - 127, Update the rollback catch around updateAssetModelImage to log failures from deleteAssetModel with Logger.error before swallowing them, while preserving the original cause as the error rethrown by the outer catch.apps/webapp/app/modules/asset-model/service.server.ts (1)
559-579: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffUnbounded query fan-out when observed images differ per row.
Grouping collapses the common cases, but per-asset re-signed URLs (each asset gets its own signed token from
refreshExpiredAssetImages) mean the map can degenerate to one group per asset — thousands of concurrentupdateManycalls in a singlePromise.all, which can saturate the Prisma connection pool. Consider capping concurrency (or chunking the entries) so the fan-out stays bounded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/modules/asset-model/service.server.ts` around lines 559 - 579, The idsByObservedImage update flow currently launches one updateMany call per observed image without a concurrency limit. Replace the unbounded Promise.all around idsByObservedImage.entries() with bounded concurrency or chunked processing, preserving each updateMany filter and the summed result count while ensuring only a fixed number of database updates run at once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/webapp/app/modules/asset-model/service.server.ts`:
- Around line 559-579: The idsByObservedImage update flow currently launches one
updateMany call per observed image without a concurrency limit. Replace the
unbounded Promise.all around idsByObservedImage.entries() with bounded
concurrency or chunked processing, preserving each updateMany filter and the
summed result count while ensuring only a fixed number of database updates run
at once.
In `@apps/webapp/app/routes/_layout`+/settings.asset-models.new.tsx:
- Around line 113-127: Update the rollback catch around updateAssetModelImage to
log failures from deleteAssetModel with Logger.error before swallowing them,
while preserving the original cause as the error rethrown by the outer catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fb9dda6-af9b-4560-bf34-5ca69a44dc80
📒 Files selected for processing (3)
apps/webapp/app/modules/asset-model/service.server.test.tsapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/routes/_layout+/settings.asset-models.new.tsx
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/modules/asset/service.server.test.ts`:
- Around line 1577-1579: Add a concise // why: comment directly above the
db.asset.findUnique mock in the “does not touch the image when a save keeps the
same model” test, explaining that it simulates the persisted inherited-image
state required for this update scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e80519b4-0b15-4170-a69d-e3c51fdb7caf
📒 Files selected for processing (5)
apps/webapp/app/components/asset-model/form.tsxapps/webapp/app/modules/asset-model/service.server.tsapps/webapp/app/modules/asset/service.server.test.tsapps/webapp/app/modules/asset/service.server.tsapps/webapp/app/routes/_layout+/settings.asset-models.index.tsx
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.
Bot review round — summary for whoever reviews thisCodex and CodeRabbit raised 8 findings. Six were real and are fixed, one is a
The one I declined (#4), and whyCodex asked for the previous image and thumbnail to be deleted from storage when
The right shape is one cleanup pass covering assets, kits and models together, Verification after the fixes
|
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webapp/app/components/asset-model/form.tsx (1)
112-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared image-error resolution.
Inline and full-page modes duplicate the “server
imageerror first, otherwise client file error” logic. Move this into a focused helper so the two modes cannot drift.As per coding guidelines, duplicated code patterns across files or functions should be extracted into focused reusable helpers.
Also applies to: 261-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/webapp/app/components/asset-model/form.tsx` around lines 112 - 127, Extract the duplicated image-error resolution into a focused reusable helper, using the existing server error data and fileError inputs. The helper should return the server error when additionalData.field is "image", otherwise return the client-side file error, and both inline and full-page modes should use it instead of implementing their own fallback logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 112-127: Scope the client-side file validation error used by the
compact model form to its own upload instance instead of reading the
module-scoped fileErrorAtom directly. Update the validator and error wiring
around assetImageValidateFileAtom, inlineImageError, and the imageError usage so
sibling asset or kit inputs cannot set or clear this form’s error; preserve the
existing server-side image error handling.
- Around line 171-188: Update the image Input in the asset-model form to provide
an aria-describedby relationship covering both the helper text and any error
description generated internally by Input. Assign a stable id to the
requirements paragraph and merge it with the existing Input error-description id
without replacing either reference.
---
Nitpick comments:
In `@apps/webapp/app/components/asset-model/form.tsx`:
- Around line 112-127: Extract the duplicated image-error resolution into a
focused reusable helper, using the existing server error data and fileError
inputs. The helper should return the server error when additionalData.field is
"image", otherwise return the client-side file error, and both inline and
full-page modes should use it instead of implementing their own fallback logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a635cf21-0ef8-4177-b8bd-3daaffe4f8f2
📒 Files selected for processing (1)
apps/webapp/app/components/asset-model/form.tsx
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).
Update — the entry point that was actually brokenThe image field only ever reached the full-page settings form. Fixed in
Browser-verified end to end on the dialog path: create a model there with an image → an asset created in the same flow with no upload of its own inherits the image and the shared thumbnail and renders in the asset list; and rejecting a file in one input no longer touches the other. Total review round: 10 threads, 0 open. Nine fixed, one declined with reasoning (deleting superseded storage objects — see the earlier summary comment). All 8 checks green. |
2 participants
Problem
AssetModel.imageandAssetModel.imageExpirationshipped as columns with theasset-models feature, but nothing ever read or wrote them:
Default category, Default valuation) — there is no way to upload an image;
modules/asset-model/service.server.tscontained the stringimagezerotimes, so the columns could never be populated by any user action;
/static/images/asset-placeholder.jpg.Net effect: a workspace with 40 identical units of the same model has to pick and
upload the same photo 40 times, and store 40 copies of it. Reported from a user
trying the product out: "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."
Kithas the identicalimage/imageExpirationpair plus all fiveplumbing pieces (multipart form, file input, clone-then-parse route, update
service, expired-URL re-sign helper).
AssetModelgot the two columns and noneof them.
Approach
Upload once on the model; every asset of that model that has no image of its own
points at that same single storage object.
The inheritance is applied where the link is made (
createAsset,updateAsset)and re-applied when the model's image changes. That means every surface which
already reads
Asset.mainImageshows the model image with no change to thosecall sites — asset index (simple + advanced), asset detail, booking rows,
scanner drawers, reports, the booking/audit PDFs, the command palette, and the
companion app.
Storage holds one image plus one thumbnail per model, regardless of how many
assets share it. No migration: both columns already exist.
Alternatives considered
Asset.mainImagenull, resolveasset.mainImage ?? assetModel.imageat render time). Conceptually purer, butasset images are rendered by six independent implementations (
AssetImage,ImageWithPreview, the command palette's privategetAssetImage, the auditreceipt PDF's raw
<img>, the reminder email, the booking PDF) plus ~11companion screens, over ~7 server select shapes — one of which is the
advanced-index raw-SQL
jsonb_build_object. Same user-visible result, an orderof magnitude more blast radius, and it could not reach the companion app
without a mobile release.
asset-modelsbucket. Actively breaks the fix — see the bucketnote below.
asks us not to do. (The CSV import path already demonstrates the cost: it
uploads one storage object per asset for the same source image.)
Changes
modules/asset-model/service.server.tsupdateAssetModelImage— mirrorsupdateKitImage: multipart parse → resize →108px thumbnail → sign → persist, then fans out to inheriting assets. No-ops
when no file was submitted, so a plain Save keeps the current image.
propagateAssetModelImageToAssets— oneupdateManyre-pointing the model'sinheriting assets at the new image + shared thumbnail.
clearInheritedAssetModelImages— called fromdeleteAssetModelandbulkDeleteAssetModelsbefore the delete, while the links still exist toidentify (
Asset.assetModelIdisON DELETE SET NULL).getInheritableAssetModelImage— narrow org-scoped read used by the assetcreate/update paths.
refreshExpiredAssetModelImages— mirrorsrefreshExpiredKitImages.isAssetModelImageUrl— the ownership test (see below).modules/asset/service.server.ts—createAssetinherits the linked model'simage when no image was supplied;
updateAssetreconciles on link, and clears aninherited image on unlink.
bulkCreateAssetsFromModelneeded no change: itroutes through
createAsset, so all N bulk-created assets inherit via the samepath.
Settings routes + form —
encType="multipart/form-data", clone-then-parse inboth actions, an Image row on the form with the shared 8 MB/type file guard and
a preview in edit mode, and the model's thumbnail in the list row.
utils/storage.server.ts— extractedgetThumbnailStoragePathso the uploadpath and the lazy
api+/asset.generate-thumbnailroute derive the thumbnail pathfrom one helper instead of two copies of the same rule.
Two decisions worth reviewing
Model images live in the
assetsbucket, not a new one. Inheriting assetsstore the model's URL in
Asset.mainImage, and three existing paths resolve itwith
extractStoragePath(url, "assets")(refreshExpiredAssetImages,api+/asset.refresh-main-image,api+/asset.generate-thumbnail). A foreignbucket makes those return
null, so every inherited image would 404 after 72hwith no recovery.
Ownership is decided by storage path, not URL string. Model images live at
<userId>/asset-models/<assetModelId>/image-<unix>, per-asset images at<userId>/<assetId>/main-image-<unix>. Signed URLs get re-signed per asset overtime (
refreshExpiredAssetImageswrites a fresh token onto the asset row), so anexact-URL comparison would lose track of the inheritance — the path is the only
stable part. An image the user uploaded for a specific asset always wins and is
never overwritten. This is the single load-bearing invariant of the design, so
it's pinned by tests.
Verification
typecheckclean,lint0 errors,react-doctor0 new errors, 3688 unit testspass (+18 new: ownership test incl. the re-signed-URL case, propagation,
delete-cleanup ordering, and the reconcile-on-relink case).
Driven end to end in a browser against a real Supabase bucket:
assets/<userId>/asset-models/<modelId>/image-<unix>.png, thumbnail beside it.model's picture on the asset index and the asset detail header. Both rows'
mainImageresolve to one storage path (verified in the DB: 2 assets,1 distinct path).
asset follows to the new picture; the one with its own image is untouched.
of a deleted model); the own-image asset keeps its picture.
Out of scope
since it reads
Asset.mainImage. Per.claude/rules/code-bearing-entity-list-consistency.mdthe companion is owned by another team; nothing here touches it.
be added afterwards on the model, which then propagates.
removed. That matches existing behaviour for asset and kit images
(
deleteAssetImagehas no callers today) — deliberately not changed here.Summary by CodeRabbit
New Features
Bug Fixes
Chores