Skip to content

37132 new assetpicker must not be used in the legacy dojo editor pick the picker per host wysiwyg block editor fileimage - #37134

Open
nicobytes wants to merge 7 commits into
mainfrom
nicobytes/37132-new-assetpicker-must-not-be-used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage
Open

37132 new assetpicker must not be used in the legacy dojo editor pick the picker per host wysiwyg block editor fileimage#37134
nicobytes wants to merge 7 commits into
mainfrom
nicobytes/37132-new-assetpicker-must-not-be-used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage

Conversation

@nicobytes

@nicobytes nicobytes commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This pull request introduces a new Angular-based Asset Picker for file and image selection in the Edit Content experience, ensuring it is only available in modern Angular hosts while maintaining the legacy picker for older or custom element hosts. It also adds comprehensive tests to verify both picker behaviors. The changes ensure a seamless transition between the new and legacy pickers depending on the hosting context, and provide clear dependency injection for the new picker.

Asset Picker Integration (Angular Hosts):

  • Registers the new AngularAssetPickerLauncher as the provider for ASSET_PICKER_LAUNCHER in all Angular Edit Content hosts (dot-create-content-dialog, side panel, and shell), ensuring the new Asset Picker is used in these contexts. [1] [2] [3]
  • Updates imports and providers to include ASSET_PICKER_LAUNCHER and AngularAssetPickerLauncher where needed. [1] [2] [3] [4] [5]

Legacy Picker Preservation (Custom Element/Legacy Hosts):

  • Modifies DotFileFieldComponent to inject ASSET_PICKER_LAUNCHER optionally; if absent, falls back to the legacy DotBrowserSelectorComponent for file selection, maintaining previous behavior in legacy hosts. [1] [2] [3]
  • Adds a dedicated test suite (dot-file-field.component.legacy-picker.spec.ts) to verify the legacy picker is used and behaves as expected when the new launcher is not provided.

Testing and Verification:

  • Expands the main file field component test to verify the new Asset Picker is opened and correctly scoped when the launcher is present (Angular hosts).
  • Ensures both picker paths prevent multiple dialogs and handle dialog closure and field disabling correctly.

Project Configuration:

  • Adds a feature directory entry for tracking this work in .specify/feature.json.

Closes #37132

@nicobytes
Add per-host asset picker launcher so legacy Dojo editors keep the brows
956b2fa 
Introduces ASSET_PICKER_LAUNCHER (AngularAssetPickerLauncher) as an optional DI
token so the new DotAssetPickerComponent only opens in Angular Edit Content
hosts. Legacy Dojo hosts (binary field, block editor, WYSIWYG plugin), where
the token is absent, fall back to the pre-existing DotBrowserSelectorComponent
/ DotAssetSearchDialogComponent, since the new picker was never designed for
those hosts.
@nicobytes
Add per-host asset picker launcher so legacy Dojo editors keep the brows
4d1ffe8 
Introduces ASSET_PICKER_LAUNCHER (AngularAssetPickerLauncher) as an optional DI
token so the new DotAssetPickerComponent only opens in Angular Edit Content
hosts. Legacy Dojo hosts (binary field, block editor, WYSIWYG plugin), where
the token is absent, fall back to the pre-existing DotBrowserSelectorComponent
/ DotAssetSearchDialogComponent, since the new picker was never designed for
those hosts.
Copilot AI lite review requested due to automatic review settings August 20, 2026 14:57
@claude

claude Bot commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @nicobytes's task in 3m 20s —— View job

Claude Code Review

I reviewed the picker-per-host DI seam, both fallback paths, the launcher/token, and the guard/teardown logic across all three entry points. Findings below.

New Issues

  • 🟡 Medium: core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.ts:943-947 — The legacy browser-selector dialog title uses message keys dot.file.field.dialog.select.existing.image.header and dot.file.field.dialog.select.existing.file.header, but neither key exists in dotCMS/src/main/webapp/WEB-INF/messages/Language.properties (only ...select.existing.file.table.* / ...actions.* keys are defined). DotMessageService.get() returns the key verbatim when a message is missing (dot-messages.service.ts:61), so in the legacy Binary Field host — the exact host this PR restores the old picker for — the dialog header renders the literal string dot.file.field.dialog.select.existing.file.header instead of a label. The equivalent block-editor keys (dot.block-editor.extension.*.dotcms.dialog-title) do exist, so only the file-field path is affected. Add the two keys to Language.properties (and any translated variants). Fix this →

Existing (non-blocking, prior reviewer suggestions still open)

  • 🟡 Medium: core-web/libs/new-block-editor/src/lib/editor/services/editor-modal.service.legacy-host.spec.ts — @oidacra's suggestion to assert the full config (toEqual(buildBrowserSelectorConfig(...))) rather than a 4-field subset is still open. Since "restore the config exactly" is an acceptance criterion, a dropped browse flag would currently pass. Worth tightening but not blocking.
  • 🟡 Medium: legacy-host specs use { ... } as DotCMSContentlet / as DotSite casts — @oidacra suggested createFakeContentlet / createFakeSite from @dotcms/utils-testing to stay in sync as those interfaces grow. Minor.

Resolved

  • ✅ dot-wysiwyg-plugin.service.ts — trackImagePicker's onClose is now held in pickerCloseSub and cancelled in ngOnDestroy before close(); the previously-leaked subscription is gone.
  • ✅ dot-wysiwyg-plugin.service.ts:ngOnDestroy — now resets imagePickerBusy = false, so the button can't get stuck.
  • ✅ dot-file-field.component.ts / dot-wysiwyg-plugin.service.ts — the synchronous legacy branch that sets the pending/busy guard before the site lookup is now wrapped in #releasePendingIfOpenThrows / releaseBusyIfOpenThrows (commit e005f05), so a throw from open() no longer leaves the button dead for the session.
  • ✅ config.utils.ts — the buildBrowserSelectorConfig doc now explicitly calls out the intentional overflow: 'hidden' / min(45rem, 80vh) differences from the file field's config rather than claiming it "mirrors" it.
  • ✅ dot-wysiwyg-plugin.service.ts doc block — softened to "No legacy host is currently known to construct this service…", matching the spec's Assumptions section.

Notes (not blocking)

  • dot-wysiwyg-plugin.service.ts openLegacyImageDialog uses a hardcoded header: 'Insert Image' rather than a message key. Per the spec this path is unreachable today (no legacy host constructs this service), so this is cosmetic, but if that assumption ever changes the header won't be translated.

The core design — an optional ASSET_PICKER_LAUNCHER token provided only by the three Angular hosts, launcher borrowing the caller's component-scoped DialogService, and both paths converging on a shared trackPicker — is sound and well-documented. Both fallback imports (DotBrowserSelectorComponent, DotAssetSearchDialogComponent) are correctly exported from @dotcms/ui. The only functional gap is the two missing i18n keys.
· nicobytes/37132-new-assetpicker-must-not-be-used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage

Copilot AI reviewed Aug 20, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses regression #37132 by introducing a host-capability DI seam so asset selection opens the new DotAssetPickerComponent only in the Angular Edit Content hosts, while legacy Dojo/JSP-mounted custom elements fall back to the pre-#36944 pickers.

Changes:

  • Added ASSET_PICKER_LAUNCHER injection token + AngularAssetPickerLauncher implementation in @dotcms/ui and exported them from the UI barrel.
  • Rewired the three entry points (Story Block, WYSIWYG “Add image”, File/Image “Select Existing File”) to choose new vs legacy picker based on optional token presence, and restored legacy browser-selector config for the block editor.
  • Added/updated unit tests to cover both token-present (Angular host) and token-absent (legacy host) branches; updated docs/spec-kit metadata.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
specs/37132-picker-per-host/spec.md Spec-kit issue resolution spec describing host-based picker selection and acceptance criteria.
core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.legacy-host.spec.ts Updates test documentation to reflect picker no longer opened from legacy host while remaining a dependency-graph guard.
core-web/libs/ui/src/lib/components/dot-asset-picker/asset-picker-launcher.token.ts Introduces ASSET_PICKER_LAUNCHER token + launcher contract for host-driven picker selection.
core-web/libs/ui/src/lib/components/dot-asset-picker/angular-asset-picker.launcher.ts Implements launcher that opens DotAssetPickerComponent via the caller-provided DialogService.
core-web/libs/ui/src/lib/components/dot-asset-picker/angular-asset-picker.launcher.spec.ts Unit tests for launcher behavior/config translation and override handling.
core-web/libs/ui/src/index.ts Exports the new token + launcher from @dotcms/ui.
core-web/libs/new-block-editor/src/lib/editor/services/editor-modal.service.ts Chooses new picker vs legacy browser selector by optional ASSET_PICKER_LAUNCHER; avoids site lookup on legacy path.
core-web/libs/new-block-editor/src/lib/editor/services/editor-modal.service.spec.ts Updates Angular-host tests to provide ASSET_PICKER_LAUNCHER.
core-web/libs/new-block-editor/src/lib/editor/services/editor-modal.service.legacy-host.spec.ts New legacy-host tests asserting browser selector usage and no site lookup when token is absent.
core-web/libs/new-block-editor/src/lib/editor/config.utils.ts Restores legacy browser-selector dialog config builder for the block editor.
core-web/libs/new-block-editor/CLAUDE.md Updates library documentation to describe host-based picker selection and site-lookup behavior.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.ts Adds optional token gating and legacy fallback dialog; adds teardown via OnDestroy.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.spec.ts Updates Angular-host tests to provide ASSET_PICKER_LAUNCHER.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.legacy-host.spec.ts New legacy-host tests validating legacy dialog, focus behavior, and teardown when token is absent.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/dot-edit-content-file-field.component.spec.ts Updates Angular-host tests to provide ASSET_PICKER_LAUNCHER.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.ts Uses optional token to select new picker vs legacy DotBrowserSelectorComponent and skips site lookup on legacy path.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.spec.ts Adds/adjusts Angular-host tests validating the new picker branch.
core-web/libs/edit-content/src/lib/fields/dot-edit-content-file-field/components/dot-file-field/dot-file-field.component.legacy-picker.spec.ts New legacy-host tests validating browser selector branch and no site lookup when token is absent.
core-web/libs/edit-content/src/lib/edit-content.shell.component.ts Provides ASSET_PICKER_LAUNCHER in the full-screen Edit Content host.
core-web/libs/edit-content/src/lib/components/dot-edit-content-side-panel/dot-edit-content-side-panel.component.ts Provides ASSET_PICKER_LAUNCHER in the side-panel Edit Content host.
core-web/libs/edit-content/src/lib/components/dot-create-content-dialog/dot-create-content-dialog.component.ts Provides ASSET_PICKER_LAUNCHER in the dialog Edit Content host (UVE/Relationship).
.specify/feature.json Points Spec-Kit to the feature directory for this work.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread core-web/libs/new-block-editor/src/lib/editor/config.utils.ts Outdated
Comment thread ...c/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.ts
Comment thread ...c/lib/fields/dot-edit-content-wysiwyg-field/dot-wysiwyg-plugin/dot-wysiwyg-plugin.service.ts
@github-actions github-actions Bot added the Area : Frontend PR changes Angular/TypeScript frontend code label Aug 20, 2026
@github-actions github-actions Bot mentioned this pull request Aug 20, 2026
Open
22 tasks
@nicobytes
Merge branch 'main' into nicobytes/37132-new-assetpicker-must-not-be-…
4aacbdd 
…used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage
@nicobytes @claude
fix(edit-content): cancel the WYSIWYG picker close handler before tea…
2310904 
…rdown closes it

`DynamicDialogRef.close()` pushes through `onClose` synchronously
(`_onClose.next(result)`; the close animation only gates `destroy()`), so the
handler ran inline inside `ngOnDestroy` and called `editor.focus()` on a TinyMCE
instance `DotWysiwygTinymceComponent.ngOnDestroy` may already have `remove()`d.

The PR review suggested `takeUntilDestroyed`, but that gives no ordering
guarantee against this very hook — if `ngOnDestroy` runs first the subscription
is still live and the handler fires anyway. So the `Subscription` is held and
unsubscribed *before* the dialog is closed, which is deterministic regardless of
hook order and also covers the leaked-subscription concern. Suppressing the
handler means nothing else clears `imagePickerBusy`, so teardown clears it.

Two tests pin this, using a ref stub whose `close()` emits synchronously like
the real one.

Also rewords `buildBrowserSelectorConfig`'s JSDoc. It claimed the config mirrors
the File field's, but this builder keeps `style.overflow: 'hidden'` and a
viewport-clamped `min-height` because the editor opens it from inside its
full-screen shell. Both builders are faithful restores of their own host's
pre-#36944 config — the comment, restored verbatim along with the function, was
the part that was wrong.

Addresses review feedback on #37134.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nicobytes
Merge branch 'main' into nicobytes/37132-new-assetpicker-must-not-be-…
21246af 
…used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage
oidacra
oidacra reviewed Aug 24, 2026
View reviewed changes
Comment thread core-web/libs/ui/src/lib/components/dot-asset-picker/angular-asset-picker.launcher.spec.ts Outdated
@nicobytes
Merge branch 'main' into nicobytes/37132-new-assetpicker-must-not-be-…
862d762 
…used-in-the-legacy-dojo-editor-pick-the-picker-per-host-wysiwyg-block-editor-fileimage
@nicobytes @claude
fix(edit-content): release the picker guard when opening throws
e005f05 
The legacy branches added by this PR set their busy/pending guard and then open
synchronously, but the guard is only ever cleared by the picker's `onClose`. An
open that throws never wires one, so the flag stayed `true` and "Select Existing
File" / the WYSIWYG *Add image* button were dead for the rest of the session,
with no toast and no log. Both entry points now release the guard and rethrow —
swallowing would leave a button that silently does nothing.

Also from review:

- Soften the `DotWysiwygPluginService` doc blocks. They asserted as settled fact
  that legacy Dojo pages construct the service, which contradicts the spec's own
  Assumptions: the Dojo editor renders WYSIWYG as a textarea plus JSP-side
  TinyMCE and no Angular custom element exists for the field, so the fallback is
  a consistency guard, not a live regression path.

- Add `config.utils.spec.ts`, pinning `buildBrowserSelectorConfig`'s restored
  values — every browse flag, the sizing, and the fullscreen-clearing
  `baseZIndex`. Restoring that config exactly is an acceptance criterion of
  #37132 and nothing covered it directly. Verified by mutation: dropping
  `showWorking` fails this spec.

- Assert the whole config object in `editor-modal.service.legacy-host.spec.ts`
  instead of four fields, which pins that the service delegates to the builder
  with the right arguments. Note this cannot catch a field dropped from the
  builder — both sides move together — which is why the spec above exists.

- Use the shared `createFakeContentlet` / `createFakeSite` factories instead of
  a cast literal and a hand-built `DotSite`.

Addresses review feedback on #37134.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nicobytes
nicobytes enabled auto-merge August 24, 2026 16:56
@nicobytes
nicobytes added this pull request to the merge queue Aug 24, 2026
@nicobytes
nicobytes removed this pull request from the merge queue due to a manual request Aug 24, 2026
@nicobytes
nicobytes added this pull request to the merge queue Aug 24, 2026
@nicobytes
nicobytes removed this pull request from the merge queue due to a manual request Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oidacra oidacra oidacra left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@adrianjm-dotCMS adrianjm-dotCMS adrianjm-dotCMS approved these changes

Assignees

No one assigned

Labels

Area : Frontend PR changes Angular/TypeScript frontend code

Projects

dotCMS - Product Planning
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New AssetPicker must not be used in the legacy Dojo editor — pick the picker per host (WYSIWYG, Block Editor, File/Image)

4 participants

@nicobytes @oidacra @adrianjm-dotCMS