feat(edit-content): enable image editor in Image and File fields (#36363)#36406
Open
oidacra wants to merge 7 commits into
Open
feat(edit-content): enable image editor in Image and File fields (#36363)#36406oidacra wants to merge 7 commits into
oidacra wants to merge 7 commits into
Conversation
Show the image editor for Image fields, and for File fields only when the referenced asset is an image. Image/File resolve a separate dotAsset by identifier, so availability is driven by a shared isImageFile() predicate over the referenced asset metadata (isImage flag, then image/* mime, then extension). Restricted to the new Angular Edit Content: Image/File never expose the editor in the legacy Dojo host. Binary behavior is unchanged. Introduce an ImageEditSaveStrategy resolved by input type so the save path is selected per field type: Binary applies the edit inline (current behavior); Image/File get a scaffolded dotAsset strategy for the upcoming versioned check-in (safe no-op that never corrupts the identifier reference). Refs #36363
The Save servlet can return the edited temp file as metadata: null, image: false, mimeType: "unknown". Downstream that stopped treating it as an image: blank thumbnail, hidden edit pencil, and a crash in the file-info dialog header (metadata.title on null). Enrich the edited temp file as an image (enrichEditedImage util) so every consumer and host stays consistent, and null-guard the dialog header binding. Refs #36363
Open
26 tasks
github-actions
Bot
added
the
Area : Frontend
Contributor
🤖 dotBot Review (Bedrock)Reviewed 15 file(s); 8 candidate(s) → 7 confirmed, 0 uncertain (unverified, kept for review). Confirmed findings
us.deepseek.r1-v1:0 · Run: #28619833600 · tokens: in: 80762 · out: 23523 · total: 104285 · calls: 28 · est. ~$0.236 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Move the image-detection predicate out of libs/utils and into the image-editor lib (alongside enrichEditedImage), exported from its public API. Groups the image-ness logic with its owner and leaves libs/utils untouched. Drop the file-extension fallback: rely on the authoritative isImage flag and the image/* content type, both reliably present on the metadata the gate runs against (dotAsset assetMetaData and enriched Binary temp files). File names are not a trusted signal. Refs #36363
12 tasks
Implement the dotAsset save strategy: on saving an image edit from an Image/File field, check in and publish a new version of the referenced dotAsset via the default PUBLISH workflow action, in the asset's own language, then refresh the preview. The field value (the identifier) is unchanged, so the reference is preserved and other content sharing the asset sees the update. The edited binary is passed as the staged temp file id in the asset field; the check-in resolves it server-side. Add DotWorkflowActionsFireService.publishContentletByIdentifier with optional language support (sent as ?language=). Verified end-to-end: PUBLISH fire returns 200, the field reflects the new version, and the reference is preserved. Refs #36363
Contributor
- Version the correct binary field: use the referenced contentlet's titleImage (asset for dotAsset, fileAsset for a legacy FileAsset) instead of hardcoding 'asset', so saves work for File fields backed by a FileAsset. - Keep the Binary edit-image gate strict (isImage flag only) so its behavior is unchanged; the mime fallback in isImageFile applies only to Image/File. - Surface a message instead of silently discarding the edit when the save strategy is reached without a resolved reference. - Tighten the with-save regression assertion to the exact enriched payload. Refs #36363
Open
26 tasks
The Strategy pattern (interface + 2 impls + resolver + DI wiring) was over-engineered for two stable branches. Replace it with a publishEditedAsset rxMethod on FileFieldStore, next to applyTempFile/getAssetData where the asset logic already lives, and branch inline in the component (Binary -> applyTempFile, Image/File -> publishEditedAsset). The rxMethod's switchMap also serializes concurrent saves (a re-triggered edit cancels the in-flight publish+refresh), closing the concurrency gap the review flagged. Behavior is unchanged: same PUBLISH-by-identifier in the asset's language, same titleImage-based field selection (asset vs fileAsset), same refresh without mutating the field value. Removes the save-strategy folder and its specs; adds store coverage. Refs #36363
| // Image/File resolve a separate dotAsset/FileAsset and are only supported | ||
| // in the new Angular Edit Content, where the image-editor launcher is | ||
| // provided — never in the legacy Dojo host. | ||
| return isImageFile(this.#currentMetadata()) && !!this.#imageEditorLauncher; |
Contributor
There was a problem hiding this comment.
🔴 [Critical] Undefined metadata passed to isImageFile causes runtime error
In dot-file-field.component.ts line 230, isImageFile is called with this.value.metadata without checking if metadata exists. When this.value exists but lacks metadata (e.g. empty file field), this results in passing undefined to isImageFile, leading to runtime errors when accessing properties on metadata.
| return true; | ||
| } | ||
|
|
||
| return !!metadata.contentType?.toLowerCase().startsWith('image/'); |
Contributor
There was a problem hiding this comment.
🔴 [Critical] Potential runtime error when metadata.contentType is undefined
The code calls startsWith('image/') on metadata.contentType?.toLowerCase(), which could be undefined if contentType is missing. This would throw TypeError: Cannot read properties of undefined (reading 'startsWith'). Optional chaining should extend to the method call: metadata.contentType?.toLowerCase()?.startsWith('image/') ?? false.
The component no longer subscribes to the launcher stream or routes by input type. onEditImage just hands the launcher's close stream to a store method, store.applyEditedImage(result$), which owns the whole flow: filter the closed editor (null), route by input type (Binary inline vs Image/File versioned), and surface a server error on stream failure. Subscription is torn down with the store. Keeps the component thin (no #applyEditedImage, no manual subscribe/#destroyRef for this path) and the persistence logic cohesive in the store. Refs #36363
| const focalPoint = parseFocalPoint(metadata?.focalPoint); | ||
|
|
||
| this.#applyEditedImage( | ||
| this.store.applyEditedImage( |
Contributor
There was a problem hiding this comment.
🔴 [Critical] Uncaught errors in image save due to missing error handling
The removal of the #applyEditedImage method without adding error handling to the store.applyEditedImage call leaves potential errors uncaught. The current code subscribes to the store method without an error handler, which could lead to silent failures when saving edited images, causing user data loss and unawareness of failures.
| */ | ||
| export function isImageFile(metadata: Partial<DotFileMetadata> | null | undefined): boolean { | ||
| if (!metadata) { | ||
| return false; |
Contributor
There was a problem hiding this comment.
🟠 [High] Incorrect handling of isImage flag in isImageFile utility
The code checks if metadata.isImage is truthy, but when the server sets isImage: false, it falls back to content type check. This can incorrectly treat non-images as images if their content type is image/*. The correct approach is to return false when isImage is explicitly false and only fallback to content type check when isImage is undefined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enables the image editor in Image and File fields, with a versioned, published save to the referenced file-asset (#36363). A single component renders Binary/Image/File, so the save path is selected per input type via a strategy; detection and the edited-image result handling are hardened along the way.
Draft while it gets review + broader test coverage (integration/Postman).
What this changes
Availability (Image/File)
isImageFile()(in the image-editor lib) using the authoritativeisImageflag then theimage/*content type. Extension sniffing is intentionally avoided — the mime type is reliably present on the metadata the gate runs against.Save (provider per input type)
ImageEditSaveStrategy:dotAssetvia the default PUBLISH workflow action, in the asset's own language, then refreshes the preview. The field value (the identifier) is unchanged, so the reference is preserved and other content sharing the asset sees the update. The edited binary is passed as the staged temp file id in theassetfield; the check-in resolves it server-side.DotWorkflowActionsFireService.publishContentletByIdentifierwith optional language support (?language=).Edited-image robustness
metadata: null, image: false, mimeType: "unknown", which broke the thumbnail, hid the edit pencil, and crashed the file-info dialog header. Fixed by enriching the edited temp file as an image (enrichEditedImage) and null-guarding the dialog header.Acceptance criteria
fire/PUBLISH→ 200)assetfield / identifier)Notes for reviewers
titleImage(assetfor dotAsset,fileAssetfor a legacy FileAsset), the Binary edit-image gate stays strict (isImageonly), and the reference-missing path surfaces a message instead of silently discarding the edit.Pending / follow-up
Test plan
edit-content(save-strategy resolver/strategies incl. dotAsset publish + refresh + error + guard; availability across field types; legacy-host restriction; preview null-metadata),image-editor(isImageFile,enrichEditedImage,with-save),data-access(fire service). All passing; lint clean.PUT /api/v1/workflow/actions/default/fire/PUBLISH?language=1→ 200; the field refreshes to the new version (size changed), the reference is preserved, and the field stays re-editable; no console errors.