37207 migrate openbrowsermodal to the new assetpicker and delete the legacy browser selector - #37273
Conversation
I will check the main configuration directory to see if there are any custom settings, READMEs, or specific project files that provide more context for the sandbox environment. I will ask you what type of sandbox environment you would like to set up in the scratch directory. I will create a sandbox project directory and add a basic Python environment structure with a `README.md` and a starting script (`main.py`) to serve as a playground. I will write the `README.md` file without the `ArtifactMetadata` parameter to create it directly in the project sandbox directory. I will create the main entry point script `main.py` in the sandbox directory. I will run the `main.py` script using the terminal to verify the environment works correctly.
- Replace the legacy BrowserSelector interface with DotBrowserOptions/ DotBrowserHandle, backed by a site-aware asset picker instead of the old browser selector dialog. - Add a resolveSite hook to createFormBridge so the Angular host can supply the current site, since the bridge has no injector of its own. - Wire resolveSite from DotSiteService in the iframe and native custom field components. - Stub openBrowserModal in the Dojo bridge to warn and resolve null, since the legacy editor never had this dialog.
- The bridge's openBrowserModal previously piped `extensions` through to the browse config even though the browse endpoint has no such parameter, so the option was silently accepted and ignored; that test now documents the gap instead of asserting dead behavior. - Introduces `mimeTypes` on `DotAssetPickerEntryOptions`, restricted to `browse` mode (media modes still derive theirs from `ASSET_PICKER_MIME_TYPES` and must stay non-overridable). - Adds coverage for the browse endpoint dropping `links` whenever a mimetype filter is set, surfacing a console warning rather than silently returning fewer kinds than requested.
…the-new-assetpicker-and-delete-the-legacy-browser-selector
github-actions
Bot
added
the
Area : Frontend
|
Claude finished @nicobytes's task in 1m 7s —— View job SDK Compatibility Analysis — Complete
Result: No SDK breaking change detected. All 31 changed files are confined to:
There are no Java/backend changes at all in this diff (
This PR migrates the legacy |
There was a problem hiding this comment.
Pull request overview
This PR migrates DotCustomFieldApi.openBrowserModal() (Angular host) from the legacy browser selector to the new Asset Picker, expanding the picker’s domain to include menu links (and related browse-mode configuration) while making the legacy Dojo bridge explicitly unsupported via a warning + always-null handle.
Changes:
- Introduces a redesigned
openBrowserModalpublic contract (DotBrowserOptions/DotBrowserSelection/DotBrowserHandle) and wires the Angular form bridge to openDotAssetPickerComponent. - Extends Asset Picker configuration to add a
browsemode and opt-in browse flags (folders/links/version state/sort direction) while keeping existing entry points gated. - Extends Content Drive frontend models to represent menu links (
DotContentDriveLink) and link paging fields.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/37207-openbrowsermodal-assetpicker/spec.md | Adds the feature specification and acceptance criteria for the migration and browse-mode behavior. |
| specs/37207-openbrowsermodal-assetpicker/data-model.md | Documents the data model changes across API, picker config/state, and drive models. |
| specs/37207-openbrowsermodal-assetpicker/contracts/README.md | Summarizes the three contract boundaries and their compatibility rules. |
| specs/37207-openbrowsermodal-assetpicker/contracts/openbrowsermodal-public-api.md | Defines the redesigned openBrowserModal contract for VTL templates. |
| specs/37207-openbrowsermodal-assetpicker/contracts/drive-search-delta.md | Specifies the (planned) additive extensions request field for drive search. |
| specs/37207-openbrowsermodal-assetpicker/contracts/asset-picker-config.md | Specifies the additive picker config changes for browse mode + nested browse options. |
| dotCMS/src/main/webapp/WEB-INF/messages/Language.properties | Adds the browse-mode Asset Picker header translation key. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/models.ts | Adds DotAssetPickerBrowseOptions, extends paging/selection state types for mixed item kinds. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-browse.feature.ts | Starts wiring browse-mode version-state flags into the drive search request. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/dot-asset-picker.store.ts | Seeds initial sort order based on browse options. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/constants.ts | Extends default page bookmark to include folder/link cursors and hasMore* flags. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/asset-picker-config.ts | Adds browse mode, browse-only options (allowedBaseTypes, browse, mimeTypes) and gating. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/asset-picker-config.spec.ts | Adds unit coverage for browse mode + opt-in guarantees. |
| core-web/libs/edit-content/src/lib/fields/dot-edit-content-custom-field/components/native-field/native-field.component.ts | Injects DotSiteService and passes resolveSite() into createFormBridge. |
| core-web/libs/edit-content/src/lib/fields/dot-edit-content-custom-field/components/iframe-field/iframe-field.component.ts | Injects DotSiteService and passes resolveSite() into createFormBridge. |
| core-web/libs/edit-content-bridge/src/lib/interfaces/form-bridge.interface.ts | Updates the bridge API to the redesigned openBrowserModal types and re-exports. |
| core-web/libs/edit-content-bridge/src/lib/interfaces/browser-selector.interface.ts | Removes the legacy browser-selector interface contract types. |
| core-web/libs/edit-content-bridge/src/lib/interfaces/asset-browser.interface.ts | Adds the redesigned openBrowserModal types (options/selection/handle). |
| core-web/libs/edit-content-bridge/src/lib/factories/form-bridge.factory.ts | Adds resolveSite plumbing into AngularFormBridge creation. |
| core-web/libs/edit-content-bridge/src/lib/bridges/dojo-form-bridge.ts | Makes openBrowserModal explicitly unsupported in Dojo (warn + always-null handle). |
| core-web/libs/edit-content-bridge/src/lib/bridges/dojo-form-bridge.spec.ts | Adds tests for the Dojo openBrowserModal stub behavior. |
| core-web/libs/edit-content-bridge/src/lib/bridges/angular-form-bridge.ts | Refactors openBrowserModal to open the new Asset Picker and return a promise-based handle. |
| core-web/libs/edit-content-bridge/src/lib/bridges/angular-form-bridge.spec.ts | Updates tests to cover opening, option mapping, result mapping, and cancellation behaviors. |
| core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts | Adds DotContentDriveLink and link paging fields to drive request/response models. |
| .specify/feature.json | Points Spec-Kit to the new feature directory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ...(pickerConfig?.browse?.showWorking === false ? { live: true } : {}), | ||
| // Invariant, not a default: the picker's list is for assets. Folders are | ||
| // navigated through the sidebar tree. | ||
| showFolders: false |
| /** Include working (unpublished) versions. Omit for live-only. */ | ||
| showWorking?: boolean; | ||
|
|
||
| /** Include archived content. Omit to exclude it, which is the picker's standing behaviour. */ | ||
| showArchived?: boolean; |
| * Browse capabilities beyond plain asset picking — folders, menu links, version state, sorting | ||
| * and extension narrowing. |
…te-openbrowsermodal-to-the-new-assetpicker-and-delete-the-legacy-browser-selector
- The bridge's openBrowserModal previously piped `extensions` through to the browse config even though the browse endpoint has no such parameter, so the option was silently accepted and ignored; that test now documents the gap instead of asserting dead behavior. - Introduces `mimeTypes` on `DotAssetPickerEntryOptions`, restricted to `browse` mode (media modes still derive theirs from `ASSET_PICKER_MIME_TYPES` and must stay non-overridable). - Adds coverage for the browse endpoint dropping `links` whenever a mimetype filter is set, surfacing a console warning rather than silently returning fewer kinds than requested.
…etpicker-and-delete-the-legacy-browser-selector' of github.com:dotCMS/core into nicobytes/37207-migrate-openbrowsermodal-to-the-new-assetpicker-and-delete-the-legacy-browser-selector
…grate-openbrowsermodal-to-the-new-assetpicker-and-delete-the-legacy-browser-selector
github-actions
Bot
added
Area : Backend
… the code Addresses the review feedback on #37273 that is still live. All three describe behaviour the browse work replaced: - `showWorking` claimed "Omit for live-only", but omitting it applies the endpoint's default, which *includes* working versions. `false` is the only value that narrows to live-only. The doc said the opposite of what the request builder does. - `DotAssetPickerConfig.browse` still advertised "extension narrowing". `extensions` was removed when the endpoint work was split into its own issue, so the option no longer exists. - The `withAssetBrowse` header still called `showFolders: false` an "invariant". It stopped being one when folders became opt-in — and the same file already says so 100 lines further down, so it contradicted itself. Comments only; no behaviour change. Verified with `nx build dotcms-ui`, lint across 5 projects, and the picker plus Content Drive suites (1377 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| * (workflow filters, user-searchable fields) and plus the two things it needs: a silent mimetype | ||
| * restriction and `showFolders: false` as an invariant. | ||
| * restriction, and folders and menu links switched off unless the caller opts in through | ||
| * `DotAssetPickerBrowseOptions`. |
There was a problem hiding this comment.
This comment is about $totalRecords at lines 74-83 (unchanged by this PR, so anchoring here at the nearest changed line).
That computed still inspects only hasMoreContent:
return bookmark?.hasMoreContent
? limit * (page + 1)
: limit * (page - 1) + items().length;Now that folders and links are independently paginated streams, is this still sufficient? Walking a browse-mode case with limit=20 — if page 1 comes back hasMoreContent: false but hasMoreFolders: true, hasMoreLinks: true with 15 rows rendered, this returns 20*0 + 15 = 15. The paginator then sees first + rows >= totalRecords and disables Next, so the remaining folders and links look unreachable even though the server still has them.
The converse (folders/links exhaust first, content continues) looks fine, since hasMoreContent keeps it growing.
Would something like this be right?
const hasMore = bookmark
? bookmark.hasMoreContent || bookmark.hasMoreFolders || bookmark.hasMoreLinks
: true;
return hasMore ? limit * (page + 1) : limit * (page - 1) + items().length;The docstring just above ("answers is there anything past what is on screen") would want updating too, since it now speaks for three streams rather than one.
| // Three states, expressed as the two flags the picker already understands. | ||
| ...(options.status ? { showWorking: options.status !== 'live' } : {}), | ||
| ...(options.status ? { showArchived: options.status === 'archived' } : {}), | ||
| ...(options.sort ? { sortByDesc: options.sort.direction === 'desc' } : {}) |
There was a problem hiding this comment.
Only sort.direction is read here — is sort.field intentionally dropped?
DotAssetPickerBrowseOptions has no sort-field property, so there is currently no way to pass it through even if a caller sets it. That makes the drop structural rather than an oversight at this line.
The public JSDoc on openBrowserModal advertises the full shape:
sort: { field: 'modDate', direction: 'desc' },
so a template author passing sort: { field: 'title', direction: 'desc' } would reasonably expect title ordering and instead get modDate from DEFAULT_ASSET_PICKER_SORT. Both shipped VTL templates happen to pass modDate, which is already the default, so this stays invisible today.
extensions right above carries an explicit "not yet, ships with the endpoint support" note. Would it be worth giving sort.field the same treatment — either wiring it through to the store's sort state, or documenting it as direction-only for now?
| // Only a `showLinks` response carries these. Defaulting the | ||
| // cursor to the one we sent keeps it from rewinding to 0 on a | ||
| // page that did not query links. | ||
| linkCursor: response.nextLinkCursor ?? 0, |
There was a problem hiding this comment.
The comment and the code seem to disagree here. The comment says the fallback keeps the cursor "from rewinding to 0" by defaulting to "the one we sent", but the code is ?? 0 — which is the rewind.
Preserving the last-sent cursor would be ?? bookmark?.linkCursor ?? 0.
Does this matter in practice? Both hasMoreLinks and nextLinkCursor are optional on the response type, so nothing enforces that they arrive together. If a response ever reports hasMoreLinks: true without a nextLinkCursor, this stores linkCursor: 0 while hasMoreLinks stays true — the next request then re-requests links from the start while content and folder cursors keep advancing, which would surface as duplicate link rows on a later page.
I could not confirm the backend ever does that, so this may be purely a comment fix. But the one change satisfies both readings:
linkCursor: response.hasMoreLinks
? (response.nextLinkCursor ?? bookmark?.linkCursor ?? 0)
: 0,(For contrast, folderCursor on the line above needs no fallback — nextFolderCursor and hasMoreFolders are both non-optional, so that asymmetry looks deliberate and correct.)
| }, | ||
| // Nothing to browse and nothing useful to say beyond that — same as the File field, | ||
| // which simply does not open when no site resolves. | ||
| error: () => finish(null) |
There was a problem hiding this comment.
Would it be worth logging here before resolving null?
getCurrentSite() is a plain HttpClient.get() with no catchError upstream, so this branch is what runs on a 5xx, a network failure, or an expired session. As written, the template author's onClose(null) fires and that is indistinguishable from the user pressing Cancel — with nothing in the console and no signal for whoever is debugging a "the browse button does nothing" report.
The comment says there is "nothing useful to say beyond that", which is fair for the user-facing side, but the developer-facing side currently gets nothing at all:
error: (err) => {
console.error('DotCustomFieldApi.openBrowserModal: failed to resolve the current site', err);
finish(null);
}The !site branch above has the same property, though that one is at least a expected-shape outcome rather than a failure.
| return { siteId: site.identifier, hostname: site.hostname, path: item.path }; | ||
| } | ||
|
|
||
| const url = (item as DotCMSContentlet).url; |
There was a problem hiding this comment.
Is this cast still accurate? After isFolder(item) returns false on the line above, the narrowed type is DotCMSContentlet | DotContentDriveLink — and a link genuinely reaches here, via the !isContentlet(asset) early-return in confirm() which calls this function with link rows.
Both union members declare url: string, so the cast also looks unnecessary — item.url should compile directly on the union:
const url = item.url;Dropping it would mean a future browse-item variant without .url fails at compile time rather than silently reading undefined here.
| showWorking: false, | ||
| showArchived: false, | ||
| sortByDesc: true, | ||
| extensions: ['jpg'] |
There was a problem hiding this comment.
extensions is not a field on DotAssetPickerBrowseOptions — should it be in this fixture?
The interface defines five fields (showFolders, showLinks, showWorking, showArchived, sortByDesc), and this PR's own spec.md documents extensions as deliberately not exposed yet because the endpoint has no such parameter. The bridge-side comment makes the same point ("No extensions here on purpose").
The assertion still passes because excess-property checking does not apply to a variable (only to an inline object literal), and toEqual compares the same object against itself. So the test would keep passing even if browse pass-through broke for the real fields.
Worth dropping extensions: ['jpg'] so the fixture matches the contract as shipped? Otherwise the next reader could reasonably conclude it is a supported browse option today.
|
|
||
| return { | ||
| ...base, | ||
| name: row['name'] ?? row['fileName'], |
There was a problem hiding this comment.
These four values are typed unknown (from Record<string, unknown>), and the trailing as DotBrowserSelection is what makes the object literal type-check rather than any actual narrowing.
The base fields above all go through String(...), so they are guaranteed strings. These do not, yet DotBrowserSelection declares them as string | undefined — so a malformed row handing back a number or an object for baseType would flow through to consumers that read .baseType as a string with full confidence.
Would a small narrowing helper be worth it here?
const asString = (v: unknown): string | undefined =>
typeof v === 'string' ? v : undefined;then name: asString(row['name']) ?? asString(row['fileName']), and likewise for the other three — which would also let the as DotBrowserSelection cast go away.
| foldersStatus: signal(ComponentStatus.LOADED), | ||
| selectedNode: signal<{ data: unknown } | undefined>(undefined), | ||
| selectedAsset: signal<DotCMSContentlet | null>(null), | ||
| selectedAsset: signal<DotContentDriveItem | null>(null), |
There was a problem hiding this comment.
Should this mock match the widened store contract? The real DotAssetPickerSelectionState.selectedAsset became DotContentDriveBrowseItem | null in this PR, which includes DotContentDriveLink; the mock is still DotContentDriveItem | null, which does not.
That is what forces the as unknown as DotContentDriveItem double-cast on SELECTED_FOLDER and SELECTED_LINK above — and a double-cast means the type-checker can no longer catch a drift between these fixtures and what the store actually accepts.
Typing the signal as signal<DotContentDriveBrowseItem | null>(null) should let both fixtures use a single direct cast (or none, for the link, which already matches the shape).
| // exactly what it always was. | ||
| archived: browse?.showArchived ?? false, | ||
| // `live: true` means published-only. The endpoint's own default is `false` | ||
| // (working included), which is what every entry point but `browse` wants, so |
There was a problem hiding this comment.
Small wording nit: should this say openBrowserModal rather than browse?
Every sibling comment in this file draws the same contrast that way — line 118 ("every entry point but openBrowserModal uses") and line 143 ("every entry point but openBrowserModal"). Here it reads "every entry point but browse", and browse is the name of the options object (pickerConfig?.browse) rather than an entry point, so it briefly scans as a different thing.
…the-new-assetpicker-and-delete-the-legacy-browser-selector
… mode All nine findings from @oidacra's review of #37273. Two change behaviour, the rest tighten types and diagnostics. Behavioural: - `$totalRecords` only inspected `hasMoreContent`, so a page whose content stream ended while folders or links kept going reported the rows already on screen as the grand total. PrimeNG then disabled Next and those rows became unreachable. It now accounts for all three streams. Covered by three new store tests. - `sort.field` was read for its direction only and silently dropped, so a caller asking for `title` got the default `modDate`. Both shipped templates pass `modDate`, which is why it stayed invisible. `DotAssetPickerBrowseOptions` gains `sortField`, wired through to the store's initial sort. Correctness and diagnostics: - The link-cursor fallback was `?? 0` while its own comment claimed it preserved the last-sent cursor. `nextLinkCursor` and `hasMoreLinks` are both optional, so a response reporting more links without a cursor would rewind the stream and re-serve links already shown. - A failed `getCurrentSite()` resolved null with nothing logged, making a 5xx or expired session indistinguishable from the user pressing Cancel. - `toSelection` cast four `unknown` values straight into `string` fields; they now go through a narrowing helper, which also removes the blanket cast. - Dropped an unnecessary `as DotCMSContentlet` in `#resolveAssetLocation` — both union members declare `url`, so reading it directly means a future variant without one fails to compile. - Test fixtures: removed `extensions` from a browse-options fixture (not part of the contract), and typed the component-spec selection mock as `DotContentDriveBrowseItem` so its fixtures no longer need double casts. - Comment wording: `openBrowserModal` rather than `browse`, matching siblings. Verified with `nx build dotcms-ui`, lint across 5 projects, and the picker, bridge and Content Drive suites (1377 Content Drive tests unchanged). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pull Request Unsafe to Rollback!!!
|
This pull request introduces support for browsing and selecting menu links (in addition to files, folders, and pages) in the asset picker modal used by custom field templates. It refactors the Angular form bridge to use the new asset picker, adds a mechanism for resolving the current site, and updates the relevant models and APIs to handle the new "link" type. The Dojo form bridge remains a stub for the browser modal, but now emits a warning for unsupported usage. The most important changes are grouped below.
Asset Picker and Browsing Enhancements
DotContentDriveLinkinterface and updatedDotContentDriveItemto include links, allowing the asset picker to display and select menu links alongside other item types. (core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts)showLinks,linkCursor,hasMoreLinks,nextLinkCursor). (core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts) [1] [2]Angular Form Bridge Refactor
AngularFormBridge.openBrowserModal()to use the newDotAssetPickerComponent, supporting selection of pages, folders, and links, and returning a consistent handle with a promise-based API. (core-web/libs/edit-content-bridge/src/lib/bridges/angular-form-bridge.ts)resolveSitemechanism to allow the bridge to obtain the current site context asynchronously, ensuring the picker always browses the correct site. (core-web/libs/edit-content-bridge/src/lib/bridges/angular-form-bridge.ts) [1] [2] [3] [4]Legacy Editor (Dojo) Handling
openBrowserModalis called, making unsupported usage more transparent. (core-web/libs/edit-content-bridge/src/lib/bridges/dojo-form-bridge.ts,core-web/libs/edit-content-bridge/src/lib/bridges/dojo-form-bridge.spec.ts) [1] [2]Feature Directory Update
.specify/feature.jsonto reflect the new asset picker modal for menu links. (.specify/feature.json)This PR fixes: #37207