Skip to content

WEB-896: Add debounced dynamic search to client list#3445

Open
Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Agaba-Ed:add-debounced-client-search
Open

WEB-896: Add debounced dynamic search to client list#3445
Agaba-Ed wants to merge 1 commit intoopenMF:devfrom
Agaba-Ed:add-debounced-client-search

Conversation

@Agaba-Ed
Copy link
Copy Markdown
Contributor

@Agaba-Ed Agaba-Ed commented Mar 26, 2026

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:

  • Search fires 500 ms after the user stops typing (debounceTime(500))
  • Rapid typing resets the timer — only the final value triggers a request (distinctUntilChanged())
  • Pressing Enter still triggers an immediate search (existing behaviour preserved)
  • A search icon button (matSuffix) is added inside the search field for click-to-search
  • RxJS subscriptions are cleaned up via destroy$ subject on component destroy

No 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

    • Debounced typing search for smoother, responsive results
    • Clickable search icon plus Enter-key support for immediate searches
    • ARIA-labeled search control for improved accessibility
    • Lifecycle-safe teardown to prevent pending searches after leaving the view
  • Tests

    • Added tests for debounce behavior, immediate search, duplicate-input suppression, non-ASCII input, and cleanup on teardown

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 26, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'pre_merge_checks'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Adds debounced live-search: template emits input events; component buffers them in a Subject with debounceTime + distinctUntilChanged, cancels prior in-flight requests, supports immediate Enter/button searches, and cleans up subscriptions on destroy. Tests validate debounce, suppression, immediate execution, non-ASCII handling, and teardown.

Changes

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
Loading

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Duplicate 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:

  1. search() is called directly (immediate)
  2. The debounced pipeline also calls search() after 500ms with the same value

distinctUntilChanged() only deduplicates consecutive emissions to searchInput$, not invocations of search().

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 switchMap pattern 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

📥 Commits

Reviewing files that changed from the base of the PR and between f82ad09 and a05c3dc.

📒 Files selected for processing (3)
  • src/app/clients/clients.component.html
  • src/app/clients/clients.component.spec.ts
  • src/app/clients/clients.component.ts

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from a05c3dc to 1654c9a Compare March 26, 2026 22:07
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between a05c3dc and 1654c9a.

📒 Files selected for processing (3)
  • src/app/clients/clients.component.html
  • src/app/clients/clients.component.spec.ts
  • src/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

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from 1654c9a to fae10ee Compare March 27, 2026 07:34
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1654c9a and fae10ee.

📒 Files selected for processing (3)
  • src/app/clients/clients.component.html
  • src/app/clients/clients.component.spec.ts
  • src/app/clients/clients.component.ts
✅ Files skipped from review due to trivial changes (1)
  • src/app/clients/clients.component.spec.ts

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from fae10ee to 1452094 Compare March 27, 2026 08:12
Copy link
Copy Markdown
Contributor

@IOhacker IOhacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explained why there are fixed values?

});

it('should ignore duplicate consecutive values', () => {
component.onSearchInput('alice');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why fixed values here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which fixed values? Are you asking about the hardcoded strings used as test search input?

@IOhacker
Copy link
Copy Markdown
Contributor

yes, becuase they are values for English names, what about names that uses another characters?

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from 1452094 to 9788c5e Compare March 28, 2026 07:40
@Agaba-Ed Agaba-Ed requested a review from IOhacker March 28, 2026 07:41
@Agaba-Ed
Copy link
Copy Markdown
Contributor Author

@IOhacker I have updated the PR.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1452094 and 9788c5e.

📒 Files selected for processing (3)
  • src/app/clients/clients.component.html
  • src/app/clients/clients.component.spec.ts
  • src/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

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from 9788c5e to e052b42 Compare March 28, 2026 08:09
@gkbishnoi07
Copy link
Copy Markdown
Collaborator

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 1, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 500 ms value in its debounceTime() 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 #searchInput instead of $event.target.

$event.target is not a value-bearing type under Angular template checking, so this is the first place strict templates will complain. Since #searchInput already 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

📥 Commits

Reviewing files that changed from the base of the PR and between b6d6d80 and e052b42.

📒 Files selected for processing (3)
  • src/app/clients/clients.component.html
  • src/app/clients/clients.component.spec.ts
  • src/app/clients/clients.component.ts

@Agaba-Ed Agaba-Ed force-pushed the add-debounced-client-search branch from e052b42 to 99b2d4d Compare April 2, 2026 12:17
@Agaba-Ed
Copy link
Copy Markdown
Contributor Author

Agaba-Ed commented Apr 2, 2026

@gkbishnoi07 I have resolved the issues reported by coderabbit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants