Fix: Enable search history by default for Search Box (#2566)

* make enable search history true by default

* fix: debounceTime being shadowed (linting)

* check for normal debounce time input

* remove redundant comment

* minimum debounce value for search history

* remove search history debounce input

* incorporate debounce time in test

* added test for higher debounce time

* search history default enabled test

Author: aneeshsharma

projects/components/src/search-box/search-box.component.test.ts

@@ -119,6 +119,21 @@ describe('Search box Component', () => {
     });
   }));
 
+  test('search history should be enabled by default', fakeAsync(() => {
+    spectator = createHost(
+      `<ht-search-box [placeholder]="placeholder" [debounceTime]="debounceTime" [searchMode]="searchMode"></ht-search-box>`,
+      {
+        hostProps: {
+          placeholder: 'Test Placeholder',
+          debounceTime: 200,
+          searchMode: SearchBoxEmitMode.Incremental,
+        },
+      },
+    );
+
+    expect(spectator.component.enableSearchHistory).toBe(true);
+  }));
+
   test('enabled search history should work as expected', fakeAsync(() => {
     spectator = createHost(
       `<ht-search-box [placeholder]="placeholder" [debounceTime]="debounceTime" [searchMode]="searchMode" [enableSearchHistory]="enableSearchHistory"></ht-search-box>`,
@@ -144,16 +159,61 @@ describe('Search box Component', () => {
     spectator.setInput({ value: 'test-value' });
     spectator.triggerEventHandler('input', 'input', 'test-value');
     expect(searchBoxELement).toHaveClass('has-value');
-    spectator.tick(500);
+
+    spectator.tick(1500);
+
+    expect(spectator.inject(PopoverService).drawPopover).not.toHaveBeenCalled();
 
     inputElement.blur();
     spectator.click('.icon.close');
-    spectator.tick(500);
+    spectator.tick(1500);
     expect(spectator.inject(PopoverService).drawPopover).toHaveBeenCalled();
 
     flush();
   }));
 
+  test('enabled search history should work as expected for high debounce time', fakeAsync(() => {
+    const debounceTime = 3000;
+    spectator = createHost(
+      `<ht-search-box [placeholder]="placeholder" [debounceTime]="debounceTime" [searchMode]="searchMode" [enableSearchHistory]="enableSearchHistory"></ht-search-box>`,
+      {
+        hostProps: {
+          placeholder: 'Test Placeholder',
+          debounceTime: debounceTime,
+          searchMode: SearchBoxEmitMode.Incremental,
+          enableSearchHistory: true,
+        },
+      },
+    );
+
+    const searchBoxELement = spectator.query('.ht-search-box')!;
+    expect(searchBoxELement).not.toHaveClass('has-value');
+    expect(searchBoxELement).not.toHaveClass('focused');
+
+    spectator.click('.icon.search');
+    expect(searchBoxELement).toHaveClass('focused');
+    expect(spectator.inject(PopoverService).drawPopover).toHaveBeenCalledTimes(1);
+
+    const inputElement = spectator.query('input') as HTMLInputElement;
+    spectator.setInput({ value: 'test-value' });
+    spectator.triggerEventHandler('input', 'input', 'test-value');
+    expect(searchBoxELement).toHaveClass('has-value');
+
+    // popover not called before debounce is over
+    expect(spectator.inject(PopoverService).drawPopover).toHaveBeenCalledTimes(1);
+
+    spectator.tick(debounceTime + 100);
+
+    expect(spectator.inject(PopoverService).drawPopover).toHaveBeenCalledTimes(1);
+
+    inputElement.blur();
+    spectator.click('.icon.close');
+    spectator.tick(debounceTime + 100);
+    expect(spectator.inject(PopoverService).drawPopover).toHaveBeenCalledTimes(2);
+
+    flush();
+  }));
+
   test('collapsable enabled should add collapsable class', () => {
     spectator = createHost(
       `<ht-search-box [placeholder]="placeholder" [debounceTime]="debounceTime" [searchMode]="searchMode" [enableSearchHistory]="enableSearchHistory" [collapsable]="collapsable"></ht-search-box>`,

projects/components/src/search-box/search-box.component.ts

@@ -21,7 +21,7 @@ import {
   TypedSimpleChanges,
 } from '@hypertrace/common';
 import { combineLatest, Observable, Subject, timer } from 'rxjs';
-import { debounce, map } from 'rxjs/operators';
+import { debounce, debounceTime, map } from 'rxjs/operators';
 import { IconSize } from '../icon/icon-size';
 import { PopoverService } from '../popover/popover.service';
 import { PopoverRef } from '../popover/popover-ref';
@@ -111,7 +111,7 @@ export class SearchBoxComponent implements OnInit, OnChanges {
   public searchMode: SearchBoxEmitMode = SearchBoxEmitMode.Incremental;
 
   @Input()
-  public enableSearchHistory: boolean = false; // Experimental
+  public enableSearchHistory: boolean = true;
 
   @Input()
   public collapsable: boolean = false;
@@ -137,6 +137,8 @@ export class SearchBoxComponent implements OnInit, OnChanges {
 
   public popover?: PopoverRef;
 
+  public defaultSearchHistoryDebounceTime = 1000;
+
   public constructor(
     private readonly cdr: ChangeDetectorRef,
     private readonly host: ElementRef,
@@ -222,7 +224,7 @@ export class SearchBoxComponent implements OnInit, OnChanges {
     this.subscriptionLifecycle.unsubscribe();
     this.subscriptionLifecycle.add(
       combineLatest([this.debouncedValueSubject, this.getDebounceTime()])
-        .pipe(debounce(([_, debounceTime]) => timer(debounceTime)))
+        .pipe(debounce(([_, valueDebounceTime]) => timer(valueDebounceTime)))
         .subscribe(([value, _]) => this.valueChange.emit(value)),
     );
   }
@@ -260,12 +262,27 @@ export class SearchBoxComponent implements OnInit, OnChanges {
       }),
     );
 
+    // Use the default search history debounce time if the value debounce time is less than the default
+    const searchHistoryDebounce =
+      this.debounceTime && this.debounceTime > this.defaultSearchHistoryDebounceTime
+        ? 0
+        : this.defaultSearchHistoryDebounceTime;
+
     this.subscriptionLifecycle.add(
-      this.valueChange.asObservable().subscribe(emittedValue => {
-        if (!isEmpty(emittedValue)) {
-          this.lastEmittedValues = [emittedValue, ...this.lastEmittedValues];
-        }
-      }),
+      searchHistoryDebounce > 0
+        ? this.valueChange
+            .asObservable()
+            .pipe(debounceTime(searchHistoryDebounce))
+            .subscribe(emittedValue => {
+              if (!isEmpty(emittedValue)) {
+                this.lastEmittedValues = [emittedValue, ...this.lastEmittedValues];
+              }
+            })
+        : this.valueChange.asObservable().subscribe(emittedValue => {
+            if (!isEmpty(emittedValue)) {
+              this.lastEmittedValues = [emittedValue, ...this.lastEmittedValues];
+            }
+          }),
     );
   }
 
@@ -296,7 +313,7 @@ export class SearchBoxComponent implements OnInit, OnChanges {
   }
 
   private handleSearchHistoryOnInputBlur(): void {
-    this.searchHistory = [...uniq(this.lastEmittedValues), ...this.searchHistory];
+    this.searchHistory = uniq([...this.lastEmittedValues, ...this.searchHistory]);
     this.filteredSearchHistory = [...this.searchHistory];
     this.lastEmittedValues = [];
   }