Issue 36702 asset picker - #36944
Open
nicobytes wants to merge 30 commits into
Open
Conversation
- Extracted DotFolderListView component and related models/constants/mocks into the new `@dotcms/ui` library. - Updated imports in the Content Drive shell to reference the new location. - Introduced `selectionMode` input to support both single and multiple selection. - Re-exported necessary symbols from `@dotcms/ui` in the Content Drive UI for compatibility. - Added tests for the new component and selection mode functionality. This migration aims to share the folder/asset list component between Content Drive and future AssetPicker implementations, enhancing code reuse and maintainability.
18 tasks
github-actions
Bot
added
the
Area : Frontend
Contributor
|
Claude finished @nicobytes's task in 2m 40s —— View job Code Review — AssetPicker PRI focused on the AssetPicker store/features, the lifted folder-tree utilities, and the File/Image field wiring, and rechecked the earlier Copilot findings against the current diff. New Issues
Existing
Resolved
Nice work overall — the store composition ordering, cursor-invalidation on sort/page-size change, and the error-before-success ordering in the folder-tree feature are all handled carefully. The one blocking-ish item is the missing · branch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the existing Folder List View used by Content Drive into a reusable presentational component in @dotcms/ui, adding a single-selection mode intended for the upcoming AssetPicker (while preserving current Content Drive behavior via the default multiple selection mode).
Changes:
- Moved Folder List View domain-agnostic types/constants into
@dotcms/uiand re-exported them from the Content Drive UI package for compatibility. - Added
selectionMode: 'single' | 'multiple'support to the table (checkboxes in multiple mode, radios in single mode) and normalized emitted selections to an array. - Updated Content Drive shell imports and updated/extended unit tests accordingly.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core-web/libs/ui/src/lib/components/dot-folder-list-view/models.ts | Introduces shared column typing and selectionMode model for the Folder List View. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/constants.ts | Defines header column config and drag MIME type local to the component folder. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/mocks.ts | Moves test mocks alongside the component. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.ts | Adds selectionMode input, normalizes selection output, and updates internal imports to local UI sources. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.html | Switches checkbox vs radio rendering based on selectionMode and updates selection binding. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.scss | Fixes relative SCSS imports to match the libs layout. |
| core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.spec.ts | Updates tests for the new selection model and adds coverage for single-selection behavior. |
| core-web/libs/ui/src/index.ts | Exposes Folder List View component + related models/constants from @dotcms/ui. |
| core-web/libs/portlets/dot-content-drive/ui/src/lib/shared/models.ts | Removes Folder List View column typing now owned by @dotcms/ui. |
| core-web/libs/portlets/dot-content-drive/ui/src/lib/shared/constants.ts | Removes list-view constants now owned by @dotcms/ui. |
| core-web/libs/portlets/dot-content-drive/ui/src/index.ts | Re-exports the Folder List View API surface from @dotcms/ui for Content Drive consumers. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts | Updates imports to use @dotcms/ui for the presentational list component/types. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts | Aligns test imports with the updated component export location. |
- Introduced new utility functions for managing folder hierarchies, including `getFolderHierarchyByPath` and `getFolderNodesByPath`, to improve folder navigation and loading in the content drive. - Added `folder-tree-load.utils.ts` and `folder-tree.utils.ts` files to encapsulate the new logic. - Implemented comprehensive unit tests for the new utilities to ensure functionality and reliability. - Updated existing services to utilize the new utilities, enhancing code organization and maintainability. These changes aim to streamline folder management and improve the user experience in the content drive interface.
Extracts the content-type, language, and search filter components (plus the chip-filter/list-item primitives and upload button) out of the content-drive portlet into @dotcms/ui so they can be shared with the AssetPicker. Store- specific logic stays behind thin adapter components in the portlet.
- serve target lacked a dependsOn, so dotcms-webcomponents could be stale or missing when dotcms-ui starts serving - webcomponents build target was missing outputs, preventing Nx from caching/detecting its build artifacts correctly
- Introduces DotAssetPickerStore in @dotcms/ui to power the upcoming AssetPicker dialog with a search request builder mirroring Content Drive's, but with no router/URL coupling so it can run inside a dialog over Edit Contentlet without corrupting host navigation. - Relocates ALL_FOLDER/SYSTEM_HOST_ID out of the Content Drive UI library into shared dot-folder-tree constants so both Content Drive and the new picker consume a single source.
…o issue-36702-asset-picker
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 78 changed files in this pull request and generated no new comments.
Suppressed comments (1)
core-web/libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-folder-tree.feature.ts:97
loadFolderssetsfoldersStatustoERRORincatchError, but then thesubscribeblock unconditionally patches it back toLOADED(becausecatchErrorreturns an empty array). This masks folder-tree failures and makes the UI indistinguishable from a successful empty tree.
The AssetPicker (browse/pick a single asset) now composes the dropzone, upload-type selector, and folder sidebar/toolbar that Content Drive already had, so both features share one implementation instead of duplicating upload flow logic. - Move `dot-content-drive-dropzone` and the upload-type-selector dialog out of the Content Drive portlet into `@dotcms/ui` as `DotUploadDropzoneComponent` and `DotUploadTypeSelectorComponent`, decoupled from `DotContentDriveStore` (folder/drag-state now passed via inputs/outputs) - Add `DotAssetPickerComponent` with sidebar/toolbar subcomponents, wiring the shared dropzone, upload selector, and folder tree to a new `DotAssetPickerStore` - Update Content Drive shell to consume the relocated shared components and derive drag/target-folder state locally
Adds a global last-used-path store, a config builder that translates Edit Content field type (File/Image) into picker filters, and server-side base-type narrowing for the content type filter so restricted hosts don't page through mostly-discarded results.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 101 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
core-web/libs/ui/src/lib/components/dot-asset-picker/components/dot-asset-picker-toolbar/dot-asset-picker-toolbar.component.ts:58
- This returns
nullfor File mode because that config intentionally has no preselected base types, butnullmakes the selector offer every base type, including Content and Widget. The acceptance criteria restrict the selector in both File and Image modes; selection and allowed options must be separate so File starts with no selection while still only offering DOTASSET/FILEASSET.
core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html:22 - Double-click only selects the row, whereas the AssetPicker acceptance criteria require it to select and confirm. Note that
DotFolderListViewComponent.doubleClickis also emitted by single clicks on the title/thumbnail, so directly callingconfirm()here would close on a single click; introduce a distinct actual-double-click confirmation path without restoring editor navigation.
core-web/libs/data-access/src/lib/dot-folder/folder-tree.utils.ts:96 - This predicate is true for every valid level (
length >= index + 1), so every node on the target path is marked as a leaf even when child levels are attached below it. PrimeNG then hides the expansion control, making expanded ancestors impossible to reopen after collapse. Only the final hierarchy level should be marked as a leaf.
const isLeaf = (levelIndex: number) => folderHierarchyLevels.length >= levelIndex + 1;
core-web/libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-browse.feature.ts:120
- Starting a new request leaves
selectedAssetuntouched. Since the list clears its own PrimeNG selection silently whenitemschanges, changing folder/filter/page/sort can leave Confirm enabled for an asset no longer visible and return that stale asset. Clear the picker selection whenever a new browse request starts (or when its result replacesitems).
core-web/libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-folder-tree.feature.ts:83 - The error status is immediately overwritten:
catchErroremits an empty hierarchy, so the subscription runs and patchesfoldersStatustoLOADED. A failed folder request therefore looks like a successfully loaded empty tree. Stop the success callback from running for the error fallback.
- The dotAsset/File Asset picker now uses the Content Drive-backed AssetPicker instead of DotBrowserSelectorComponent, restricting by base type/mime for image fields and resolving site/locale via GlobalStore and DotEditContentStore. - Guards against opening the picker before a site has resolved. - Updates specs to mock GlobalStore's siteDetails and cover the new picker config, locale fallback, and open/close guards.
- Add `allowedBaseTypes` to the picker config so both File and Image modes restrict the content-type selector to DOTASSET/FILEASSET, independent of `baseTypes` (which only controls pre-selection). The File field previously fell back to "no restriction" and listed Widget/Content. - Reorder `withAssetSelection` before `withAssetBrowse` so a new browse result can clear a stale selection instead of leaving Confirm enabled for a row no longer in the list. - Fix folder-tree hierarchy loading to only patch LOADED state on success (in `tap`, before `catchError`), so a failed request stays in ERROR instead of looking like an empty tree. - Reset the tree highlight to the site root when a search widens the list back to site-wide, since `$targetFolder` reads the highlight to pick the upload destination.
Adds a `{ state: type<DotAssetPickerState>() }` constraint to the signal store feature so it correctly composes with `withAssetBrowse`, which now runs after it and depends on `selectedAsset`. Also fixes a Prettier import formatting issue in the file field spec.
…o issue-36702-asset-picker
- Replace the single-site folder tree with a sites-and-folders tree so users can pick assets from any site, not just the one being edited; adds sidebar search, cross-site "last used" location memory, and a custom header that drives PrimeNG's maximize state for full screen. - Extract the full-screen dialog styling shared with the image editor into `@dotcms/ui` so both consumers stay in sync.
- Introduce DotAssetPickerHeaderComponent with title, full-screen toggle, and close button wired to the picker store - Add DotAssetPickerSidebarComponent tests covering multi-site tree rendering, node selection/expansion, and load-more pagination - Extract site-tree.utils.ts with shared helpers for multi-site folder trees (pagination, load-more sentinels, site/folder lookups) used by both the AssetPicker and the legacy Browser Selector - Add fullscreen-dialog.ts constants for driving dialog full-screen state via PrimeNG's maximized class instead of inline style overrides
Widen height and narrow width for better content fit before reaching full screen.
Lazy-load tap/catchError handlers mutated the tree node captured before the request fired, but publish() replaces the whole folders array with a clone on every update, orphaning that reference — so the loading spinner and expanded state landed on a node the tree no longer rendered. mutateNode()/findNodeByKey() re-locate the target by key against the current state before each mutation, and publish() re-derives selectedNode by key so the highlight survives across a load. Also adds a showActions input to DotFolderListViewComponent to hide the kebab menu in the picker's single-selection context.
Switch the width/height caps from fixed pixels (1120px/58rem) to rem-based caps (114rem/68rem) so the dialog scales with content sizing and only clamps on large external monitors, while the viewport-relative min() still governs on typical laptop screens.
…o issue-36702-asset-picker
Previously, clearing the base-type chip (or starting with none, as File fields do) dropped the restriction entirely and let the picker list Pages and other non-asset content types. Now allowedBaseTypes acts as a floor that always constrains the request, while a user-selected baseType filter can narrow further.
- Add createDotAsset helper to seed dotAsset contentlets via a single multipart workflow call, so picker tests can seed real assets without going through the /api/v1/temp fingerprinting endpoint - Introduce AssetPickerDialog page object and wire it into file/image field specs to cover selecting, confirming, and cancelling an existing asset, plus mimetype filtering for image fields - Add data-testid="asset-picker" to the picker root so the new tests have a stable selector
| const root = site ? findSiteRoot(roots, site.identifier) : undefined; | ||
|
|
||
| if (!site || !root) { | ||
| return of({ folders: roots }); |
Member
There was a problem hiding this comment.
The site being browsed can disappear from the sidebar entirely. sitesPage() filters the sites list with the same tree search term (filter: activeSearchTerm() || '*'). If the term matches a folder name but not the site's own name/hostname, the site gets filtered out of roots, findSiteRoot returns undefined, and we fall into of({ folders: roots }) — no search runs and the current site vanishes from the tree.
Repro: while browsing "democms.com", type a folder name like "logos" that isn't part of the hostname → the site disappears and no results are shown at all, even though matching folders exist.
| folders, | ||
| selectedNode, | ||
| foldersStatus: ComponentStatus.LOADED | ||
| }) |
Member
There was a problem hiding this comment.
TreeLoadResult documents that an absent selectedNode means "leave the highlight alone" (distinct from null). But destructuring ({ folders, selectedNode }) and passing it to patchState(store, { folders, selectedNode, ... }) turns that absent key into a present key with value undefined — and patchState merges via spread ({...current, ...partial}), so it overwrites the signal to undefined anyway. The highlight gets wiped in the exact same case as the comment above, contradicting the contract documented on TreeLoadResult.
Suggestion: only include selectedNode in the patch when it's actually present in the result (same conditional-spread pattern already used in publish() further down this file).
| // Persist on confirm, not on selection: a row the user highlights and then cancels | ||
| // must not move a value shared by every picker in the system. | ||
| writeLastAssetLocation(this.#resolveAssetLocation(hydrated)); | ||
| this.#dialogRef.close(hydrated); |
Member
There was a problem hiding this comment.
This subscribe has no error handler, unlike #uploadByBaseType elsewhere in this file, which shows a toast on failure. If getContentletByInodeWithContent errors (network blip, 404, permissions changed since selection), the error is silently swallowed, the dialog never closes, and the user gets no feedback — Confirm just looks like it did nothing.
Suggestion: add an error: handler (toast + keep dialog open), matching the pattern in #uploadByBaseType.
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
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.
This pull request introduces a set of improvements and refactorings to the folder tree utilities and related data-access APIs, aimed at enhancing code reuse, maintainability, and consistency across the Content Drive and Host Folder Field features. The changes include moving folder tree logic into shared utilities, updating service providers, and aligning data models.
Core refactoring and utility extraction:
folder-tree.utils.tsandfolder-tree-load.utils.tsin@dotcms/data-access, centralizing the logic for building, loading, and paginating folder trees. This includes new functions such asgenerateAllParentPaths,createTreeNode, andbuildTreeFolderNodes, as well as utilities for paginated loading and "Load more" node handling. [1] [2]API and provider updates:
DotContentDriveServiceto use Angular'sprovidedIn: 'root'for global availability, and removed it from route-level providers. This ensures the service can be used from dialogs and other contexts without explicit injection in every route. [1] [2]Imports and dependency cleanup:
@dotcms/data-accessinstead of local utility definitions, and cleaned up duplicate or outdated imports for components such asDotFolderListViewComponent. [1] [2] [3] [4]Data model alignment:
TreeNodeContentDatamodel to include additional metadata fields (inode,defaultBaseType,fromTable) for improved folder selection and upload behavior in the Content Drive and AssetPicker.Constants and type re-exports:
@dotcms/data-access. [1] [2] [3] [4]These changes improve code maintainability, reduce duplication, and ensure that folder tree behaviors remain consistent across all consuming features.
This PR fixes: #36702
AssetPicker series — all seven sub-issues are resolved in this PR:
Fixes #36831 — AssetPicker 1/7: Migrate DotFolderListView to @dotcms/ui
Fixes #36832 — AssetPicker 2/7: Lift folder-tree helpers and harden drive data-access
Fixes #36833 — AssetPicker 3/7: Decouple Content Drive toolbar chrome for AssetPicker reuse
Fixes #36834 — AssetPicker 4/7: Headless browse store (no URL) for AssetPicker
Fixes #36835 — AssetPicker 5/7: Build DotAssetPicker shell in @dotcms/ui
Fixes #36836 — AssetPicker 6/7: File/Image entry configs, silent mime, CT limits, global last path
Fixes #36837 — AssetPicker 7/7: Wire AssetPicker into Edit Content File/Image fields