Refactor shared assets filter into reusable DotFilterBar components - #37374
Conversation
- Extracts the shared-assets toggle chip and a new DotFilterFacade seam into `@dotcms/ui` so both Content Drive and the AssetPicker drive it through the same store-agnostic contract instead of each surface reimplementing it. - Adds facade implementations for both stores plus a shared conformance test suite so their differing encodings (numeric vs. named base types) can't silently drift apart. - Fixes AssetPicker defects surfaced along the way: clearFilters used to drop the caller-seeded locale/base types, the filter bag couldn't hold dynamic/new keys, and includeSystemHost was hardcoded true with no way to turn it off.
- Introduces DotFilterBarComponent (layout + clear-all) plus content-type, language, and status filter chip adapters that bind the shared facade to presentational filters, so both surfaces gain the same chip set without duplicating adapter logic. - Adds a DOT_RELATIONSHIP_PICKER token so the field filter can offer a Relationship picker where a surface supplies it, avoiding a circular dependency on @dotcms/edit-content. - Updates spec.md with the resolved decision and new FRs (FR-020/FR-021, SC-010) covering optional per-surface filter extensions.
…ontent-drive-components-folder-tree-filter-chips-row-selection-and-scoped-upload
|
Claude finished @nicobytes's task in 1m 59s —— View job Code Review — PR #37374Reviewed the facade seam, the shared chips, and the Content Drive toolbar wiring against New Issues
Resolved (from earlier reviews)
Notes (not blocking)
The type-safety and contract regressions the earlier bots caught are genuinely fixed. The language-chip removable regression is the one I'd treat as blocking, since it ships a control that does nothing but reset pagination. Review by Claude (Opus 4.8). |
There was a problem hiding this comment.
🟡 Changes recommended
The Content Drive facade currently violates the facade “unset == undefined” contract for baseType, and a couple of shared chips make unsafe string[] casts that can throw if a surface ever stores/restores a single-string filter value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes the “Shared Assets” filter and related filter-chip plumbing so both Content Drive and the Asset Picker can share the same chip UI while talking to their own stores through a store-agnostic DotFilterFacade seam. It also introduces a reusable conformance test suite to keep both surfaces’ filter behaviors aligned over time.
Changes:
- Added
DOT_FILTER_FACADEcontract + per-surface facades (Content Drive + Asset Picker) and a shared conformance suite exported via@dotcms/ui/testing. - Moved the Shared Assets chip into
@dotcms/uiand wired the Asset Picker request to honorincludeSystemHostbased on the chip state. - Updated store models/defaults and expanded test coverage for filter defaults, “Clear all” semantics, and shared-assets behavior.
File summaries
| File | Description |
|---|---|
| specs/37174-shared-picker-toolbar/spec.md | Feature spec for shared picker toolbar work (scope: finding 6). |
| specs/37174-shared-picker-toolbar/data-model.md | Data model and invariants for facade, normalized values, and defaults. |
| specs/37174-shared-picker-toolbar/contracts/filter-facade.contract.md | Formal contract/obligations for DOT_FILTER_FACADE and shared conformance suite. |
| core-web/tsconfig.base.json | Adds TS path alias for @dotcms/ui/testing. |
| core-web/libs/ui/tsconfig.lib.json | Excludes test-only entrypoints from production compilation. |
| core-web/libs/ui/src/testing.ts | Test-only barrel exporting the conformance suite. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/testing/filter-facade.conformance.ts | Shared Jest suite asserting facade obligations O1–O9. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/filter-facade.token.ts | Defines DotFilterFacade, DotFilterValue, and the injection token. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/dot-filter-bar.component.ts | Introduces shared filter-bar wrapper component (projection + Clear all). |
| core-web/libs/ui/src/lib/components/dot-filter-bar/dot-filter-bar.component.spec.ts | Unit tests for projection behavior, Clear all visibility, and wrapping layout. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/dot-filter-bar.component.html | Template for projected chips plus conditional Clear all button. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/constants.ts | Canonical chip order + helper to validate subsequence ordering. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-status-filter/dot-status-filter.component.ts | New shared Status chip implementation wired via DOT_FILTER_FACADE. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-status-filter/dot-status-filter.component.spec.ts | Status chip tests incl. allowed-options bounding behavior. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-status-filter/dot-status-filter.component.html | Status chip UI (popover + listbox + bounded-note). |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-status-filter/constants.ts | Shared constants for status filter key and options. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-shared-assets-filter/dot-shared-assets-filter.component.ts | Shared “Show Shared Assets” chip implementation via facade. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-shared-assets-filter/dot-shared-assets-filter.component.spec.ts | Updated tests to validate facade usage and canonical chip id attribute. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-shared-assets-filter/dot-shared-assets-filter.component.html | Shared-assets chip template. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-shared-assets-filter/constants.ts | Single-source-of-truth constants for shared-assets filter key/values. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-language-filter-chip/dot-language-filter-chip.component.ts | New adapter chip wiring dot-language-filter through the facade. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-language-filter-chip/dot-language-filter-chip.component.spec.ts | Unit tests for language chip facade binding and removal behavior. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-field-filter/relationship-picker.token.ts | Optional capability token for relationship-picker integration without circular deps. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-content-type-filter-chip/dot-content-type-filter-chip.component.ts | New adapter chip wiring content-type filter through the facade. |
| core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-content-type-filter-chip/dot-content-type-filter-chip.component.spec.ts | Unit tests for content-type chip facade binding + allowed-base-type bounds. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/models.ts | Expands picker filter model to support shared keys + dynamic field filters. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/filter-defaults.ts | Centralizes picker filter defaults + non-default detection. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/features/with-asset-browse.feature.ts | Drives includeSystemHost from shared-assets filter state. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/dot-asset-picker.store.ts | Adds $hasNonDefaultFilters, getFilterValue, and clears back to caller seeds. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/dot-asset-picker.store.spec.ts | Updates + adds tests for defaults restore, bag widening, and shared-assets behavior. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/asset-picker-filter-facade.ts | Adds picker facade + provider and idempotence guard. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/store/asset-picker-filter-facade.spec.ts | Runs shared conformance suite against Asset Picker implementation. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts | Provides asset-picker facade alongside its store. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/components/dot-asset-picker-toolbar/dot-asset-picker-toolbar.component.ts | Renders shared-assets chip in the picker toolbar. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/components/dot-asset-picker-toolbar/dot-asset-picker-toolbar.component.spec.ts | Adds canonical-order tests and shared-assets rendering assertions. |
| core-web/libs/ui/src/lib/components/dot-asset-picker/components/dot-asset-picker-toolbar/dot-asset-picker-toolbar.component.html | Adds <dot-shared-assets-filter /> to picker toolbar markup. |
| core-web/libs/ui/src/index.ts | Re-exports shared filter constants/token and shared-assets chip from @dotcms/ui. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/content-drive-filter-facade.ts | Adds Content Drive facade (incl. base-type name↔number normalization). |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/content-drive-filter-facade.spec.ts | Runs conformance suite against Content Drive implementation. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/constants.ts | Re-exports shared-assets constants from @dotcms/ui. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts | Provides Content Drive facade alongside the store. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts | Updates tests to include facade provider. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.ts | Replaces Content Drive-only shared-assets chip with shared @dotcms/ui chip. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.spec.ts | Adds facade provider for shared chip dependency graph. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.html | Switches <dot-content-drive-shared-assets-filter> to <dot-shared-assets-filter>. |
| core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-shared-assets-filter/dot-content-drive-shared-assets-filter.component.ts | Removes Content Drive-only shared-assets component. |
| .claude/skills/dot-ui-vtl-migration/references/migration-guide.md | Updates migration docs for status behavior (esp. "archived" semantics + "live" bounding). |
Review details
Suppressed comments (1)
core-web/libs/ui/src/lib/components/dot-filter-bar/chips/dot-content-type-filter-chip/dot-content-type-filter-chip.component.ts:56
getFilterValue('contentType')may returnstringas well asstring[]. The direct cast tostring[]can throw if a single string is ever stored/restored. Normalize to an array (or[]) before passing it to the presentational filter.
protected readonly $contentTypes = computed(
() => (this.#filters.getFilterValue('contentType') as string[]) ?? []
);
- Files reviewed: 46/47 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Unblocks the `format-test` (nx format:check) step of PR Test / Frontend Unit Tests, which was failing on these three files and preventing the unit test and E2E jobs from running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Content Drive's content-type, language, and status filters were bespoke components duplicating what dot-filter-bar's chip adapters already do. Route both toolbars through the shared bar/chips, move workflow and field-filter chips onto the facade pattern, and fix the encoded-filter undefined-vs-empty-array distinction that the base-type/language chips were silently collapsing.
Content Drive's content-type, language, and status filters were bespoke components duplicating what dot-filter-bar's chip adapters already do. Route both toolbars through the shared bar/chips, move workflow and field-filter chips onto the facade pattern, and fix the encoded-filter undefined-vs-empty-array distinction that the base-type/language chips were silently collapsing.
- Move field-filter/status-filter logic, single-selectable value casting, and the relationship-picker/error-reporting seams into `@dotcms/ui` behind host tokens so Content Drive and the Asset Picker consume one implementation instead of two. - Replace the AssetPicker's `showArchived` pin with a shared, seedable Status chip (`status`), bounded to Locked-only when the picker is pinned to published content, so content condition has one representation across both surfaces. - Re-export moved constants/utilities from their original locations to keep existing importers working, and update docs/spec/messages to reflect the shared chip pattern.
…e-components-folder-tree-filter-chips-row-selection-and-scoped-upload' of github.com:dotCMS/core into nicobytes/37174-improvements-to-the-shared-content-drive-components-folder-tree-filter-chips-row-selection-and-scoped-upload
- Introduced a new statusFilterFor function in AngularFormBridge to streamline status handling for asset picking, replacing the previous archived status logic with a more flexible approach. - Updated DotContentDriveToolbarComponent tests to utilize the new DotContentDriveFilters type for improved type safety and clarity in filter management. - Adjusted the getFilterValue method to return a DotFilterValue type, ensuring consistent typing across filter operations.
Review notesNice refactor. The three seams ( Findings below, roughly by severity. Numbers 1 and 2 I think are actual regressions; the rest are questions or gaps. 1. Content Drive's language chip lost its non-removable-by-default behaviorThe old adapter passed // dot-content-drive-language-field.component.ts (deleted)
protected readonly $removable = computed(() => {
const selected = this.$selectedLanguageIds();
return !(selected.length === 1 && selected[0] === this.#store.defaultLanguageId());
});
The five tests covering this ( 2. The workflow-run indicator will likely wrap to its own row
3.
|
…ontent-drive-components-folder-tree-filter-chips-row-selection-and-scoped-upload
The generated header still reported 22 first-party skills after speckit-docs-converge was added, so `skill-lint` failed the "Lint skills" check with "CATALOG.md is stale". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This pull request refactors how the "shared assets" filter is implemented and shared between the Content Drive and AssetPicker features. The filter logic and UI component are now centralized in
@dotcms/ui, ensuring consistent behavior and removing duplicated code. The changes also introduce a store-agnostic facade for filter operations, making the filter chips reusable across different parts of the application. Additionally, the PR updates tests and documentation to reflect these architectural improvements.Shared Assets Filter Refactor and Reuse
The
dot-content-drive-shared-assets-filtercomponent is removed from the Content Drive and replaced with the new, shareddot-shared-assets-filtercomponent from@dotcms/ui, allowing the same filter UI and logic to be used in both Content Drive and AssetPicker. [1] [2] [3] [4]The constants for the shared assets filter (
SHARED_ASSETS_FILTER_KEY,SHARED_ASSETS_ENABLED_VALUE,SHARED_ASSETS_DISABLED_VALUE) are now re-exported from@dotcms/uiinstead of being defined locally, ensuring a single source of truth for these values.Store Facade Introduction and Integration
A new
provideContentDriveFilterFacadeprovider is introduced and registered alongside the Content Drive store. This facade abstracts filter operations so that filter chips interact with a store-agnostic interface, supporting reuse and encapsulation. [1] [2] [3] [4] [5]The toolbar and shell component tests are updated to provide the filter facade, ensuring that components using the shared filter chips have access to the correct store instance. [1] [2] [3] [4]
Testing and Conformance
content-drive-filter-facade.spec.ts) is added to verify that the Content Drive's filter facade conforms to the shared filter chip interface and encoding expectations, ensuring consistent filter behavior and encoding/decoding logic across surfaces.Documentation
statusfilter, especially how the"live"option affects the status filter UI in the dialog.This PR fixes: #37174