Conversation
cb61157 to
db6b1b0
Compare
… instead of entire folders
…or single/index files
- use full path for new and existing entries - update path extraction for EW - update meta path validation to consider path type
|
We need to handle the deletion of entries that are missing some translations. Currently, we get |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for index files with separate field configurations in folder collections, addressing issue #7381. The feature allows users to differentiate between index files (like _index.md) and regular content files within the same collection, each with their own field definitions.
Changes:
- Adds
index_fileconfiguration option to collection schemas with pattern matching, custom fields, and editor settings - Implements path type selection dropdown for nested collections to choose between creating index pages or content pages
- Updates backend implementations (GitHub, local Git, local FS) to handle index file identification and folder-aware file operations
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/decap-cms-core/src/types/redux.ts | Adds TypeScript types for index_file config, path_type meta field, readonly field property, and CmsCollectionMeta interface |
| packages/decap-cms-core/index.d.ts | Updates public TypeScript definitions with index_file configuration support |
| packages/decap-cms-core/src/constants/configSchema.js | Adds schema validation for index_file configuration including pattern validation and file collection restriction |
| packages/decap-cms-core/src/constants/tests/configSchema.spec.js | Adds comprehensive tests for index_file validation rules |
| packages/decap-cms-core/src/lib/indexFileHelper.ts | Creates new utility functions for identifying index files and entries with nested collection support |
| packages/decap-cms-core/src/reducers/collections.ts | Updates field selection logic to return index_file fields when appropriate and adds nested collection helpers |
| packages/decap-cms-core/src/reducers/entryDraft.js | Modifies custom path calculation to handle both index and slug path types |
| packages/decap-cms-core/src/reducers/entries.ts | Adds logic to ensure index file entries appear first in the entry list |
| packages/decap-cms-core/src/reducers/tests/entries.spec.js | Adds tests for index_file entry loading and path_type meta field handling |
| packages/decap-cms-core/src/backend.ts | Updates entry loading to set path_type meta field and implements folder-aware file operations |
| packages/decap-cms-core/src/actions/entries.ts | Updates path validation to check both index and slug path types, fixes duplicate entry action |
| packages/decap-cms-core/src/actions/config.ts | Adds meta field injection for path and path_type fields in collections with index_file |
| packages/decap-cms-core/src/actions/tests/entries.spec.js | Updates test expectations for path_type in meta field validation |
| packages/decap-cms-core/src/lib/i18n.ts | Adds meta field removal from i18n data, updates file path comparison for nested collections |
| packages/decap-cms-core/src/lib/tests/i18n.spec.js | Adds mock for isNestedSubfolders selector |
| packages/decap-cms-core/src/components/Collection/CollectionTop.js | Adds dropdown for path type selection in nested collections with index_file |
| packages/decap-cms-core/src/components/Collection/NestedCollection.js | Uses centralized isNestedSubfolders helper |
| packages/decap-cms-core/src/components/Collection/Entries/EntryCard.js | Adds home icon indicator for index file entries |
| packages/decap-cms-core/src/components/Editor/EditorInterface.js | Updates preview detection to respect index_file editor settings |
| packages/decap-cms-core/src/components/Editor/EditorControlPane/EditorControl.js | Adds readonly field support to disable editing |
| packages/decap-cms-core/src/components/Editor/Editor.js | Passes path_type query param to field selection and fixes nested collection back link calculation |
| packages/decap-server/src/middlewares/utils/fs.ts | Adds isFolder parameter to move function to conditionally move child files |
| packages/decap-server/src/middlewares/types.ts | Adds isFolder property to DataFile type |
| packages/decap-server/src/middlewares/localGit/index.ts | Passes isFolder parameter to move function |
| packages/decap-server/src/middlewares/localFs/index.ts | Passes isFolder parameter to move function |
| packages/decap-cms-lib-util/src/implementation.ts | Adds isFolder property to DataFile type |
| packages/decap-cms-backend-github/src/API.ts | Updates tree operations to handle folder-aware file moves |
| packages/decap-cms-locales/src/en/index.js | Adds English translations for path type labels |
| packages/decap-cms-locales/src/sl/index.js | Adds Slovenian translations for path type labels |
| packages/decap-cms-core/src/tests/backend.spec.js | Updates test expectation to include path_type in meta |
| packages/decap-cms-core/src/valueObjects/Entry.ts | Adds path_type to meta object type and srcSlug to EntryValue |
| dev-test/config.yml | Adds index_file configuration example |
| dev-test/index.html | Adds _index.md test file data |
| dev-test/backends/test/config.yml | Adds index_file configuration example |
| dev-test/backends/test/index.html | Adds _index.md test file data |
| cypress/e2e/index_fields_spec.js | Adds E2E tests for index file feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| usedSlugs, | ||
| customPath, | ||
| ); | ||
| isFolder = prepareMetaPathType(slug, collection) === 'index'; |
There was a problem hiding this comment.
The condition pathType === 'index' means the code determines isFolder = true for index file entries and isFolder = false for slug-type entries. However, this naming is confusing because an "index file" is still a file, not a folder. The variable name isFolder suggests it represents a directory, but it actually represents whether the entry should be treated as having subfolders that need to be moved along with it. Consider renaming this to something more descriptive like moveSubfolders or hasSubfolders to clarify its purpose.
| data: entry.get('data'), | ||
| i18n: entry.get('i18n'), | ||
| meta: entry.get('meta').toJS(), | ||
| i18n: entry.get('i18n').toJS(), |
There was a problem hiding this comment.
The draftDuplicateEntry function is calling .toJS() on entry.get('i18n') which may be undefined. If the entry doesn't have i18n data, this will throw a TypeError. You should add a check to ensure i18n exists before calling .toJS(), or provide a fallback value like an empty object.
| payload: createEntry(entry.get('collection'), '', '', { | ||
| data: entry.get('data'), | ||
| i18n: entry.get('i18n'), | ||
| meta: entry.get('meta').toJS(), |
There was a problem hiding this comment.
Similarly, calling .toJS() on entry.get('meta') may fail if meta is undefined. While meta is more commonly present, it's safer to add a check or provide a fallback value to avoid potential runtime errors.
| meta: entry.get('meta').toJS(), | |
| meta: (entry.get('meta') || Map()).toJS(), |
| .get('fields') | ||
| ?.find(f => f.get('name') === inferredFields.imageField && f.get('widget') === 'image'), | ||
| isLoadingAsset, | ||
| showIndexFileIcon: indexFileConfig && new RegExp(indexFileConfig.get('pattern')).test(fileSlug), |
There was a problem hiding this comment.
The pattern matching logic doesn't consider the nested flag from the collection. The isIndexFile helper function accounts for nested collections by extracting just the filename from the slug when nested is true. However, here you're testing the entire fileSlug directly against the pattern without considering whether the collection is nested. This could lead to inconsistent behavior where a file is identified as an index file in some parts of the code but not others.
Consider using the isIndexFile helper function here instead of duplicating the pattern matching logic, or ensure the slug extraction logic is consistent with what's done in the helper.
| } | ||
| const meta = entryDraft.getIn(['entry', 'meta']); | ||
| const path = meta && meta.get('path'); | ||
| const pathType = meta && meta.get('path_type', 'index'); |
There was a problem hiding this comment.
The check condition seems backwards. When pathType === 'index' the default should be 'index', not 'slug'. Similarly, when checking for nested subfolders or index path type, both should use the index file name. The current logic may be correct but would benefit from clarification. Consider restructuring to make the intent clearer:
const pathType = meta && meta.get('path_type', 'slug'); // default to 'slug' for regular files
This makes it clearer that 'slug' is the default for content files, while 'index' is explicitly set for index files.
| const pathType = meta && meta.get('path_type', 'index'); | |
| const pathType = meta && meta.get('path_type', 'slug'); // default to 'slug' for regular files |
| path_type: string, | ||
| ) { | ||
| const customPath = selectCustomPath(collection, fromJS({ entry: { meta: { path, path_type } } })); |
There was a problem hiding this comment.
The function signature shows path_type using snake_case naming, which is inconsistent with JavaScript/TypeScript conventions where camelCase is typically used for variable names. Consider renaming this parameter to pathType for consistency with the codebase's naming conventions.
| path_type: string, | |
| ) { | |
| const customPath = selectCustomPath(collection, fromJS({ entry: { meta: { path, path_type } } })); | |
| pathType: string, | |
| ) { | |
| const customPath = selectCustomPath( | |
| collection, | |
| fromJS({ entry: { meta: { path, path_type: pathType } } }), | |
| ); |
| path_type: string, | ||
| ) { | ||
| const customPath = selectCustomPath(collection, fromJS({ entry: { meta: { path, path_type } } })); |
There was a problem hiding this comment.
The variable name uses snake_case (path_type) which is inconsistent with JavaScript/TypeScript naming conventions. While this matches the data property name, for local variables camelCase (pathType) is more conventional in JavaScript/TypeScript code.
| path_type: string, | |
| ) { | |
| const customPath = selectCustomPath(collection, fromJS({ entry: { meta: { path, path_type } } })); | |
| pathType: string, | |
| ) { | |
| const customPath = selectCustomPath( | |
| collection, | |
| fromJS({ entry: { meta: { path, path_type: pathType } } }), | |
| ); |
| @@ -443,7 +447,7 @@ function mapStateToProps(state, ownProps) { | |||
| if (collection.has('nested') && slug) { | |||
There was a problem hiding this comment.
The path calculation logic has changed from slice(0, -2) to slice(0, -1), but there's no accompanying test or comment explaining why this change is necessary. This appears to be fixing an off-by-one error in the back link calculation for nested collections, but without context it's unclear if this is correct for all nested collection scenarios. Consider adding a comment explaining the logic or ensuring test coverage validates this change works correctly for different path depths.
| if (collection.has('nested') && slug) { | |
| if (collection.has('nested') && slug) { | |
| // For nested collections, the slug is a path-like string: | |
| // <folder>/<subfolder>/.../<entrySlug> | |
| // We want the back link to point to the parent folder view, not the entry itself. | |
| // Therefore, we drop only the final entry segment with slice(0, -1) and keep | |
| // all leading folders, which are then used as the filter path. |
| if (!data || typeof data !== 'object' || !('delete' in data)) { | ||
| return data; | ||
| } | ||
| return (data as Map<string, unknown>).delete('path').delete('path_type'); |
There was a problem hiding this comment.
The removeMetaFields function has a fragile type check. The condition !('delete' in data) will return true for any object that doesn't have a 'delete' property, not just for Immutable.js Map objects. This could lead to unexpected behavior if non-Map objects are passed to this function. Consider using a more specific check like Map.isMap(data) to verify that the data is actually an Immutable.js Map before attempting to call the delete method.
| if (!data || typeof data !== 'object' || !('delete' in data)) { | |
| return data; | |
| } | |
| return (data as Map<string, unknown>).delete('path').delete('path_type'); | |
| if (!Map.isMap(data)) { | |
| return data; | |
| } | |
| return data.delete('path').delete('path_type'); |
| const [p1, p2] = [path1, path2].map(p => p.split('/')); | ||
| return hasSubfolders | ||
| ? p1.slice(-2).join('/') === p2.slice(-2).join('/') | ||
| : p1.at(-1) === p2.at(-1); |
There was a problem hiding this comment.
The array at() method is used here, but it's a relatively recent addition to JavaScript (ES2022). While the code may work in modern environments, this could cause issues in older browsers or Node.js versions that don't support this method. Consider using traditional array indexing instead: p1[p1.length - 1] === p2[p2.length - 1] for better compatibility, or ensure your build configuration properly transpiles this feature.
| : p1.at(-1) === p2.at(-1); | |
| : p1[p1.length - 1] === p2[p2.length - 1]; |
| const pathParts = slug.split('/'); | ||
| if (pathParts.length > 2) { | ||
| editorBackLink = `${editorBackLink}/filter/${pathParts.slice(0, -2).join('/')}`; | ||
| editorBackLink = `${editorBackLink}/filter/${pathParts.slice(0, -1).join('/')}`; |
There was a problem hiding this comment.
Should this change be applied to all nested collections or only ones using index fields? Just flagging in case that's a possible regression.
| .get('fields') | ||
| ?.find(f => f.get('name') === inferredFields.imageField && f.get('widget') === 'image'), | ||
| isLoadingAsset, | ||
| showIndexFileIcon: indexFileConfig && new RegExp(indexFileConfig.get('pattern')).test(fileSlug), |
There was a problem hiding this comment.
Maybe this should use the isIndexFile helper instead?
| dataFiles.forEach(async dataFile => { | ||
| await move( | ||
| path.join(repoPath, dataFile.path), | ||
| path.join(repoPath, dataFile.newPath!), | ||
| dataFile.isFolder, |
There was a problem hiding this comment.
GitHub is weird with suggestions outside the lines changed, but AFAIK, forEach doesn't await properly, so this should be an for loop instead.
dataFiles.forEach(async dataFile => {
await move(
path.join(repoPath, dataFile.path),
path.join(repoPath, dataFile.newPath!),
dataFile.isFolder,
);
});
| dataFiles.forEach(async dataFile => { | ||
| await move(path.join(repoPath, dataFile.path), path.join(repoPath, dataFile.newPath!)); | ||
| await move( | ||
| path.join(repoPath, dataFile.path), | ||
| path.join(repoPath, dataFile.newPath!), | ||
| dataFile.isFolder, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Same here, it should be a for loop
Summary
As described in issue #7381 we need a way to have a specific field set for index files in collections. More info coming as we smooth out the details and clean up the code.
Update March 5th:
Added an option to select path type when adding new entries in such collections. Updated proxy and github backends to work in combination with the new nested system, i18n.
It is ready for some careful internal testing, but there is a ton of stuff still to do:
Test plan
Add the following config to the collection options. Pattern is required. Fields are optional, setting them will result in a different entry form for this file. Label is also optional and will add prefix to the title in the entry list.
Checklist
Please add a
xinside each checkbox:A picture of a cute animal (not mandatory but encouraged)