WEB-896: Add debounced dynamic search to client list#3445
WEB-896: Add debounced dynamic search to client list#3445Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Search UI src/app/clients/clients.component.html |
Added #searchInput template ref, (input) bound to onSearchInput($event.target.value), retained (keydown.enter), and added a suffix icon button that calls search(searchInput.value). |
Component logic src/app/clients/clients.component.ts |
Component now implements OnDestroy; added searchInput$ Subject, destroy$, and clientsRequestSub; subscribes to searchInput$ with debounceTime(500) + distinctUntilChanged() + takeUntil(destroy$) to call search(...); onSearchInput(value: string) enqueues values; ngOnDestroy() unsubscribes/completes; MatIcon/MatIconButton added to imports. |
Tests src/app/clients/clients.component.spec.ts |
New Jest tests (fake timers) and mocked services to verify debounce timing and reset, duplicate suppression, immediate search(...) behavior, non-ASCII passthrough, and cancellation after ngOnDestroy(). |
Sequence Diagram
sequenceDiagram
participant User
participant Component as ClientsComponent
participant Subject as searchInput$ (Subject)
participant Pipeline as RxJS Pipeline
participant Service as ClientsService
User->>Component: types in search box
Component->>Subject: onSearchInput(value)
Subject->>Pipeline: emit value
Pipeline->>Pipeline: debounceTime(500ms), distinctUntilChanged()
Pipeline->>Component: trigger search(value)
Component->>Service: searchByText(value)
Service-->>Component: returns results
Component-->>User: render results
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: adding debounced dynamic search to the client list. It is clear, specific, and concise. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/clients/clients.component.ts (1)
137-148:⚠️ Potential issue | 🟠 MajorDuplicate network requests when Enter/button triggers search while debounce is pending.
When a user types and then immediately presses Enter or clicks the search button, two search requests fire:
search()is called directly (immediate)- The debounced pipeline also calls
search()after 500ms with the same value
distinctUntilChanged()only deduplicates consecutive emissions tosearchInput$, not invocations ofsearch().Proposed fix: Cancel pending debounce when search is triggered directly
+ private lastSearchedValue: string | null = null; + onSearchInput(value: string) { this.searchInput$.next(value); } /** * Searches server for query and resource. */ search(value: string) { + if (value === this.lastSearchedValue) { + return; + } + this.lastSearchedValue = value; this.filterText = value; this.resetPaginator(); this.getClients(); }Alternatively, clear the pending debounce by re-creating the subject or using a
switchMappattern that cancels in-flight debounces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.ts` around lines 137 - 148, The duplicate search happens because pressing Enter/click calls search() immediately while the debounced pipeline on searchInput$ is still pending; fix by cancelling the pending debounce before calling search(): add a member (e.g. this.searchInputDebounceSub) that holds the Subscription returned when you subscribe to searchInput$.pipe(debounceTime(500), distinctUntilChanged(), ...). In search(value: string) unsubscribe/complete that subscription (or emit to a takeUntil cancel Subject) to cancel the pending debounced emission, then proceed with resetPaginator() and getClients(); if you unsubscribe, recreate/resubscribe the debounced pipeline (where onSearchInput pushes into searchInput$) so future typing still debounces.
🧹 Nitpick comments (1)
src/app/clients/clients.component.spec.ts (1)
100-104: Add test for Enter/button press while debounce is pending.The tests verify immediate search works, but don't cover the scenario where a user types (triggering debounce) and then immediately presses Enter. This would help catch the duplicate request issue.
Suggested additional test case
it('should not double-fire when Enter is pressed while debounce is pending', () => { component.onSearchInput('dave'); jest.advanceTimersByTime(200); // debounce still pending component.search('dave'); // Enter pressed expect(clientsService.searchByText).toHaveBeenCalledTimes(1); jest.advanceTimersByTime(500); // let debounce complete // Should still be 1 call, not 2 (assuming the fix is applied) expect(clientsService.searchByText).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.spec.ts` around lines 100 - 104, Add a test in clients.component.spec.ts that simulates a user typing to trigger the debounce (call component.onSearchInput('dave')), advance timers by less than the debounce interval (jest.advanceTimersByTime(200)), then simulate an Enter/button press by calling component.search('dave'), and assert clientsService.searchByText was called exactly once; finally advance timers past the debounce (jest.advanceTimersByTime(500)) and assert clientsService.searchByText is still called only once to confirm no duplicate request. Use the existing component.search, component.onSearchInput and clientsService.searchByText mocks and ensure Jest fake timers are enabled for the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/clients/clients.component.html`:
- Around line 21-23: Replace the hardcoded aria-label on the search button with
a translated string using `@ngx-translate/core` (e.g., use the translate pipe or
TranslateService) so the label is internationalized; locate the button element
that calls search(searchInput.value) and change aria-label="Search" to something
like aria-label="{{ 'COMMON.SEARCH' | translate }}" (or bind via
[attr.aria-label]="translate.instant('COMMON.SEARCH')") and ensure the
translation key (COMMON.SEARCH) is added to your i18n JSON files.
---
Outside diff comments:
In `@src/app/clients/clients.component.ts`:
- Around line 137-148: The duplicate search happens because pressing Enter/click
calls search() immediately while the debounced pipeline on searchInput$ is still
pending; fix by cancelling the pending debounce before calling search(): add a
member (e.g. this.searchInputDebounceSub) that holds the Subscription returned
when you subscribe to searchInput$.pipe(debounceTime(500),
distinctUntilChanged(), ...). In search(value: string) unsubscribe/complete that
subscription (or emit to a takeUntil cancel Subject) to cancel the pending
debounced emission, then proceed with resetPaginator() and getClients(); if you
unsubscribe, recreate/resubscribe the debounced pipeline (where onSearchInput
pushes into searchInput$) so future typing still debounces.
---
Nitpick comments:
In `@src/app/clients/clients.component.spec.ts`:
- Around line 100-104: Add a test in clients.component.spec.ts that simulates a
user typing to trigger the debounce (call component.onSearchInput('dave')),
advance timers by less than the debounce interval
(jest.advanceTimersByTime(200)), then simulate an Enter/button press by calling
component.search('dave'), and assert clientsService.searchByText was called
exactly once; finally advance timers past the debounce
(jest.advanceTimersByTime(500)) and assert clientsService.searchByText is still
called only once to confirm no duplicate request. Use the existing
component.search, component.onSearchInput and clientsService.searchByText mocks
and ensure Jest fake timers are enabled for the test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0d18f9d-c218-48ac-9399-a8f9ffa92376
📒 Files selected for processing (3)
src/app/clients/clients.component.htmlsrc/app/clients/clients.component.spec.tssrc/app/clients/clients.component.ts
a05c3dc to
1654c9a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/clients/clients.component.spec.ts (1)
68-119: Extract the debounce duration into a shared test constant.Using a single constant avoids brittle drift if the debounce interval changes later.
As per coding guidelines,
**/*.spec.ts: "encourage clear Arrange-Act-Assert structure, stable selectors, and minimal brittle timing dependencies."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.spec.ts` around lines 68 - 119, Extract the hard-coded 500ms debounce into a shared test constant (e.g. const DEBOUNCE_MS = 500) at the top of the describe block and replace every literal 500 (and any related checks like 499 -> DEBOUNCE_MS - 1) in the specs that call component.onSearchInput, component.search, and component.ngOnDestroy so that jest.advanceTimersByTime and expectations against clientsService.searchByText use DEBOUNCE_MS; keep other timing values (e.g. 200) as-is or express them relative to the constant if clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/clients/clients.component.spec.ts`:
- Around line 68-119: Extract the hard-coded 500ms debounce into a shared test
constant (e.g. const DEBOUNCE_MS = 500) at the top of the describe block and
replace every literal 500 (and any related checks like 499 -> DEBOUNCE_MS - 1)
in the specs that call component.onSearchInput, component.search, and
component.ngOnDestroy so that jest.advanceTimersByTime and expectations against
clientsService.searchByText use DEBOUNCE_MS; keep other timing values (e.g. 200)
as-is or express them relative to the constant if clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6963df57-9a19-49b6-8314-4f7e69e4f9e3
📒 Files selected for processing (3)
src/app/clients/clients.component.htmlsrc/app/clients/clients.component.spec.tssrc/app/clients/clients.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/clients/clients.component.html
- src/app/clients/clients.component.ts
1654c9a to
fae10ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/clients/clients.component.html (1)
21-24: Set an explicit button type for future form safety.Add
type="button"to the search icon button to prevent accidental submits if this template is later nested inside a<form>.Suggested tweak
<button mat-icon-button + type="button" matSuffix (click)="search(searchInput.value)" [attr.aria-label]="'labels.buttons.Search' | translate" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.html` around lines 21 - 24, The search icon button currently has no explicit type which can cause accidental form submissions if this template is later placed inside a form; update the button element that invokes search(searchInput.value) (the button with mat-icon-button, matSuffix and (click)="search(searchInput.value)") to include type="button" so it will not act as a submit button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/clients/clients.component.ts`:
- Around line 123-127: The search subscription can trigger multiple overlapping
HTTP requests in getClients(), causing out-of-order responses to overwrite the
table; fix by storing the in-flight subscription (e.g., a class property like
clientsSub or clientsRequestSub) and, inside getClients(), unsubscribe the
existing subscription before creating a new one so only the latest HTTP
observable is active; update getClients() and any callers (search()) to use this
stored subscription and ensure cleanup on destroy (this.destroy$ or ngOnDestroy)
to avoid leaks.
---
Nitpick comments:
In `@src/app/clients/clients.component.html`:
- Around line 21-24: The search icon button currently has no explicit type which
can cause accidental form submissions if this template is later placed inside a
form; update the button element that invokes search(searchInput.value) (the
button with mat-icon-button, matSuffix and (click)="search(searchInput.value)")
to include type="button" so it will not act as a submit button.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f9964d0-3cb2-48c5-9b8d-65523560a366
📒 Files selected for processing (3)
src/app/clients/clients.component.htmlsrc/app/clients/clients.component.spec.tssrc/app/clients/clients.component.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/clients/clients.component.spec.ts
fae10ee to
1452094
Compare
IOhacker
left a comment
There was a problem hiding this comment.
Explained why there are fixed values?
| }); | ||
|
|
||
| it('should ignore duplicate consecutive values', () => { | ||
| component.onSearchInput('alice'); |
There was a problem hiding this comment.
Which fixed values? Are you asking about the hardcoded strings used as test search input?
|
yes, becuase they are values for English names, what about names that uses another characters? |
1452094 to
9788c5e
Compare
|
@IOhacker I have updated the PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/clients/clients.component.spec.ts (1)
51-55: Consider loading only required icons for faster test execution.Loading all FontAwesome solid icons is heavyweight. If test performance becomes a concern, consider importing only the specific icons used by
ClientsComponent.♻️ Suggested optimization
- const iconList = Object.keys(solidIcons) - .filter((key) => key !== 'fas' && key !== 'prefix' && key.startsWith('fa')) - .map((icon) => (solidIcons as any)[icon]); - faIconLibrary.addIcons(...iconList); + // Only add icons actually used by the component + faIconLibrary.addIcons(solidIcons.faSearch, solidIcons.faUsers /* add other required icons */);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.spec.ts` around lines 51 - 55, Tests currently add the entire FontAwesome solid icon set via FaIconLibrary/addIcons using solidIcons and iconList which is heavy; update the test to import and register only the specific icons that ClientsComponent actually uses (reference FaIconLibrary, solidIcons, iconList, addIcons and ClientsComponent) by replacing the broad filter with an explicit list of the required icon symbols (or direct imports of those icons) and call faIconLibrary.addIcons(...) with just those selected icons to reduce test startup time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/clients/clients.component.spec.ts`:
- Around line 51-55: Tests currently add the entire FontAwesome solid icon set
via FaIconLibrary/addIcons using solidIcons and iconList which is heavy; update
the test to import and register only the specific icons that ClientsComponent
actually uses (reference FaIconLibrary, solidIcons, iconList, addIcons and
ClientsComponent) by replacing the broad filter with an explicit list of the
required icon symbols (or direct imports of those icons) and call
faIconLibrary.addIcons(...) with just those selected icons to reduce test
startup time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f46417d9-63b9-4227-915c-a7a3fc0ad581
📒 Files selected for processing (3)
src/app/clients/clients.component.htmlsrc/app/clients/clients.component.spec.tssrc/app/clients/clients.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/clients/clients.component.html
- src/app/clients/clients.component.ts
9788c5e to
e052b42
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/clients/clients.component.spec.ts (2)
26-26: Avoid re-declaring the debounce interval in the test.The component hardcodes the same
500ms value in itsdebounceTime()pipe, so this constant can drift from production behavior while the suite still passes. Export one shared constant and reuse it here.As per coding guidelines: "For tests: encourage clear Arrange-Act-Assert structure, stable selectors, and minimal brittle timing dependencies".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.spec.ts` at line 26, The test declares a local DEBOUNCE_MS (500) that duplicates the value used in the component's debounceTime, risking drift; instead export the canonical debounce interval from the component (or a shared constants file) and import it into clients.component.spec.ts, then remove the local constant and use the imported constant wherever DEBOUNCE_MS is referenced so tests always use the production value (refer to the component's debounceTime usage and the DEBOUNCE_MS symbol to locate the change).
112-117: The non-ASCII case still skips IME/composition input.Calling
onSearchInput('مريم')only proves Unicode passthrough at the method level. The new regression surface is the raw(input)binding, so please verify a composition-based flow (for example Japanese/Chinese/Korean input) in the browser or with an interaction-level test before treating the non-English-name concern as closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.spec.ts` around lines 112 - 117, The current test calls component.onSearchInput('مريم') which bypasses IME composition behavior; add an interaction-level unit test that simulates composition events on the input binding (trigger compositionstart, update input value to partial composition, ensure clientsService.searchByText is NOT called during composition, then trigger compositionend with the final text and advance timers by DEBOUNCE_MS to assert clientsService.searchByText was called once with the final non-ASCII string). Target the component's template input element (the element that wires to onSearchInput) and use the element.dispatchEvent/triggerEventHandler sequence for compositionstart/compositionend to validate the real browser-like flow rather than calling onSearchInput directly.src/app/clients/clients.component.html (1)
17-19: Bind through#searchInputinstead of$event.target.
$event.targetis not a value-bearing type under Angular template checking, so this is the first place strict templates will complain. Since#searchInputalready exists, use it for both handlers and keep the template type-safe.Suggested fix
- (input)="onSearchInput($event.target.value)" - (keydown.enter)="search($event.target.value)" + (input)="onSearchInput(searchInput.value)" + (keydown.enter)="search(searchInput.value)"As per coding guidelines: "For Angular code: verify component separation, trackBy on *ngFor, strict type safety, and clean observable patterns".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/clients/clients.component.html` around lines 17 - 19, The template is using $event.target.value which fails Angular strict template type checking; change both handlers to use the existing template reference `#searchInput` instead (pass searchInput.value into onSearchInput and search) so the calls become onSearchInput(searchInput.value) and (keydown.enter)="search(searchInput.value)"; ensure the input element defines `#searchInput` and that the component methods onSearchInput and search accept a string parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/clients/clients.component.ts`:
- Around line 123-128: The debounced search triggers two HTTP calls because
search() always calls getClients() after resetPaginator() which itself may call
MatPaginator.firstPage() and fire pageChanged(); update search() to only call
getClients() when the paginator is already on page 0: check
this.paginator?.pageIndex (or MatPaginator instance used) and if pageIndex !== 0
call resetPaginator() (which will cause pageChanged() to fetch) and return;
otherwise proceed to call getClients() directly. Ensure references to search(),
resetPaginator(), pageChanged(), getClients(), and this.paginator are used so
the change is applied to the correct methods.
---
Nitpick comments:
In `@src/app/clients/clients.component.html`:
- Around line 17-19: The template is using $event.target.value which fails
Angular strict template type checking; change both handlers to use the existing
template reference `#searchInput` instead (pass searchInput.value into
onSearchInput and search) so the calls become onSearchInput(searchInput.value)
and (keydown.enter)="search(searchInput.value)"; ensure the input element
defines `#searchInput` and that the component methods onSearchInput and search
accept a string parameter.
In `@src/app/clients/clients.component.spec.ts`:
- Line 26: The test declares a local DEBOUNCE_MS (500) that duplicates the value
used in the component's debounceTime, risking drift; instead export the
canonical debounce interval from the component (or a shared constants file) and
import it into clients.component.spec.ts, then remove the local constant and use
the imported constant wherever DEBOUNCE_MS is referenced so tests always use the
production value (refer to the component's debounceTime usage and the
DEBOUNCE_MS symbol to locate the change).
- Around line 112-117: The current test calls component.onSearchInput('مريم')
which bypasses IME composition behavior; add an interaction-level unit test that
simulates composition events on the input binding (trigger compositionstart,
update input value to partial composition, ensure clientsService.searchByText is
NOT called during composition, then trigger compositionend with the final text
and advance timers by DEBOUNCE_MS to assert clientsService.searchByText was
called once with the final non-ASCII string). Target the component's template
input element (the element that wires to onSearchInput) and use the
element.dispatchEvent/triggerEventHandler sequence for
compositionstart/compositionend to validate the real browser-like flow rather
than calling onSearchInput directly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36316701-ddb1-4fc3-8002-0ba9c041b1cb
📒 Files selected for processing (3)
src/app/clients/clients.component.htmlsrc/app/clients/clients.component.spec.tssrc/app/clients/clients.component.ts
e052b42 to
99b2d4d
Compare
|
@gkbishnoi07 I have resolved the issues reported by coderabbit |
Description
The client list search previously only triggered on Enter key press, requiring explicit user action for every lookup. This change adds debounced dynamic search so that results update automatically as the user types, with a 500 ms idle delay to avoid flooding the server with requests on every keystroke.
Changes:
debounceTime(500))distinctUntilChanged())matSuffix) is added inside the search field for click-to-searchdestroy$subject on component destroyNo new dependencies required.
Related issues and discussion
#896
Screenshots, if any
Before/after screenshots to be added — see contributing guide requirement for UI changes.
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Tests