Fixes #7546 - Relation widget shows "No {{fields.name}}" when collapsed and search returns "No options"#7657
Fixes #7546 - Relation widget shows "No {{fields.name}}" when collapsed and search returns "No options"#7657BiaDrab wants to merge 13 commits intodecaporg:mainfrom
Conversation
|
@BiaDrab before I review, please make sure that tests are passing. |
…rab/decap-cms into fix/widget-return-no-option
…rab/decap-cms into fix/widget-return-no-option
packages/decap-cms-backend-git-gateway/src/netlify-lfs-client.ts
Outdated
Show resolved
Hide resolved
| "test:watch": "yarn test --watch", | ||
| "test:coverage": "yarn test --coverage" | ||
| "test:coverage": "npm test -- --coverage" |
There was a problem hiding this comment.
yarn shouldn't be used anymore I believe, I'll make a new PR to fix that.
There was a problem hiding this comment.
Pull request overview
This PR appears intended to address relation widget UX issues (collapsed display/search) and prevent duplicate selections in list-based relation fields. In the actual diffs provided, the main functional change is duplicate-selection prevention in the list widget, along with updates to the dev-test configuration/fixtures and some dependency/script adjustments.
Changes:
- Add duplicate-selection detection for
relationfields insideListControland display an inline error banner. - Update
ListControlJest snapshots to match the new render output. - Add dev-test config/fixtures and update root dependencies/lockfiles (currently introducing some configuration and dependency issues).
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/decap-server/package.json | Adjusts test:coverage script (now mixes npm/yarn invocation). |
| packages/decap-cms-widget-list/src/ListControl.js | Adds duplicate detection + inline error banner for relation fields; changes render wrapper. |
| packages/decap-cms-widget-list/src/tests/snapshots/ListControl.spec.js.snap | Snapshot updates reflecting ListControl DOM changes. |
| package.json | Adds decap-cms, decap-cms-app, decap-server dependencies (problematic in a monorepo that already contains these workspaces). |
| package-lock.json | Lockfile updated to include the above dependencies (encodes a self-dependency). |
| dev-test/config.yml | Appends a second top-level collections: block (overrides existing config) and includes incomplete filter rules. |
| dev-test/assets/images/computer-monitor-is-desk-with-rainbow-colored-background_763111-288451.jpg | Adds an image whose header indicates AVIF despite .jpg extension. |
| content/entries/2024-01-15-top-10-best-camping-cocktails.md | Adds a content entry fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| widget: string | ||
| meta: { path: { widget: string, label: 'Path', index_file: 'index' } } | ||
|
|
||
| collections: |
There was a problem hiding this comment.
dev-test/config.yml already defines collections: earlier in the file; adding a second top-level collections: key here will override the original one in YAML parsers, effectively discarding the existing dev-test collections configuration. This should be merged into the existing collections array instead of redefining the key.
| collections: |
| - label: "Top Posts" | ||
| name: "top_posts" | ||
| widget: "list" | ||
| summary: '{{title}}' | ||
| fields: | ||
| - label: "Post" | ||
| name: "featured_entries" | ||
| widget: "relation" | ||
| collection: "entries" | ||
| max: 4 | ||
| search_fields: ["title", "publications", "topics"] | ||
| display_fields: ["{{title}}"] | ||
| value_field: "{{slug}}" | ||
| - {label: "Instagram Heading", name: ig_heading, widget: string} |
There was a problem hiding this comment.
The new dev-test config block still uses summary: '{{title}}' and value_field: "{{slug}}", which doesn’t align with the PR description (claims summary/value_field changes to fix #7546). If this file is meant to be the reproduction/fixed config, update it to reflect the intended fixed configuration so the dev-test environment validates the bugfix.
| "decap-cms": "^3.8.4", | ||
| "decap-cms-app": "^3.8.4", |
There was a problem hiding this comment.
Root package.json is for the decap-cms monorepo (workspaces include packages/*, and packages/decap-cms + packages/decap-cms-app exist). Adding published decap-cms/decap-cms-app as dependencies here creates a self-dependency and can cause duplicate installs / resolution conflicts. These entries should be removed, or if you truly need to reference workspace packages, use the workspace protocol instead of semver from npm.
| "decap-cms": "^3.8.4", | |
| "decap-cms-app": "^3.8.4", |
| "cypress": "^12.17.4", | ||
| "cypress-jest-adapter": "^0.1.1", | ||
| "cypress-plugin-tab": "^1.0.0", | ||
| "decap-server": "^3.3.1", |
There was a problem hiding this comment.
Similarly, adding decap-server as a devDependency at the repo root will pull the published package even though packages/decap-server is part of this workspace. This is likely to introduce duplicate copies and version skew; prefer using the workspace package or referencing it via workspace:* if needed for tooling.
| "decap-server": "^3.3.1", | |
| "decap-server": "workspace:*", |
| this.duplicateErrorIndex = index; | ||
| this.forceUpdate(); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (this.duplicateErrorIndex === index) { | ||
| this.duplicateErrorIndex = null; |
There was a problem hiding this comment.
This introduces UI state via an ad-hoc instance property + forceUpdate(). That bypasses React state flow and makes the behavior harder to reason about and test. Prefer storing the duplicate error in component state (and updating it alongside list mutations) so renders are driven by setState.
| this.duplicateErrorIndex = index; | |
| this.forceUpdate(); | |
| return; | |
| } | |
| } | |
| if (this.duplicateErrorIndex === index) { | |
| this.duplicateErrorIndex = null; | |
| this.setState({ duplicateErrorIndex: index }); | |
| return; | |
| } | |
| } | |
| if (this.state && this.state.duplicateErrorIndex === index) { | |
| this.setState({ duplicateErrorIndex: null }); |
| folder: "content/entries" # The path to the folder where the documents are stored | ||
| create: true # Allow users to create new documents in this collection | ||
| filter: {field: "headless"} | ||
| slug: "{{year}}-{{month}}-{{day}}-{{slug}}" # Filename template, e.g., YYYY-MM-DD-title.md | ||
| sortable_fields: ['title', 'date'] |
There was a problem hiding this comment.
filter rules require both a field and a value. With only field: "headless", the backend filter logic will compare entry.data.headless === undefined, which will likely filter out all entries that have headless: true. Add an explicit value (e.g. true) if the intent is to include only headless entries.
| folder: "content/pages" # The path to the folder where the documents are stored | ||
| create: true # Allow users to create new documents in this collection | ||
| filter: {field: "headless"} | ||
| slug: "{{title}}" # Filename template, e.g., YYYY-MM-DD-title.md | ||
| sortable_fields: ['title', 'date'] |
There was a problem hiding this comment.
Same issue here: filter: {field: "headless"} omits the required value, so the filter will match only entries where headless is undefined. Specify value: true/false (or remove the filter) depending on the intended behavior.
| if (f.get('widget') === 'relation') { | ||
| const isDuplicate = | ||
| value && | ||
| value.some((item, idx) => { | ||
| if (idx === index) return false; | ||
| const itemValue = withNameKey ? item.get(f.get('name')) : item; | ||
| return itemValue === newValue; | ||
| }); | ||
|
|
||
| if (isDuplicate) { | ||
| console.warn(`⚠️ Duplicate: "${newValue}" already selected`); | ||
|
|
||
| this.duplicateErrorIndex = index; | ||
| this.forceUpdate(); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (this.duplicateErrorIndex === index) { | ||
| this.duplicateErrorIndex = null; | ||
| } |
There was a problem hiding this comment.
duplicateErrorIndex is stored as a list index. If the user removes/reorders items after triggering a duplicate error, the index can point to a different item and show the error on the wrong row (or fail to clear). Consider tracking the error by the item key (this.state.keys[index]) or resetting/updating the duplicate marker on remove/sort.
| {hasDuplicateError && ( | ||
| <div | ||
| style={{ | ||
| background: '#fee', | ||
| color: '#c33', | ||
| padding: '12px', | ||
| marginBottom: '12px', | ||
| borderRadius: '4px', | ||
| border: '1px solid #fcc', | ||
| fontSize: '14px', | ||
| fontWeight: 'bold', | ||
| }} | ||
| > | ||
| ❌ This entry is already selected in this list | ||
| </div> |
There was a problem hiding this comment.
The duplicate-selection banner is hard-coded (inline styles + emoji) and not localized, even though this component already receives t for translations. Please route this through the existing styling system (emotion styles/theme) and use a translated message via t(...) so it’s consistent and accessible.
| if (f.get('widget') === 'relation') { | ||
| const isDuplicate = | ||
| value && | ||
| value.some((item, idx) => { | ||
| if (idx === index) return false; | ||
| const itemValue = withNameKey ? item.get(f.get('name')) : item; | ||
| return itemValue === newValue; | ||
| }); | ||
|
|
||
| if (isDuplicate) { | ||
| console.warn(`⚠️ Duplicate: "${newValue}" already selected`); | ||
|
|
||
| this.duplicateErrorIndex = index; | ||
| this.forceUpdate(); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
New duplicate-prevention behavior isn’t covered by unit tests. Since ListControl.spec.js already exists, please add a test that selecting a duplicate relation value does not call onChange and that the duplicate error message is shown/cleared appropriately.
|
Thanks @BiaDrab, Copilot found a few things, could you take a look and update your PR accordingly? 🙌 |
Summary
This PR addresses the relation widget bugs reported in issue #7546 and discovers an additional related issue during investigation.
Issues Found and Fixed
Issue #7546 (Primary)
Additional Issue (Discovered)
4. Duplicate entry selection allowed - Same entry could be selected multiple times in the Top Posts list.
Root Causes
Solution:
Changed the list summary from '{{title}}' to '{{fields.featured_entries}}' to display the selected entry name
Added the slug field to the entries collection form so it's available in front matter
The relation widget now properly uses value_field: "slug" without extra braces
Testing
Configuration Changes
Homepage - Top Postssummary: '{{fields.featured_entries}}' # Changed from '{{title}}'# Entries collection - added slug field- {label: "Slug", name: "slug", widget: "string", hint: "URL-friendly version (use hyphens, no spaces)"}