feat: support list values in resource field component#202
Open
gkrajniak wants to merge 6 commits into
Open
Conversation
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
…he collection to propertyCollection Signed-off-by: gkrajniak <gkrajniak@gmail.com>
…he collection to propertyCollection Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
📝 WalkthroughWalkthroughAdds an expandable card-based renderer for ChangesResource collection rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WithRichCells
participant ResourceField
participant ResourceCollectionField
participant RowResourceField
WithRichCells->>ResourceField: provide collection field definition and resource
ResourceField->>ResourceCollectionField: render propertyCollection
ResourceCollectionField->>ResourceCollectionField: derive entries and preview
ResourceCollectionField->>RowResourceField: render expanded sub-fields
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gkrajniak
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.html (1)
2-2: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider tracking entries by a stable key instead of
$index.Using
track $indexmeans Angular reuses DOM nodes by position, not identity. If the underlying array is reordered or items are inserted/deleted in the middle, expanded-state and DOM will be incorrectly associated with the wrong entry. If entries have a natural unique field (e.g.id,name), track by that instead.♻️ Suggested track expression
- `@for` (entry of entries(); track $index; let index = $index) { + `@for` (entry of entries(); track entry.id ?? $index; let index = $index) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.html` at line 2, Replace track $index in the `@for` loop of ResourceCollectionFieldComponent with a stable unique property from each entry, such as entry.id or entry.name, so DOM and expanded state remain associated with the correct item after reordering or middle insertion/deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.ts`:
- Around line 56-61: Update the entries computed in the resource collection
field component to pass the field definition’s jsonPathExpression along with
collectionPath() to getResourceValueByJsonPath, preserving the fallback to
property so both path configurations resolve correctly.
In `@projects/ngx/declarative-ui/resource-field/resource-field.component.ts`:
- Around line 33-54: Reorder the `@Component` decorator keys in
ResourceFieldComponent to satisfy sort-keys-in-type-decorator: move host after
encapsulation, while preserving the existing imports, template, changeDetection,
and schemas configuration.
---
Nitpick comments:
In
`@projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.html`:
- Line 2: Replace track $index in the `@for` loop of
ResourceCollectionFieldComponent with a stable unique property from each entry,
such as entry.id or entry.name, so DOM and expanded state remain associated with
the correct item after reordering or middle insertion/deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bee59355-7836-4279-a0a4-c167c3d1dc4d
📒 Files selected for processing (9)
projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.htmlprojects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.scssprojects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.spec.tsprojects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.tsprojects/ngx/declarative-ui/resource-field/resource-field.component.htmlprojects/ngx/declarative-ui/resource-field/resource-field.component.scssprojects/ngx/declarative-ui/resource-field/resource-field.component.spec.tsprojects/ngx/declarative-ui/resource-field/resource-field.component.tsprojects/ngx/declarative-ui/stories/declarative-table-card.stories.ts
Comment on lines
+56
to
+61
| protected readonly entries = computed<Record<string, unknown>[]>(() => { | ||
| const value = getResourceValueByJsonPath(this.resource() ?? {}, { | ||
| property: this.collectionPath(), | ||
| }); | ||
| return Array.isArray(value) ? (value as Record<string, unknown>[]) : []; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
entries computed ignores jsonPathExpression from the field definition.
getResourceValueByJsonPath checks field.jsonPathExpression || field.property, but only property is passed here. If a collection field uses jsonPathExpression instead of (or alongside) property, the array will silently resolve to [] with no error logged.
🔧 Proposed fix
protected readonly entries = computed<Record<string, unknown>[]>(() => {
- const value = getResourceValueByJsonPath(this.resource() ?? {}, {
- property: this.collectionPath(),
- });
+ const fd = this.fieldDefinition();
+ const value = getResourceValueByJsonPath(this.resource() ?? {}, {
+ jsonPathExpression: fd.jsonPathExpression,
+ property: this.collectionPath(),
+ });
return Array.isArray(value) ? (value as Record<string, unknown>[]) : [];
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected readonly entries = computed<Record<string, unknown>[]>(() => { | |
| const value = getResourceValueByJsonPath(this.resource() ?? {}, { | |
| property: this.collectionPath(), | |
| }); | |
| return Array.isArray(value) ? (value as Record<string, unknown>[]) : []; | |
| }); | |
| protected readonly entries = computed<Record<string, unknown>[]>(() => { | |
| const fd = this.fieldDefinition(); | |
| const value = getResourceValueByJsonPath(this.resource() ?? {}, { | |
| jsonPathExpression: fd.jsonPathExpression, | |
| property: this.collectionPath(), | |
| }); | |
| return Array.isArray(value) ? (value as Record<string, unknown>[]) : []; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/ngx/declarative-ui/resource-field/resource-collection-field/resource-collection-field.component.ts`
around lines 56 - 61, Update the entries computed in the resource collection
field component to pass the field definition’s jsonPathExpression along with
collectionPath() to getResourceValueByJsonPath, preserving the fallback to
property so both path configurations resolve correctly.
Comment on lines
+33
to
54
| imports: [ | ||
| Icon, | ||
| BooleanValue, | ||
| LinkValue, | ||
| SecretValue, | ||
| Button, | ||
| TagListValue, | ||
| // `ResourceCollectionField` imports this component back (each expanded | ||
| // card renders its sub-fields via `mfp-resource-field`), so the two form | ||
| // a module-init cycle. `forwardRef` defers resolution until the template | ||
| // is instantiated, by which point both classes are defined. | ||
| forwardRef(() => ResourceCollectionField), | ||
| ], | ||
| templateUrl: './resource-field.component.html', | ||
| styleUrl: './resource-field.component.scss', | ||
| host: { | ||
| '[class.resource-field--collection]': 'isCollection()', | ||
| }, | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| encapsulation: ViewEncapsulation.ShadowDom, | ||
| schemas: [CUSTOM_ELEMENTS_SCHEMA], | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix @Component decorator key ordering to satisfy ESLint.
Static analysis reports an @angular-eslint/sort-keys-in-type-decorator error: host should appear after encapsulation, not before changeDetection. This is a lint error that may fail CI.
🔧 Proposed fix
`@Component`({
selector: 'mfp-resource-field',
imports: [
Icon,
BooleanValue,
LinkValue,
SecretValue,
Button,
TagListValue,
forwardRef(() => ResourceCollectionField),
],
templateUrl: './resource-field.component.html',
styleUrl: './resource-field.component.scss',
- host: {
- '[class.resource-field--collection]': 'isCollection()',
- },
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.ShadowDom,
+ host: {
+ '[class.resource-field--collection]': 'isCollection()',
+ },
schemas: [CUSTOM_ELEMENTS_SCHEMA],
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| imports: [ | |
| Icon, | |
| BooleanValue, | |
| LinkValue, | |
| SecretValue, | |
| Button, | |
| TagListValue, | |
| // `ResourceCollectionField` imports this component back (each expanded | |
| // card renders its sub-fields via `mfp-resource-field`), so the two form | |
| // a module-init cycle. `forwardRef` defers resolution until the template | |
| // is instantiated, by which point both classes are defined. | |
| forwardRef(() => ResourceCollectionField), | |
| ], | |
| templateUrl: './resource-field.component.html', | |
| styleUrl: './resource-field.component.scss', | |
| host: { | |
| '[class.resource-field--collection]': 'isCollection()', | |
| }, | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| encapsulation: ViewEncapsulation.ShadowDom, | |
| schemas: [CUSTOM_ELEMENTS_SCHEMA], | |
| }) | |
| imports: [ | |
| Icon, | |
| BooleanValue, | |
| LinkValue, | |
| SecretValue, | |
| Button, | |
| TagListValue, | |
| // `ResourceCollectionField` imports this component back (each expanded | |
| // card renders its sub-fields via `mfp-resource-field`), so the two form | |
| // a module-init cycle. `forwardRef` defers resolution until the template | |
| // is instantiated, by which point both classes are defined. | |
| forwardRef(() => ResourceCollectionField), | |
| ], | |
| templateUrl: './resource-field.component.html', | |
| styleUrl: './resource-field.component.scss', | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| encapsulation: ViewEncapsulation.ShadowDom, | |
| host: { | |
| '[class.resource-field--collection]': 'isCollection()', | |
| }, | |
| schemas: [CUSTOM_ELEMENTS_SCHEMA], | |
| }) |
🧰 Tools
🪛 ESLint
[error] 48-50: Keys in @Component decorator should be ordered: selector, imports, templateUrl, styleUrl, changeDetection, encapsulation, host, schemas
(@angular-eslint/sort-keys-in-type-decorator)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/ngx/declarative-ui/resource-field/resource-field.component.ts`
around lines 33 - 54, Reorder the `@Component` decorator keys in
ResourceFieldComponent to satisfy sort-keys-in-type-decorator: move host after
encapsulation, while preserving the existing imports, template, changeDetection,
and schemas configuration.
Source: Linters/SAST tools
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add collection display to the resource filed component
Summary by CodeRabbit
New Features
Bug Fixes
Tests