Skip to content

refactor: Modularize AEM Asset Selector into testable helpers with explicit repository mode support#841

Merged
auniverseaway merged 19 commits intoadobe:mainfrom
ravuthu:asset-selector-refactor
Mar 23, 2026
Merged

refactor: Modularize AEM Asset Selector into testable helpers with explicit repository mode support#841
auniverseaway merged 19 commits intoadobe:mainfrom
ravuthu:asset-selector-refactor

Conversation

@ravuthu
Copy link
Copy Markdown
Contributor

@ravuthu ravuthu commented Mar 13, 2026

Description

Problem

The existing da-assets.js was a single ~250-line function (openAssets) that
mixed config loading, URL construction, ProseMirror insertion, smart crop UI,
approval checking, and DOM management. This made it difficult to maintain,
extend, or test. It also had a correctness bug: DM Open API delivery assets use
repo:assetId and repo:repositoryId in their response, but the old code
incorrectly read repo:id (the author-tier field) for URL construction.

Changes

Refactored blocks/edit/da-assets/

Split the monolith into four focused helper modules:

  • helpers/config.js — config fetch/cache, getConfKey, getRepositoryConfig,
    getResponsiveImageConfig. getRepositoryConfig is the new central function
    that resolves the three repository modes from the DA site config keys.

  • helpers/urls.js — three explicit URL builders, one per mode:

    • buildAuthorUrl — author tier, publish URLs via asset.path
    • buildDmUrl — author tier + DM delivery, uses correct asset['repo:id']
    • buildDeliveryUrl — delivery tier (DM Open API), uses repo:assetId /
      repo:repositoryId / repo:name per the AEM Open API spec
    • All three builders correctly route images, videos (/play), and other
      formats (PDFs, CSVs → /original/as/) to the right URL pattern.
  • helpers/insert.js — ProseMirror helpers: insertImage, insertLink,
    insertFragment, createImageNode, findBlockContext, getBlockName.

  • helpers/smart-crop.js — self-contained smart crop dialog with clean
    callbacks (onInsert, onBack, onCancel). Refactored crop list click
    handler from li:hover to e.target.closest('li') for reliability.

da-assets.js is now a thin ~50-line orchestrator. The NX/IMS dynamic imports
are lazy (inside openAssets) so the module is importable in unit tests.

Bug fixes

  • Approval check in author+DM mode changed from && to ||: an asset now
    requires both status === 'approved' and
    activationTarget === 'delivery' to pass, not just one of them.
  • Videos in author+DM mode now use the /play endpoint (same as delivery tier)
    instead of /original/as/.
  • Non-image/non-video files (PDFs, CSVs) in both DM modes now use the
    /original/as/ path per the AEM Delivery API spec.
  • getConfKey is no longer re-exported from da-assets.js; da-library now
    imports it directly from helpers/config.js.

Tests (test/unit/blocks/edit/da-assets/)

Added 5 new test files with 462 tests, bringing helper coverage to 100%:

File Coverage
helpers/config.js 100%
helpers/urls.js 100%
helpers/insert.js 100%
helpers/smart-crop.js 100%
da-assets.js 74% (openAssets excluded — IMS dependency)

Configuration reference

All keys from https://docs.da.live/administrators/guides/setup-aem-assets
are supported: aem.repositoryId, aem.assets.prod.origin,
aem.assets.image.type, aem.asset.dm.delivery, aem.asset.smartcrop.select,
aem.assets.renditions.select (reserved for future use).

Related Issue

#842

Motivation and Context

How Has This Been Tested?

Tested using Chrome Dev Tools override option.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Comment thread blocks/edit/da-assets/helpers/config.js Outdated
Comment thread blocks/edit/da-assets/helpers/urls.js Outdated
*/
export function buildDmUrl(asset, dmOrigin) {
const mimetype = asset.mimetype || asset['dc:format'] || '';
const base = `https://${dmOrigin}/adobe/assets/${asset['repo:id']}`;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could we enable ${dmOrigin.includes('/') ? '' : '/adobe/assets/'} again in all places where /adobe/assets/ is appended/hardcoded? It enables that if aem.assets.prod.origin is used the base path can be overwritten:

  • aem.assets.prod.origin=example.org -> DM path is https://example.org/adobe/assets/urn:aaid:aem:...
  • aem.assets.prod.origin=example.org/custom/path/ -> DM path is https://example.org/custom/path/urn:aaid:aem:...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I expect aem.assets.prod.origin to contain just the custom origin and not the base path. Even if it contains /, it doesn't guarantee that the base path is /adobe/assets/ IMHO. If we think that making the base path configurable adds value, I can create a new configuration for the base path.

@auniverseaway , thoughts?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we support it today and I'll need it for my client, the idea was that as soon as an origin with path is configured it's up to the project to make sure the right base path is present

Copy link
Copy Markdown
Contributor Author

@ravuthu ravuthu Mar 16, 2026

Choose a reason for hiding this comment

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

If it is just one site, and I create a new configuration for the base path, you can then add the base path config for that site. Will that work? Because the configuration for aem.assets.prod.origin was meant to load the assets from a custom domain. Allowing the base path would not be ideal and may create confusion if adopted by more customers IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added support for custom base path configuration aem.assets.prod.basepath. Defaults to '/adobe/assets' if not configured.

Comment thread blocks/edit/da-assets/helpers/urls.js Outdated
Comment thread blocks/edit/da-assets/helpers/urls.js Outdated
if (mimetype.startsWith('image/')) {
return `${base}/as/${asset.name}`;
}
if (mimetype.startsWith('video/')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this will change how video URLs are handled currently, I think we need a config flag here:

  • default can be ${base}/play to get the video player URL
  • if "disable video player" flag is set: ${base}/original/as/${asset.name} to enable direct URL with filename

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of having a configuration to deliver the original file for videos. I can implement one.

@auniverseaway do you agree?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe this can be combined with #832, looks like if that configuration can be customised we can handle also this case

Copy link
Copy Markdown
Contributor Author

@ravuthu ravuthu Mar 17, 2026

Choose a reason for hiding this comment

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

This has been implemented through aem.asset.mime.renditions configuration. Images will be rendered in .avif format, videos use /play and all other files will be loaded as original by default and can be overridden by configuring the aem.asset.mime.renditions as image/*:original,video/*:original

Copy link
Copy Markdown
Member

@auniverseaway auniverseaway left a comment

Choose a reason for hiding this comment

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

This is really great stuff. Thanks for this.

Copy link
Copy Markdown

@sagarsane sagarsane left a comment

Choose a reason for hiding this comment

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

❤️ 🎉 🚀

@auniverseaway auniverseaway merged commit 2d37fac into adobe:main Mar 23, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants