Table card search and scopes#182
Conversation
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds scoped search support to ChangesTableCard search API and related updates
Sequence Diagram(s)sequenceDiagram
participant Config as searchConfig
participant TCS as TableCardSearch
participant UI5 as ui5-search
participant Output as outputs
Config->>TCS: initialScopeValue / value / scopes
UI5->>TCS: input
TCS->>TCS: update searchControl
TCS->>Output: searchChanged (debounced)
UI5->>TCS: submit
TCS->>Output: searchSubmit
UI5->>TCS: scope change
TCS->>Output: scopeChanged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
projects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.ts (1)
703-710: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert delegated child output forwarding, not just emitter existence.
These tests pass even if the template bindings for
searchSubmitandscopeChangedare removed. Emit from the renderedmfp-table-card-searchtest node and assert the parent outputs receive the same payload.🤖 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/table-card/declarative-table-card.component.spec.ts` around lines 703 - 710, The tests for exposes searchSubmit output and exposes scopeChanged output only verify that the emit functions exist on the component outputs, but do not test that the parent component actually receives and forwards the events from the child mfp-table-card-search component. Replace these tests to emit events from the rendered child component in the template, subscribe to or spy on the parent component's searchSubmit and scopeChanged outputs, verify that the parent outputs receive the same event payloads that were emitted from the child, ensuring true integration between parent and child component outputs.
🤖 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/stories/declarative-table-card.stories.ts`:
- Around line 480-508: The DeclarativeTableCardSearchStory component needs to be
updated to follow Angular component guidelines. Add `standalone: true` and
`changeDetection: ChangeDetectionStrategy.OnPush` to the component decorator
configuration. Replace the `@Input`() decorators for the config and resources
properties with signal-based inputs using the input() function instead. Import
ChangeDetectionStrategy from '`@angular/core`' and the input function from
'`@angular/core`' to enable these changes.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.html`:
- Around line 6-7: The scopeValue binding in the ui5-search component should be
connected to the component's live activeScope() method instead of
searchConfig().scopeValue to ensure the scope selection persists when the search
element is recreated during collapse/re-expand cycles. Update the [scopeValue]
binding property in the ui5-search template to use activeScope() as its source,
and apply this same change to the other occurrence mentioned at lines 27-28 to
keep both search element instances synchronized with the internal scope state.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.ts`:
- Around line 59-60: The ESLint suppressions for
`@typescript-eslint/no-explicit-any` at lines 59, 66, 74, 83, 85, 93, 353, 366,
380, 392, and 395 are missing required explanatory comments and TODO statements
per coding guidelines. Either add a documented rationale comment above each
suppression explaining why the any cast is necessary and include a TODO
indicating when it should be removed, or refactor the test assertions to rely on
public component outputs and DOM assertions instead of white-box access to
internal state properties like searchExpanded(), searchCollapsing(),
searchControl, and searchState().
- Line 4: In the table-card-search.component.spec.ts file, remove
NO_ERRORS_SCHEMA from the import statement at the top of the file where
CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA are imported together from
`@angular/core`. Then, locate the TestBed configuration (typically in a beforeEach
setup function) where schemas array is defined and remove NO_ERRORS_SCHEMA from
that array, leaving only CUSTOM_ELEMENTS_SCHEMA. This aligns the test
configuration with the component's own schema configuration and respects the
strictTemplates Angular setting.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts`:
- Around line 55-77: The searchControl form control is initialized to an empty
string and never updated when the searchConfig signal changes, breaking the
ability for parent components to control the search value through
searchConfig.value. Update the existing effect in the constructor that currently
only syncs activeScope to also sync the searchConfig().value to the
searchControl. Use patchValue on searchControl to update its value when
searchConfig().value changes, ensuring that parent-driven value control works as
intended.
---
Nitpick comments:
In
`@projects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.ts`:
- Around line 703-710: The tests for exposes searchSubmit output and exposes
scopeChanged output only verify that the emit functions exist on the component
outputs, but do not test that the parent component actually receives and
forwards the events from the child mfp-table-card-search component. Replace
these tests to emit events from the rendered child component in the template,
subscribe to or spy on the parent component's searchSubmit and scopeChanged
outputs, verify that the parent outputs receive the same event payloads that
were emitted from the child, ensuring true integration between parent and child
component outputs.
🪄 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: 8962f7b9-8b0e-4e99-9194-62157353bda2
📒 Files selected for processing (15)
docs/declarative-table-card.mdprojects/ngx/declarative-ui/stories/declarative-table-card.stories.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.htmlprojects/ngx/declarative-ui/table-card/declarative-table-card.component.scssprojects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/index.tsprojects/ngx/declarative-ui/table-card/models/configs.tsprojects/ngx/declarative-ui/table-card/models/search-config.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.htmlprojects/ngx/declarative-ui/table-card/search/table-card-search.component.scssprojects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.tsprojects/ngx/declarative-ui/table/models/index.tsprojects/ngx/declarative-ui/table/models/table-config.ts
💤 Files with no reviewable changes (1)
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.scss
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
|
If we go with this implementation then:
|
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
@gkrajniak live inside ui-definition
Done |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts (1)
13-13: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign the story wrapper with Angular component rules (
standalone, OnPush, signal inputs).
DeclarativeTableCardCreateStorystill uses@Input()+OnInitinitialization, and the shown decorator does not includestandalone: true/changeDetection: ChangeDetectionStrategy.OnPush. Please migrate to signal inputs (input()) and initialize derived state viaeffect()instead ofngOnInit.Suggested fix
-import { Component, Input, OnInit } from '`@angular/core`'; +import { ChangeDetectionStrategy, Component, effect, input } from '`@angular/core`'; @@ `@Component`({ + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, template: ` @@ }) -class DeclarativeTableCardCreateStory implements OnInit { - `@Input`() config!: TableCardConfig; - `@Input`() resources: GenericResource[] = []; +class DeclarativeTableCardCreateStory { + config = input.required<TableCardConfig>(); + resources = input<GenericResource[]>([]); @@ - ngOnInit(): void { - this.searchTerm = this.config?.searchConfig?.value ?? ''; - this.activeScope = this.config?.searchConfig?.scopeValue; - } + constructor() { + effect(() => { + const searchConfig = this.config().searchConfig; + this.searchTerm = searchConfig?.value ?? ''; + this.activeScope = searchConfig?.scopeValue; + }); + } @@ - if (!sc) return this.resources; + if (!sc) return this.resources(); @@ - let result = this.resources as Pod[]; + let result = this.resources() as Pod[];As per coding guidelines: "
**/*.{ts,tsx}: Use standalone components withstandalone: trueconfiguration", "Use signal-based APIs:input(),output(),model(),computed(),effect()", and "Use OnPush change detection on all components".Also applies to: 168-170, 176-179
🤖 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/stories/declarative-table-card.stories.ts` at line 13, The DeclarativeTableCardCreateStory component uses outdated Angular patterns and needs to be modernized. Replace all `@Input`() decorators with signal input() calls, remove the OnInit interface from the class implements clause, and delete the ngOnInit method. Instead, use effect() to handle any derived state initialization that was previously in ngOnInit. Update the component decorator to include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush. Make sure to import the necessary signal functions (input, effect) from `@angular/core` and import ChangeDetectionStrategy from `@angular/core`.Source: Coding guidelines
🤖 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/table-card/search/table-card-search.component.html`:
- Around line 13-14: The `@for` loop tracking key in the scopes iteration uses
s.value, which is optional and can cause collisions when multiple items have
undefined values, leading to incorrect DOM diffing. Replace the track expression
from track s.value to track $index to use the loop index as a collision-safe
tracking key, ensuring each DOM element maintains its identity correctly even
when value properties are undefined or duplicated.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts`:
- Around line 69-72: The fixSelectWidth() workaround is currently applied only
once on initial render within the setTimeout block. When searchConfig().scopes
is updated or changed after the component initializes, the workaround does not
run again, causing long scope labels to truncate. Add a reactive effect or
watcher that monitors changes to searchConfig().scopes and triggers
fixSelectWidth() whenever the scopes change, ensuring the select width
adjustment is applied every time the scopes are updated.
---
Duplicate comments:
In `@projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts`:
- Line 13: The DeclarativeTableCardCreateStory component uses outdated Angular
patterns and needs to be modernized. Replace all `@Input`() decorators with signal
input() calls, remove the OnInit interface from the class implements clause, and
delete the ngOnInit method. Instead, use effect() to handle any derived state
initialization that was previously in ngOnInit. Update the component decorator
to include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush.
Make sure to import the necessary signal functions (input, effect) from
`@angular/core` and import ChangeDetectionStrategy from `@angular/core`.
🪄 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: 67fad9bf-0490-4bc5-b414-2b1238b79e66
📒 Files selected for processing (11)
projects/ngx/declarative-ui/stories/declarative-table-card.stories.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.htmlprojects/ngx/declarative-ui/table-card/declarative-table-card.component.scssprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/models/configs.tsprojects/ngx/declarative-ui/table-card/models/search-config.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.htmlprojects/ngx/declarative-ui/table-card/search/table-card-search.component.scssprojects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.tsprojects/ngx/declarative-ui/table/models/table-config.ts
💤 Files with no reviewable changes (4)
- projects/ngx/declarative-ui/table-card/models/search-config.ts
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.html
- projects/ngx/declarative-ui/table-card/models/configs.ts
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.ts
✅ Files skipped from review due to trivial changes (1)
- projects/ngx/declarative-ui/table-card/search/table-card-search.component.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/ngx/declarative-ui/table/models/table-config.ts
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
|
the idea behind the predefined filters is to be able to build search queries, fq: (member:"C5343390") AND accountRole:"Project" that's why there was initial proposal to use in the event the it the current solution the export interface FieldFielterDefinition/Scope {
label: string;
property?: string | string[];
value: string;
}once having it the search query can be better produce: fq: property=value&qtext=value |
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
Signed-off-by: gkrajniak <gkrajniak@gmail.com>
| buttonSettings?: TableCardButtonSettings; | ||
| /** When `true`, shows the search input and button in the card toolbar. */ | ||
| resourcesSearchable?: boolean; | ||
| /** When set, renders a `<ui5-search>` in the card toolbar. Replaces the previous `resourcesSearchable` flag. */ |
There was a problem hiding this comment.
this part of the comment is not needed Replaces the previousresourcesSearchable flag.
| | `searchChanged` | `string` | Emits 300 ms after the search input changes | | ||
| | `searchChanged` | `{ value: string; scope?: string }` | Emits 300 ms after the search input changes; `scope` reflects the currently active scope (if any) | | ||
| | `searchSubmit` | `{ value: string; scope?: string }` | Emits synchronously when the user submits the search (Enter or search icon) | | ||
| | `scopeChanged` | `{ value: string; scope?: string }` | Emits synchronously when the user picks a different scope from the dropdown; `value` is the current in-flight search text | |
There was a problem hiding this comment.
here for example think how would you be able to produce a query based on the scope the scope should indicate exact object property to which the value belongs
| ```ts | ||
| interface TableCardConfig { | ||
| header: string; | ||
| header?: string; |
There was a problem hiding this comment.
I thought I was changing that already .... moght has been lost somewhere
| placeholder: 'Search pods…', | ||
| scopes: [ | ||
| { label: 'All', value: 'all' }, | ||
| { label: 'My Contributions', value: 'mine' }, |
There was a problem hiding this comment.
how generically build a query having only a value, it needs to refer to the property by which we filter (or there was a different solution in mind)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/declarative-table-card.md (2)
336-347: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
standalone: trueandOnPushto the second Angular example.Same issue as the first example — missing required
standalone: trueandchangeDetection: ChangeDetectionStrategy.OnPush.`@Component`({ + standalone: true, imports: [DeclarativeTableCard], + changeDetection: ChangeDetectionStrategy.OnPush, template: `🤖 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 `@docs/declarative-table-card.md` around lines 336 - 347, The second Angular example using `@Component` and DeclarativeTableCard is missing the required standalone setup and OnPush change detection. Update that example to include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush, matching the first example’s component configuration while keeping the existing imports and template unchanged.Source: Coding guidelines
79-98: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
standalone: trueandOnPushto the Angular example.The
@Componentdecorator is missingstandalone: trueandchangeDetection: ChangeDetectionStrategy.OnPush. As per coding guidelines, all components must be standalone withstandalone: trueand useOnPushchange detection. Documentation examples should reflect these requirements so consumers copy-paste correct patterns.`@Component`({ + standalone: true, imports: [DeclarativeTableCard], + changeDetection: ChangeDetectionStrategy.OnPush, template: `🤖 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 `@docs/declarative-table-card.md` around lines 79 - 98, Add standalone component metadata to the Angular example in the `@Component` decorator for DeclarativeTableCard usage: include standalone: true and set changeDetection to ChangeDetectionStrategy.OnPush. Update the example so it matches the project’s required component pattern, keeping the existing template and imports while referencing the same `@Component`, DeclarativeTableCard, and ChangeDetectionStrategy symbols.Source: Coding guidelines
projects/ngx/declarative-ui/table-card/declarative-table-card.component.ts (1)
57-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the search output docs with the new payload types.
The outputs no longer carry combined
{ value, scope }data:searchChanged/searchSubmitemit only the value, andscopeChangedemits only the scope.Proposed fix
/** * Emitted after the user types into the search input, debounced by 300 ms. - * Carries the current input value and the active scope (if any). + * Carries the current input value. */ readonly searchChanged = output<string | null>(); /** * Emitted synchronously when the user submits the search (presses Enter or - * clicks the search icon). Carries the submitted value and the active scope. + * clicks the search icon). Carries the submitted value. */ readonly searchSubmit = output<string | null>(); /** * Emitted synchronously when the user selects a different scope in the - * scopes dropdown. Carries the current in-flight search text and the new scope. + * scopes dropdown. Carries the selected scope, if any. */ readonly scopeChanged = output<Scope | undefined>();🤖 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/table-card/declarative-table-card.component.ts` around lines 57 - 71, Update the output documentation on DeclarativeTableCardComponent so it matches the current payload types: searchChanged and searchSubmit now emit only the search value, and scopeChanged emits only the scope. Adjust the JSDoc in DeclarativeTableCardComponent to remove references to combined { value, scope } payloads and to describe the actual emitted data for each output consistently.
🧹 Nitpick comments (1)
docs/declarative-table-card.md (1)
351-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider initializing
currentScopeto matchinitialScopeValue.Since
scopeChangedonly fires on user interaction, the host never receives an event for the initial scope. If yourreload()logic needs the scope filter from the start, initializecurrentScopeto matchinitialScopeValue:private currentQuery = ''; - private currentScope: Scope | undefined; + private currentScope: Scope | undefined = this.ALL_SCOPE;🤖 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 `@docs/declarative-table-card.md` around lines 351 - 366, The initial scope state is not aligned with the configured initial scope, so the host never sees the starting filter. In DeclarativeTableCard, initialize currentScope to the same Scope selected by initialScopeValue (for example from ALL_SCOPE or MINE_SCOPE) so reload() starts with the correct scope filter. Update the component’s scope initialization logic alongside currentQuery/currentScope so the first render and subsequent scopeChanged behavior stay consistent.
🤖 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 `@docs/declarative-table-card.md`:
- Around line 265-266: The `initialScopeValue` docs are inaccurate: it is not
matched against `scopes[].id` on init, because `table-card-search.component.ts`
sets it directly via `this.activeScope.set(config.initialScopeValue)`. Update
the `initialScopeValue` description in `declarative-table-card.md` to say it
should be a `Scope` object from `scopes` (or an equivalent `Scope`-shaped
value), and reserve the `id`-based lookup wording for the dropdown change path
in `onSearchScopeChange`.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts`:
- Around line 191-199: The clear flow in table-card-search.component.ts is
emitting the empty search twice because the native input handler in the input
listener calls searchChanged.emit('') immediately and then
searchControl.setValue(next) triggers the valueChanges debounce path again.
Update the clear branch in the input listener to set the control without
re-emitting events (use emitEvent: false when next is empty) so the existing
valueChanges subscription does not fire a second delayed empty search, while
preserving the immediate emit for the clear action.
- Around line 65-73: The effect in `TableCardSearchComponent` is syncing parent
config into `searchControl` in a way that triggers the `valueChanges`
subscription and causes extra `searchChanged` emissions. Update the `effect` so
parent-driven state (`searchConfig`, `externalValue`) is applied separately from
the form control update, and make the `searchControl.setValue` call suppress
emission when the change comes from config sync. Keep the user-initiated path in
the `valueChanges` subscription intact so only real user edits emit
`searchChanged`.
- Around line 95-100: Unify the search scope contract so the template and
handler both use Scope.value instead of Scope.id. Update
table-card-search.component.html where the ui5-search-scope binding is set, and
update table-card-search.component.ts in onSearchScopeChange so the lookup
compares the emitted scopeValue against scope.value before setting activeScope
and emitting scopeChanged.
---
Outside diff comments:
In `@docs/declarative-table-card.md`:
- Around line 336-347: The second Angular example using `@Component` and
DeclarativeTableCard is missing the required standalone setup and OnPush change
detection. Update that example to include standalone: true and changeDetection:
ChangeDetectionStrategy.OnPush, matching the first example’s component
configuration while keeping the existing imports and template unchanged.
- Around line 79-98: Add standalone component metadata to the Angular example in
the `@Component` decorator for DeclarativeTableCard usage: include standalone:
true and set changeDetection to ChangeDetectionStrategy.OnPush. Update the
example so it matches the project’s required component pattern, keeping the
existing template and imports while referencing the same `@Component`,
DeclarativeTableCard, and ChangeDetectionStrategy symbols.
In `@projects/ngx/declarative-ui/table-card/declarative-table-card.component.ts`:
- Around line 57-71: Update the output documentation on
DeclarativeTableCardComponent so it matches the current payload types:
searchChanged and searchSubmit now emit only the search value, and scopeChanged
emits only the scope. Adjust the JSDoc in DeclarativeTableCardComponent to
remove references to combined { value, scope } payloads and to describe the
actual emitted data for each output consistently.
---
Nitpick comments:
In `@docs/declarative-table-card.md`:
- Around line 351-366: The initial scope state is not aligned with the
configured initial scope, so the host never sees the starting filter. In
DeclarativeTableCard, initialize currentScope to the same Scope selected by
initialScopeValue (for example from ALL_SCOPE or MINE_SCOPE) so reload() starts
with the correct scope filter. Update the component’s scope initialization logic
alongside currentQuery/currentScope so the first render and subsequent
scopeChanged behavior stay consistent.
🪄 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: 4fcd987d-a91f-4809-a5c4-97897e6951ee
📒 Files selected for processing (15)
docs/declarative-table-card.mdnodemon.jsonpackage.jsonprojects/ngx/declarative-ui/models/index.tsprojects/ngx/declarative-ui/models/ui-definition.tsprojects/ngx/declarative-ui/stories/declarative-table-card.stories.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/models/search-config.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.htmlprojects/ngx/declarative-ui/table-card/search/table-card-search.component.scssprojects/ngx/declarative-ui/table-card/search/table-card-search.component.tsprojects/ngx/declarative-ui/table/models/index.tsprojects/ngx/declarative-ui/table/models/table-config.tsprojects/ngx/declarative-ui/tsconfig.lib.jsonprojects/ngx/tsconfig.lib.json
💤 Files with no reviewable changes (1)
- projects/ngx/declarative-ui/table-card/search/table-card-search.component.scss
✅ Files skipped from review due to trivial changes (2)
- projects/ngx/declarative-ui/table-card/models/search-config.ts
- projects/ngx/declarative-ui/models/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/ngx/declarative-ui/table-card/search/table-card-search.component.html
- projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts
| /** Initial / controlled scope — must be one of the entries in `scopes`. Matched against `scopes[].id`. */ | ||
| initialScopeValue?: Scope; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the initialScopeValue description.
The documentation claims initialScopeValue is "Matched against scopes[].id," but the component (see table-card-search.component.ts) directly assigns the object via this.activeScope.set(config.initialScopeValue) without matching by id. The id-based lookup only happens when the user changes the scope via the dropdown (onSearchScopeChange). Update the description to clarify that initialScopeValue should be a Scope object from scopes (or an equivalent object), not an id string to be matched.
- /** Initial / controlled scope — must be one of the entries in `scopes`. Matched against `scopes[].id`. */
+ /** Initial / controlled scope object. Should reference one of the objects in `scopes` (or an equivalent `Scope` value). */📝 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.
| /** Initial / controlled scope — must be one of the entries in `scopes`. Matched against `scopes[].id`. */ | |
| initialScopeValue?: Scope; | |
| /** Initial / controlled scope object. Should reference one of the objects in `scopes` (or an equivalent `Scope` value). */ | |
| initialScopeValue?: Scope; |
🤖 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 `@docs/declarative-table-card.md` around lines 265 - 266, The
`initialScopeValue` docs are inaccurate: it is not matched against `scopes[].id`
on init, because `table-card-search.component.ts` sets it directly via
`this.activeScope.set(config.initialScopeValue)`. Update the `initialScopeValue`
description in `declarative-table-card.md` to say it should be a `Scope` object
from `scopes` (or an equivalent `Scope`-shaped value), and reserve the
`id`-based lookup wording for the dropdown change path in `onSearchScopeChange`.
| effect(() => { | ||
| const config = this.searchConfig(); | ||
| this.activeScope.set(config.initialScopeValue); | ||
|
|
||
| const nextValue = config.value ?? ''; | ||
| if (this.searchControl.value !== nextValue) { | ||
| this.searchControl.setValue(nextValue); | ||
| this.externalValue.set(nextValue); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '1,100p' projects/ngx/declarative-ui/table-card/search/table-card-search.component.tsRepository: openmfp/webcomponents
Length of output: 3540
Suppress spurious searchChanged emissions during parent-driven config sync.
The effect block currently calls searchControl.setValue, which triggers the valueChanges subscription and emits searchChanged. This causes the parent to receive duplicate or incorrect updates when the searchConfig.value is updated programmatically or mirrors user input.
Separate the externalValue signal update from the control update and suppress event emission in setValue when syncing from the parent config.
Proposed fix
const nextValue = config.value ?? '';
+ if (this.externalValue() !== nextValue) {
+ this.externalValue.set(nextValue);
+ }
+
if (this.searchControl.value !== nextValue) {
- this.searchControl.setValue(nextValue);
- this.externalValue.set(nextValue);
+ this.searchControl.setValue(nextValue, { emitEvent: false });
}📝 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.
| effect(() => { | |
| const config = this.searchConfig(); | |
| this.activeScope.set(config.initialScopeValue); | |
| const nextValue = config.value ?? ''; | |
| if (this.searchControl.value !== nextValue) { | |
| this.searchControl.setValue(nextValue); | |
| this.externalValue.set(nextValue); | |
| } | |
| effect(() => { | |
| const config = this.searchConfig(); | |
| this.activeScope.set(config.initialScopeValue); | |
| const nextValue = config.value ?? ''; | |
| if (this.externalValue() !== nextValue) { | |
| this.externalValue.set(nextValue); | |
| } | |
| if (this.searchControl.value !== nextValue) { | |
| this.searchControl.setValue(nextValue, { emitEvent: false }); | |
| } |
🤖 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/table-card/search/table-card-search.component.ts`
around lines 65 - 73, The effect in `TableCardSearchComponent` is syncing parent
config into `searchControl` in a way that triggers the `valueChanges`
subscription and causes extra `searchChanged` emissions. Update the `effect` so
parent-driven state (`searchConfig`, `externalValue`) is applied separately from
the form control update, and make the `searchControl.setValue` call suppress
emission when the change comes from config sync. Keep the user-initiated path in
the `valueChanges` subscription intact so only real user edits emit
`searchChanged`.
| onSearchScopeChange(event: Event): void { | ||
| const target = event.target as Ui5SearchEventTarget | null; | ||
| const scopeId = target?.scopeValue || undefined; | ||
| const scope = this.searchConfig().scopes?.find((e) => e.id === scopeId); | ||
| this.activeScope.set(scope); | ||
| this.scopeChanged.emit(scope); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect UI5 search scope value binding and handler usage.
rg -n -C2 'ui5-search-scope|scopeValue|value=' \
projects/ngx/declarative-ui/table-card/search \
projects/ngx/declarative-ui/table-card/modelsRepository: openmfp/webcomponents
Length of output: 9827
Unify scope binding to use Scope.value across template and handler.
The SearchScope model explicitly documents that the value field drives the ui5-search-scope event. The template currently binds [value]="s.id", and the handler compares the resulting scopeValue against scope.id. While currently consistent, this violates the defined contract; if id and value diverge, the system fails to align with the model's intent. The proposed fix must align both the template and the handler to use scope.value.
Required changes:
- Update
table-card-search.component.htmlto bind[value]="s.value". - Update
table-card-search.component.tsto resolve scopes usingscope.value.
Full corrected fix
diff --git a/projects/ngx/declarative-ui/table-card/search/table-card-search.component.html b/projects/ngx/declarative-ui/table-card/search/table-card-search.component.html
--- a/projects/ngx/declarative-ui/table-card/search/table-card-search.component.html
+++ b/projects/ngx/declarative-ui/table-card/search/table-card-search.component.html
@@ -10,7 +10,7 @@
<ui5-search
[value]="externalValue()"
>
`@for` (s of searchConfig().scopes ?? []; track s.id) {
- <ui5-search-scope slot="scopes" [text]="s.label" [value]="s.id" />
+ <ui5-search-scope slot="scopes" [text]="s.label" [value]="s.value" />
</ui5-search>
</ui5-search>
diff --git a/projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts b/projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts
--- a/projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts
+++ b/projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts
@@ -95,8 +95,8 @@
onSearchScopeChange(event: Event): void {
const target = event.target as Ui5SearchEventTarget | null;
- const scopeId = target?.scopeValue || undefined;
- const scope = this.searchConfig().scopes?.find((e) => e.id === scopeId);
+ const scopeValue = target?.scopeValue || undefined;
+ const scope = this.searchConfig().scopes?.find((scope) => scope.value === scopeValue);
this.activeScope.set(scope);
this.scopeChanged.emit(scope);
}📝 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.
| onSearchScopeChange(event: Event): void { | |
| const target = event.target as Ui5SearchEventTarget | null; | |
| const scopeId = target?.scopeValue || undefined; | |
| const scope = this.searchConfig().scopes?.find((e) => e.id === scopeId); | |
| this.activeScope.set(scope); | |
| this.scopeChanged.emit(scope); | |
| onSearchScopeChange(event: Event): void { | |
| const target = event.target as Ui5SearchEventTarget | null; | |
| const scopeValue = target?.scopeValue || undefined; | |
| const scope = this.searchConfig().scopes?.find((scope) => scope.value === scopeValue); | |
| this.activeScope.set(scope); | |
| this.scopeChanged.emit(scope); | |
| } |
🤖 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/table-card/search/table-card-search.component.ts`
around lines 95 - 100, Unify the search scope contract so the template and
handler both use Scope.value instead of Scope.id. Update
table-card-search.component.html where the ui5-search-scope binding is set, and
update table-card-search.component.ts in onSearchScopeChange so the lookup
compares the emitted scopeValue against scope.value before setting activeScope
and emitting scopeChanged.
| nativeEl.addEventListener('input', () => { | ||
| const next = nativeEl.value ?? ''; | ||
| if (this.searchControl.value !== next) { | ||
| this.searchControl.setValue(next); | ||
| } | ||
| // Fire the parent notification immediately on clear (skipping the | ||
| // 300ms debounce that exists for typed input). | ||
| if (next === '') { | ||
| this.searchChanged.emit(''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n projects/ngx/declarative-ui/table-card/search/table-card-search.component.tsRepository: openmfp/webcomponents
Length of output: 8736
Avoid double-emitting empty search clears
When the clear icon is clicked, the immediate searchChanged.emit('') at line 199 fires immediately, while setValue(next) at line 194 also triggers the debounced valueChanges subscription at line 62, causing a second emission 300ms later.
Use emitEvent: false during clear operations to suppress the redundant debounced emission:
Proposed fix
const next = nativeEl.value ?? '';
if (this.searchControl.value !== next) {
- this.searchControl.setValue(next);
+ this.searchControl.setValue(next, { emitEvent: next !== '' });
}This ensures the clear action emits exactly once (immediately) instead of twice (immediate + delayed).
📝 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.
| nativeEl.addEventListener('input', () => { | |
| const next = nativeEl.value ?? ''; | |
| if (this.searchControl.value !== next) { | |
| this.searchControl.setValue(next); | |
| } | |
| // Fire the parent notification immediately on clear (skipping the | |
| // 300ms debounce that exists for typed input). | |
| if (next === '') { | |
| this.searchChanged.emit(''); | |
| nativeEl.addEventListener('input', () => { | |
| const next = nativeEl.value ?? ''; | |
| if (this.searchControl.value !== next) { | |
| this.searchControl.setValue(next, { emitEvent: next !== '' }); | |
| } | |
| // Fire the parent notification immediately on clear (skipping the | |
| // 300ms debounce that exists for typed input). | |
| if (next === '') { | |
| this.searchChanged.emit(''); |
🤖 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/table-card/search/table-card-search.component.ts`
around lines 191 - 199, The clear flow in table-card-search.component.ts is
emitting the empty search twice because the native input handler in the input
listener calls searchChanged.emit('') immediately and then
searchControl.setValue(next) triggers the valueChanges debounce path again.
Update the clear branch in the input listener to set the control without
re-emitting events (use emitEvent: false when next is empty) so the existing
valueChanges subscription does not fire a second delayed empty search, while
preserving the immediate emit for the clear action.
2 participants
Known limitations: there is currently no way to natively extend the width of the scope select. Long labels get truncated. A DOM manipulation workaround can be applied inside the component, but it's fragile.
I have raised an issue in the ui5-webcomponents repository:
UI5/webcomponents#13719
Summary by CodeRabbit
Release Notes
New Features
searchConfig, with selectable scope dropdown filtering.searchChangednow emitsstring | null,searchSubmitadded, andscopeChangedadded.Refactor
Documentation
searchConfig, scope types, and revised event contract.Tests / Chores