Skip to content

feat(edit-content): enable image editor in Image and File fields (#36363)#36406

Open
oidacra wants to merge 9 commits into
mainfrom
issue-36363-image-editor-image-file-fields
Open

feat(edit-content): enable image editor in Image and File fields (#36363)#36406
oidacra wants to merge 9 commits into
mainfrom
issue-36363-image-editor-image-file-fields

Conversation

@oidacra

@oidacra oidacra commented Jul 2, 2026

Copy link
Copy Markdown
Member

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.

CleanShot 2026-07-03 at 10 39 19@2x

Draft while it gets review + broader test coverage (integration/Postman).

What this changes

Availability (Image/File)

  • The image editor shows for Image fields, and for File fields only when the referenced asset is an image; hidden when it is not; empty fields never show it.
  • Detection is centralized in isImageFile() (in the image-editor lib) using the authoritative isImage flag then the image/* content type. Extension sniffing is intentionally avoided — the mime type is reliably present on the metadata the gate runs against.
  • Restricted to the new Angular Edit Content — Image/File never expose the editor in the legacy Dojo host (gated on the Angular launcher). Binary is unchanged and keeps its legacy fallback.

Save (provider per input type)

  • The "apply edited image" step is selected by ImageEditSaveStrategy:
    • Binary: applies the edit inline (existing behavior).
    • Image/File: checks in and publishes a new version of the referenced dotAsset via 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 the asset field; the check-in resolves it server-side.
  • Adds DotWorkflowActionsFireService.publishContentletByIdentifier with optional language support (?language=).

Edited-image robustness

  • The Save servlet can return the edited temp file as 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

  • Image editor available in Image fields
  • Image editor available in File fields only when the file is an image
  • Image editor hidden in File fields when the file is not an image
  • Binary field behavior unchanged
  • Saving an edit creates and publishes a new version of the referenced file-asset (verified: fire/PUBLISH → 200)
  • After saving, the field reflects the updated image without changing the field value
  • Editing from Image and File both update the same referenced asset (same asset field / identifier)

Notes for reviewers

Pending / follow-up

  • No-op save guard (skip the check-in when nothing changed)
  • Specific "no edit permission" message (currently a generic server-error message)
  • "Saving" indicator during the fire
  • Integration / Postman coverage for the versioned save path

Test plan

  • Unit: 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.
  • Manual E2E in the new Edit Content: pencil shows on Image and image-File fields (hidden on non-image File); editing beach.png on an Image field flips + saves; 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.

oidacra added 2 commits July 2, 2026 15:08
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
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🤖 dotBot Review (Bedrock)

Reviewed 17 file(s); 12 candidate(s) → 8 confirmed, 0 uncertain (unverified, kept for review).

⚠️ Coverage capped: 0 file(s) + 1 lower-severity candidate(s) skipped (limits: 40 files, 12 candidates).

Confirmed findings

  • 🔴 Critical core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.ts:239 — Data truncation in publishContentletByIdentifier omits required 'asset' field
    The method publishContentletByIdentifier constructs the request body with only 'identifier' and 'language', omitting other fields like 'asset' which are required for the server to process the edited image. The PR description states the 'asset' field (containing the temp file ID) must be included in the PUBLISH action to save the new version. Without this field, the server cannot apply the image edits, breaking core functionality.
  • 🟠 High core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.ts:435 — Missing error handling for image editor operations
    The call to store.applyEditedImage(editedImage) at line 435 in dot-file-field.component.ts lacks error handling. The removed #applyEditedImage method previously included a catchError operator to handle failures, but the current implementation does not have equivalent error handling. This could result in uncaught exceptions and silent failures when image editing operations fail.
  • 🟠 High core-web/libs/image-editor/src/lib/utils/enrich-edited-image.util.ts:36 — Server metadata overrides enforced isImage and focalPoint
    The code spreads tempFile.metadata after setting isImage and focalPoint, allowing server-provided metadata to overwrite these values. This can revert isImage to false or use an outdated focal point. The properties should be set after spreading to ensure correct values.
  • 🟠 High core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts:18 — Incorrect handling of isImage flag in isImageFile utility
    The code checks if isImage is truthy (line 18), but when isImage is explicitly false, it falls back to content type check. This could misclassify non-image files as images if the server's authoritative isImage: false is overridden by a matching content type. The correct behavior is to respect isImage: false and return false without checking content type.
  • 🟡 Medium core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.spec.ts:238 — Type safety bypass in test setup
    Using as never when setting component inputs in the test bypasses TypeScript checks, potentially allowing invalid props. The test initializes component.field and component.contentlet with mocks, but the forced type assertion could mask mismatches between mock data and actual input interfaces.
  • 🟡 Medium core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/dot-edit-content-file-field.component.ts:32 — Singleton service provided at component level
    DotWorkflowActionsFireService is declared with providedIn: 'root' in its @Injectable decorator (dot-workflow-actions-fire.service.ts:8), but also listed in component providers (dot-edit-content-file-field.component.ts:32). This creates separate instances when the component is used multiple times, potentially breaking singleton expectations.
  • 🟡 Medium core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/store/file-field.store.spec.ts:295 — Incorrect legacy FileAsset simulation in test
    The test uses 'titleImage' property to mock a legacy FileAsset, but code comments and implementation indicate legacy FileAssets use 'fileAsset' field. This creates a test gap where legacy asset detection logic might not be properly validated, as evidenced by code comments stating: 'for a dotAsset, fileAsset for a legacy FileAsset' in file-field.store.ts:369.
  • 🟡 Medium core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/store/file-field.store.spec.ts:360 — Test does not verify upload status reset on error
    The test 'surfaces a server error when the launcher stream fails' in file-field.store.spec.ts checks the error message but does not assert that uploadStatus is reset from 'UPLOADING' to 'IDLE' or 'ERROR' after the failure. The store's error handling should reset the upload state to prevent the UI from showing an incorrect persistent loading state.

us.deepseek.r1-v1:0 · Run: #28679330603 · tokens: in: 132137 · out: 33637 · total: 165774 · calls: 42 · est. ~$0.360

oidacra added 2 commits July 2, 2026 15:17
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
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
@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @oidacra's task in 2m 6s —— View job


Rollback-Safety Analysis

  • Read docs/core/ROLLBACK_UNSAFE_CATEGORIES.md
  • Get full PR diff (25a3bba2...4bd85a4d)
  • Analyze diff against every unsafe category
  • Post verdict (label applied)

Verdict: ✅ Safe to Rollback

All 17 changed files are frontend-only (Angular/TypeScript, core-web/libs/**):

  • dot-workflow-actions-fire.service.ts — adds an optional language param to publishContentletByIdentifier(), sent as ?language= on the existing PUT /api/v1/workflow/actions/default/fire/PUBLISH endpoint. The language query param is already handled server-side (WorkflowResource.java, @DefaultValue("-1") @QueryParam("language")), so this is an additive, backward-compatible use of an existing contract — not a new one. N-1's frontend doesn't send it, and the server already defaults it — no break either direction.
  • file-field.store.ts / dot-file-field.component.ts / dot-edit-content-file-field.component.ts — new save-strategy routing and publishEditedAsset method, all client-side state/UI logic.
  • image-editor lib (is-image-file.util.ts, enrich-edited-image.util.ts, with-save.feature.ts) — pure client-side detection/enrichment utilities.
  • No *.java, *.sql, or runonce task files are touched anywhere in the diff (confirmed via git diff ... -- '*.java' '*.sql' → empty).
  • No ES mapping, contentlet_as_json model version, table/column/PK changes, storage provider changes, or OSGi/VTL contract changes.

Since N-1 has no code path that depends on anything introduced here, and the only server interaction reuses an existing, already-optional endpoint parameter, rolling back to the previous release requires no manual DB/ES intervention.

Label AI: Safe To Rollback has been applied. No unsafe-rollback comment needed.

- 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
@oidacra oidacra marked this pull request as ready for review July 2, 2026 20:04
Comment thread core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts
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
Comment thread core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts
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
Comment thread core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts
…eldStore is

FileFieldStore now injects DotWorkflowActionsFireService at construction, so every
injector that provides the store must satisfy that dependency. The wrapper field
component and its spec provided FileFieldStore without it, breaking the full
edit-content unit suite in CI (NG0201). Provide the service alongside the store
(component + spec) and assert the edit flow via the store's applyEditedImage
entry point.

Refs #36363
@oidacra oidacra self-assigned this Jul 3, 2026
Comment thread core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts
Comment thread core-web/libs/image-editor/src/lib/utils/is-image-file.util.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Enable image editor in Image and File fields with versioned save to the referenced content

1 participant