Skip to content

Scope Asset Picker uploads to the field that opened the picker - #37372

Open
nicobytes wants to merge 2 commits into
mainfrom
nicobytes/37365-asset-picker-scope-the-upload-flow-to-the-field-that-opened-the-picker
Open

Scope Asset Picker uploads to the field that opened the picker#37372
nicobytes wants to merge 2 commits into
mainfrom
nicobytes/37365-asset-picker-scope-the-upload-flow-to-the-field-that-opened-the-picker

Conversation

@nicobytes

@nicobytes nicobytes commented Sep 3, 2026

Copy link
Copy Markdown
Member

Problem

The Asset Picker already restricts what you can browse to the field that opened it (e.g. an Image field only lists images), but it never applied that same restriction to uploads. From inside an Image field or a Story Block video/audio node, you could upload any file type — via the OS file dialog, drag-and-drop, or the Asset/File prompt — and the upload would succeed but then silently disappear from the list because it didn't match the browse filter.

Solution

The upload flow now reads the same mimeTypes restriction already used for browsing and applies it consistently everywhere a file enters the picker:

  • The hidden file input's accept attribute is derived from the restriction, pre-filtering the OS dialog (removed entirely when unrestricted, not set to "").
  • A new pure upload-restriction.ts module (isUploadAllowed, buildUploadAccept, resolveUploadRestrictionLabel) centralizes the matching logic — no filename/extension checks, empty file types are allowed through (server stays the authority), case-insensitive family matching (image/*, etc).
  • A guard runs both early (on drag-and-drop, before the Asset/File prompt opens) and at the single mandatory convergence point (#resolveFilesUpload) so every route — Upload button, drag-and-drop, and folders with a pinned default upload type — is covered.
  • Refused uploads show an error toast naming the allowed types (new dot.asset.picker.upload.* message keys).
  • The Asset/File prompt (DotUploadTypeSelectorComponent) gains an optional restrictionLabel input; when set, it swaps in new *.description.scoped copy instead of the generic description, without removing either storage option.
  • Content Drive, which shares the same prompt/dropzone components, passes no restriction and keeps its exact current behavior and wording — covered by a new regression spec.

Includes unit tests for the new restriction module, the picker's upload guard across all routes, the selector's scoped/unscoped copy, and the Content Drive regression case, plus the accompanying spec-kit documents (spec.md, data-model.md, upload-restriction contract).

Test plan

  • Unit tests for upload-restriction.ts (allowed/rejected types, accept string building, label resolution)
  • DotAssetPickerComponent spec: refused drops, refused OS-dialog picks, refused pinned-folder uploads, allowed uploads, unclassifiable file types allowed, accept attribute presence/absence
  • DotUploadTypeSelectorComponent spec: scoped vs default descriptions, both options always rendered
  • DotContentDriveShellComponent regression spec: restriction label stays empty
  • Manual verification in an Image field, a Story Block video node, and Content Drive (not run in this session)

This PR fixes: #37365

- The OS file dialog, drag-and-drop, and the Asset/File prompt all ignored
  the mimetype restriction already used to filter browsing, so an Image
  field or a Story Block video/audio node could upload any file type,
  producing assets that then vanished from the filtered list.
- Add a pure `upload-restriction.ts` module (matcher, `accept` builder,
  label resolver) driven by the picker's existing `config.mimeTypes`, wire
  it into the hidden file input, a pre-upload guard on every upload route,
  and scoped copy in the Asset/File prompt, while leaving Content Drive and
  the unrestricted File/browse modes unchanged.
Copilot AI lite review requested due to automatic review settings September 3, 2026 15:04
@github-actions github-actions Bot added the Area : Frontend PR changes Angular/TypeScript frontend code label Sep 3, 2026
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nicobytes's task in 1m 35s —— View job


Code Review — PR #37372

  • Read core diff (upload-restriction module, asset picker, selector, dropzone)
  • Analyze for bugs, edge cases, conventions
  • Post review

I reviewed all 14 changed files, focusing on the enforcement logic (upload-restriction.ts, the guard placement in dot-asset-picker.component.ts), the message-key wiring, and the selector copy switching.

New Issues

No issues found.

The design holds up well against the failure modes I checked:

  • Guard convergence is real, not just claimed. #refuseDisallowedUpload runs at the single mandatory point (#resolveFilesUpload:587) that all three routes funnel through — Upload button (onFileChange#resolveFilesUpload), drag-and-drop (onRequestUpload → early guard + convergence), and pinned-folder (onRequestUpload#resolveFilesUpload). The early guard in onRequestUpload:443 is a UX nicety (avoid prompting for an ineligible file), not the guarantee, and the code comments correctly say so.
  • accept is a hint, not the fence. [attr.accept]="$uploadAccept()" (html:133) pre-filters the OS dialog and is removed entirely (null, not "") when unrestricted — correct, since a user can switch the dialog back to "all files," which is exactly why the JS guard still stands behind it.
  • Family matching is prefix-anchored, not substring. upload-restriction.ts:64-66 uses type.startsWith('image/') rather than a substring test, so x-image/foo is correctly rejected. Exact patterns fall through to type === candidate. Case handled via toLowerCase() on both sides.
  • Permissive-by-absence is deliberate and safe. Empty mimeTypes → allowed (File field / browse stay unrestricted), and empty file.type → allowed (server stays the authority). Both are documented and covered by tests.
  • Label resolution degrades gracefully. LABELLED_FAMILIES is derived from ASSET_PICKER_MIME_TYPES keys, and the new dot.asset.picker.upload.types.{image,video,audio} keys match those families exactly; an unlabelled family falls back to the raw pattern rather than producing "Only can be uploaded here."
  • Message interpolation is validDotMessageService.get(key, ...args) (dot-messages.service.ts:56) supports the {0} placeholder used in upload.rejected.detail and the .scoped description keys.
  • Content Drive unchanged — it passes no restrictionLabel, so the ''-means-unrestricted path keeps the generic copy, and a regression spec pins that.

Notes (non-blocking)

  • Multi-file drop refuses on files[0] only (#refuseDisallowedUpload:557, #resolveFilesUpload:606). This matches the pre-existing "only the first file is uploaded" behavior (the multi-file warning toast already fires), so it's consistent — not a regression. Worth a mention only because a mixed drop (allowed first, disallowed second) silently ignores the second file, which was already the case before this PR.

Nice work — the pure module + single convergence guard is the right shape, and the test coverage across all upload routes is thorough.
· nicobytes/37365-asset-picker-scope-the-upload-flow-to-the-field-that-opened-the-picker

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.

🟢 Approval recommended

The upload restriction is implemented centrally and enforced at all upload entry points with strong unit/component coverage while preserving unrestricted Content Drive behavior.

Pull request overview

This PR fixes an Asset Picker UX/behavior gap in core-web where browsing was scoped by the opening field’s mimeTypes, but uploads were not—allowing “wrong-type” uploads to succeed and then disappear from the filtered list. The change centralizes upload restriction logic and applies it consistently across OS file dialog selection, drag-and-drop, and pinned-folder upload flows, while explicitly preserving Content Drive’s unrestricted behavior.

Changes:

  • Added a pure upload restriction module (isUploadAllowed, buildUploadAccept, resolveUploadRestrictionLabel) and wired it into the Asset Picker as both an early guard (drop) and a mandatory convergence-point guard before upload.
  • Scoped the OS file dialog via the hidden file input’s [attr.accept] (removed entirely when unrestricted) and updated the upload selector copy to optionally reflect a restriction label.
  • Added i18n keys plus unit/component regression coverage for restricted picker modes and for unrestricted Content Drive behavior.
File summaries
File Description
specs/37365-asset-picker-upload-scope/spec.md Spec-kit spec documenting the bug, scope, ACs, and verification strategy.
specs/37365-asset-picker-upload-scope/data-model.md Defines the in-memory restriction shape, validation rules, and guard placement.
specs/37365-asset-picker-upload-scope/contracts/upload-restriction.contract.md Pins component inputs/DOM/message-key contracts to prevent regressions and over-reach.
dotCMS/src/main/webapp/WEB-INF/messages/Language.properties Adds new message keys for scoped upload selector copy + upload rejection toast + family labels.
core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.ts Adds optional restrictionLabel input to support scoped copy without changing option availability.
core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.html Switches descriptions to scoped keys when restrictionLabel is present.
core-web/libs/ui/src/lib/components/dot-upload-type-selector/constants.ts Adds per-option scopedDescriptionKey metadata.
core-web/libs/ui/src/lib/components/dot-upload-type-selector/dot-upload-type-selector.component.spec.ts Verifies scoped vs unscoped copy and ensures both options always render.
core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.ts New pure module implementing matching, accept-building, and user-label resolution for restrictions.
core-web/libs/ui/src/lib/components/dot-asset-picker/upload-restriction.spec.ts Unit tests for restriction matching/accept/label behavior (including unclassifiable file types).
core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.ts Applies restriction to uploads via early + mandatory guards and computes accept/label from config.mimeTypes.
core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.html Adds [attr.accept] binding and passes restriction label into the upload type selector.
core-web/libs/ui/src/lib/components/dot-asset-picker/dot-asset-picker.component.spec.ts Adds specs covering refusal across all upload routes + accept attribute presence/absence + selector label wiring.
core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.spec.ts Regression test ensuring Content Drive remains unrestricted (empty restrictionLabel).
Review details
  • Files reviewed: 14/14 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Asset Picker: scope the upload flow to the field that opened the picker

3 participants