feat: Grouped all search- and filter-related fields on TableCardConfig#188
feat: Grouped all search- and filter-related fields on TableCardConfig#188gkrajniak wants to merge 3 commits into
Conversation
…r a new searchConfig object, adding initialSearch and initialFilter as one-shot seeds so hosts can hydrate the search input and selected filter tab from external state (e.g. URL params) — previously impossible. Removed the now-redundant top-level resourcesSearchable (derived from searchConfig presence) and filterTabs (moved into searchConfig), a breaking change. Signed-off-by: gkrajniak <gkrajniak@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR consolidates table-card search/filter inputs under ChangesTableCardConfig search/filter consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/declarative-table-card.component.spec.ts`:
- Around line 322-323: The test file uses multiple inline
`@typescript-eslint/no-explicit-any` disables around `component as any`, which
violates the linting guidelines. Replace these accesses with a narrow test-only
internals type (for example, a `TestTableCardInternals` cast used in the
affected specs) or, if a disable must remain, add an explanatory reason plus a
TODO to remove it. Update all affected assertions and helpers in
`declarative-table-card.component.spec.ts` so the internals are accessed without
undocumented `any` usage.
- Around line 332-337: The seeded search test in
declarative-table-card.component.spec.ts is asserting too early to prove silence
because the debounce window has not elapsed. Update the spec around
setupWithSearchConfig and the searchChanged assertion to run under fakeAsync,
advance time with tick(301), and then verify searchEvents remains empty so the
test will fail if the initial seed emits.
In `@projects/ngx/declarative-ui/table-card/declarative-table-card.component.ts`:
- Around line 153-160: `DeclarativeTableCardComponent` is latching tab state too
early in the `getFilterTabs`/`seededFilterTabs` flow. Update the logic so
`initialFilter` is only consumed once when it actually exists, but do not mark
`hasSeededFilter` on the no-initial case; also stop returning a permanently
cached promoted array so later `searchConfig().filterTabs` changes can still
flow through. Apply the same one-shot-but-not-frozen behavior to the related
tab/filter handling in the other referenced blocks so `initialFilter` and tab
updates stay in sync.
🪄 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: 8ff12274-642f-4db0-a971-7dd3d9ca844f
📒 Files selected for processing (6)
projects/ngx/declarative-ui/form/declarative-form/declarative-form.component.tsprojects/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.spec.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/models/configs.ts
trigger rebuild Signed-off-by: Grzegorz Krajniak <gkrajniak@gmail.com>
…tial-search-and-filter-tab
Sobyt483
left a comment
Sobyt483
left a comment
There was a problem hiding this comment.
And please can you put less comments on code itself. I don't know why, but It feel so messy when i see so many comments in code
| // We also auto-expand the input so the seeded value is visible instead of | ||
| // hidden behind the collapsed toggle button. | ||
| effect(() => { | ||
| if (this.hasSeededSearch) return; |
There was a problem hiding this comment.
One small suggestion: the "run once" semantics can be expressed structurally with take(1) instead of a boolean flag, which removes the need for hasSeededSearch and the guard at the top of the effect:
toObservable(this.searchConfig)
.pipe(
map(config => config?.initialSearch),
filter((initial): initial is string => !!initial),
take(1),
takeUntilDestroyed()
)
.subscribe(initial => {
this.searchControl.setValue(initial, { emitEvent: false });
this.searchState.set('expanded');
});
Not blocking — the current approach works fine. Just a readability nit.
| * changes. Subsequent recomputations pass through untouched so user-driven | ||
| * tab clicks (which flow through `filterTabChanged`) aren't overridden. | ||
| */ | ||
| protected filterTabs = computed(() => { |
There was a problem hiding this comment.
I think here can be found hidden bug.
There's a subtle correctness bug in the initialFilter path. Once hasSeededFilter latches and seededFilterTabs is set, any subsequent change to filterTabs in the config is silently ignored — the computed always returns the cached promoted array:
if (this.hasSeededFilter) {
return this.seededFilterTabs ?? tabs; // tabs (fresh) is never used when seededFilterTabs is set
}
So if a host updates filterTabs after initial render (adds/removes a tab or translate), the new tabs never appear in the component.
Note that this only affects the initialFilter code path — when initialFilter is absent, seededFilterTabs stays undefined and ?? tabs returns the fresh value, so dynamic updates work fine. The inconsistency is hard to notice.
So I suggest to simplify here the API. We have default property for filter tab, so we can omit initialFilter (as they kind do same work) and user can just change default base on some logic like initial querry or something like this
2 participants
Summary by CodeRabbit
searchConfigfor table card search and filter tabs.initialSearchcan preset the search input, andinitialFiltercan preselect the matching filter tab.initialSearchvalues from triggering an extrasearchChangedevent on load.initialFilterseeding so default-tab promotion happens only once and won’t override later user selections.