mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-11 19:46:16 -08:00
fix(search): keep history authoritative and retire the debounce on Enter
Two follow-ups from review of the same-page navigation guard. Greptile: the guard could not tell an app-initiated `q` echo from back/forward landing on a history entry that carries the same term. Key the exemption on `Navigation.trigger === 'imperative'` instead, so browser history always wins over in-flight typing. `lastSuccessfulNavigation` is set immediately before `NavigationEnd` is emitted, so it describes the navigation being handled. Codex: with the guard in place, an Enter commit no longer had its queued debounce cancelled as a side effect of the resulting `NavigationEnd`. Typing "Beta " and pressing Enter before the debounce expired applied the trimmed term, then the stale timeout reapplied the untrimmed one — leaving the box and URL on "Beta" while the provider store searched "Beta ". `applySearchQuery()` now cancels the pending debounce itself, which is the correct owner of that rule rather than relying on a navigation side effect. Both new tests were mutation-checked: dropping either sub-fix fails exactly its own test and no other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
7dba817244
commit
c20b9e7fdf
3 files changed
+107
-33
No files matched your search
@@ -164,13 +164,20 @@ Search is shell-owned and route-aware:
|
||||
matches the rest of the workspace.
|
||||
8. The URL is authoritative for the search box only when it carries search
|
||||
intent. `WorkspaceShellSearchSyncService` re-reads `q` on every
|
||||
`NavigationEnd`, but a navigation that stays on the same page and carries
|
||||
the term already applied is ignored while input is still debouncing —
|
||||
otherwise a page writing an unrelated query param (a downloads filter chip,
|
||||
a refresh bump) or the router echoing back our own `q` would cancel the
|
||||
pending debounce and reset the box, eating everything typed since. Pages are
|
||||
free to write their own query params while the user types; they must not
|
||||
assume the shell will re-apply the search afterwards.
|
||||
`NavigationEnd`, but an **app-initiated** navigation that stays on the same
|
||||
page and carries the term already applied is ignored while input is still
|
||||
debouncing — otherwise a page writing an unrelated query param (a downloads
|
||||
filter chip, a refresh bump) or the router echoing back our own `q` would
|
||||
cancel the pending debounce and reset the box, eating everything typed
|
||||
since. Pages are free to write their own query params while the user types;
|
||||
they must not assume the shell will re-apply the search afterwards.
|
||||
9. Browser history overrides that guard. The exemption is keyed on
|
||||
`Navigation.trigger === 'imperative'`, so back/forward always re-applies
|
||||
what the history entry carries, even mid-typing.
|
||||
10. Applying a term explicitly supersedes a queued one. `applySearchQuery()`
|
||||
cancels any pending debounce, so the Enter key committing a trimmed term
|
||||
cannot be overwritten a moment later by the untrimmed keystroke still
|
||||
waiting behind it.
|
||||
|
||||
Rail navigation is also shell-owned:
|
||||
|
||||
|
||||
+63
-3
@@ -11,6 +11,7 @@ import { RuntimeCapabilitiesService } from '@iptvnator/services';
|
||||
import { WorkspaceStartupPreferencesService } from '@iptvnator/workspace/shell/util';
|
||||
import { SEARCH_INPUT_DEBOUNCE_MS } from './helpers/workspace-shell-constants';
|
||||
import { WorkspaceShellRouteStateService } from './workspace-shell-route-state.service';
|
||||
import { WorkspaceShellSearchService } from './workspace-shell-search.service';
|
||||
import { WorkspaceShellSearchSyncService } from './workspace-shell-search-sync.service';
|
||||
|
||||
const DOWNLOADS_URL = '/workspace/downloads';
|
||||
@@ -19,28 +20,41 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
let service: WorkspaceShellSearchSyncService;
|
||||
let routeState: WorkspaceShellRouteStateService;
|
||||
let events: Subject<NavigationEnd>;
|
||||
let searchService: WorkspaceShellSearchService;
|
||||
let trigger: 'imperative' | 'popstate';
|
||||
let router: {
|
||||
url: string;
|
||||
events: Subject<NavigationEnd>;
|
||||
navigate: jest.Mock;
|
||||
navigateByUrl: jest.Mock;
|
||||
parseUrl: jest.Mock;
|
||||
lastSuccessfulNavigation: () => { trigger: string };
|
||||
};
|
||||
|
||||
/** Emits the NavigationEnd both shell services listen for. */
|
||||
function navigateTo(url: string): void {
|
||||
/**
|
||||
* Emits the NavigationEnd both shell services listen for. `origin` mirrors
|
||||
* `Navigation.trigger`: 'imperative' is the app navigating, 'popstate' is
|
||||
* the user moving through browser history.
|
||||
*/
|
||||
function navigateTo(
|
||||
url: string,
|
||||
origin: 'imperative' | 'popstate' = 'imperative'
|
||||
): void {
|
||||
router.url = url;
|
||||
trigger = origin;
|
||||
events.next(new NavigationEnd(1, url, url));
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
jest.useFakeTimers();
|
||||
events = new Subject<NavigationEnd>();
|
||||
trigger = 'imperative';
|
||||
router = {
|
||||
url: DOWNLOADS_URL,
|
||||
events,
|
||||
navigate: jest.fn().mockResolvedValue(true),
|
||||
navigateByUrl: jest.fn().mockResolvedValue(true),
|
||||
lastSuccessfulNavigation: () => ({ trigger }),
|
||||
parseUrl: jest.fn((url: string) => {
|
||||
const parsed = new URL(url, 'http://localhost');
|
||||
const queryParams: Record<string, string> = {};
|
||||
@@ -55,6 +69,7 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
providers: [
|
||||
WorkspaceShellRouteStateService,
|
||||
WorkspaceShellSearchSyncService,
|
||||
WorkspaceShellSearchService,
|
||||
{ provide: Router, useValue: router },
|
||||
{
|
||||
provide: Store,
|
||||
@@ -95,17 +110,23 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
useValue: {
|
||||
setSearchTerm: jest.fn(),
|
||||
setCategorySearchTerm: jest.fn(),
|
||||
getSelectedCategory: signal(null),
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: StalkerStore,
|
||||
useValue: { setSearchPhrase: jest.fn() },
|
||||
useValue: {
|
||||
setSearchPhrase: jest.fn(),
|
||||
getSelectedCategoryName: signal(''),
|
||||
itvFullListActive: signal(false),
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
routeState = TestBed.inject(WorkspaceShellRouteStateService);
|
||||
service = TestBed.inject(WorkspaceShellSearchSyncService);
|
||||
searchService = TestBed.inject(WorkspaceShellSearchService);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
@@ -125,6 +146,13 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('Beta Movie');
|
||||
expect(service.searchQuery()).toBe('Beta Movie');
|
||||
|
||||
// The term has to reach the URL — that is what the page reads back.
|
||||
TestBed.flushEffects();
|
||||
expect(router.navigateByUrl).toHaveBeenCalledWith(
|
||||
`${DOWNLOADS_URL}?q=Beta+Movie`,
|
||||
{ replaceUrl: true }
|
||||
);
|
||||
});
|
||||
|
||||
it('does not roll typing back to the term its own q navigation echoes', () => {
|
||||
@@ -168,6 +196,38 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
expect(service.appliedSearchQuery()).toBe('Alpha');
|
||||
});
|
||||
|
||||
it('lets browser history win over in-flight typing', () => {
|
||||
// Back/forward is authoritative even when the entry restores the term
|
||||
// already applied — only app-initiated navigations are treated as
|
||||
// echoes worth ignoring.
|
||||
service.onSearchInput('Beta');
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
service.onSearchInput('Beta Movie');
|
||||
|
||||
navigateTo(`${DOWNLOADS_URL}?q=Beta`, 'popstate');
|
||||
|
||||
expect(service.searchQuery()).toBe('Beta');
|
||||
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('Beta');
|
||||
});
|
||||
|
||||
it('does not let a pending keystroke reapply behind an Enter commit', () => {
|
||||
// Enter commits the trimmed term immediately; the debounce still
|
||||
// holding the untrimmed keystroke must not overwrite it afterwards.
|
||||
service.onSearchInput('Beta Movie ');
|
||||
searchService.onSearchEnter('Beta Movie ');
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('Beta Movie');
|
||||
|
||||
navigateTo(`${DOWNLOADS_URL}?q=Beta+Movie`);
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('Beta Movie');
|
||||
expect(service.searchQuery()).toBe('Beta Movie');
|
||||
});
|
||||
|
||||
it('syncs the search box from the url when nothing is being typed', () => {
|
||||
navigateTo(`${DOWNLOADS_URL}?q=Gamma`);
|
||||
|
||||
|
||||
+30
-23
@@ -29,12 +29,7 @@ export class WorkspaceShellSearchSyncService {
|
||||
readonly appliedSearchQuery = signal('');
|
||||
|
||||
constructor() {
|
||||
this.destroyRef.onDestroy(() => {
|
||||
if (this.searchDebounceTimeoutId !== null) {
|
||||
clearTimeout(this.searchDebounceTimeoutId);
|
||||
this.searchDebounceTimeoutId = null;
|
||||
}
|
||||
});
|
||||
this.destroyRef.onDestroy(() => this.cancelPendingSearchApply());
|
||||
|
||||
this.router.events
|
||||
.pipe(
|
||||
@@ -115,16 +110,18 @@ export class WorkspaceShellSearchSyncService {
|
||||
}
|
||||
|
||||
setSearchState(value: string): void {
|
||||
if (this.searchDebounceTimeoutId !== null) {
|
||||
clearTimeout(this.searchDebounceTimeoutId);
|
||||
this.searchDebounceTimeoutId = null;
|
||||
}
|
||||
|
||||
this.cancelPendingSearchApply();
|
||||
this.searchQuery.set(value);
|
||||
this.appliedSearchQuery.set(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Applies a term now. This supersedes a queued debounce — pressing Enter
|
||||
* commits the trimmed term, and the keystroke that is still waiting must
|
||||
* not reapply the untrimmed one behind it.
|
||||
*/
|
||||
applySearchQuery(value: string): void {
|
||||
this.cancelPendingSearchApply();
|
||||
this.appliedSearchQuery.set(value);
|
||||
}
|
||||
|
||||
@@ -136,18 +133,24 @@ export class WorkspaceShellSearchSyncService {
|
||||
? getRouteQueryParam(this.router, url, 'q')
|
||||
: '';
|
||||
|
||||
// A navigation that stays on the same page and carries the term we
|
||||
// already applied brings no search intent of its own: it is either an
|
||||
// unrelated query param the page wrote (a filter chip, a refresh bump)
|
||||
// or the router echoing back our own `q`. Syncing anyway would cancel
|
||||
// the pending debounce and reset the box to the applied term, silently
|
||||
// eating everything typed since — including the whole word, when the
|
||||
// first keystroke has not been applied yet.
|
||||
// An app-initiated navigation that stays on the same page and carries
|
||||
// the term we already applied brings no search intent of its own: it is
|
||||
// either an unrelated query param the page wrote (a filter chip, a
|
||||
// refresh bump) or the router echoing back our own `q`. Syncing anyway
|
||||
// would cancel the pending debounce and reset the box to the applied
|
||||
// term, silently eating everything typed since — including the whole
|
||||
// word, when the first keystroke has not been applied yet.
|
||||
//
|
||||
// The trigger check keeps that narrow: history is always authoritative,
|
||||
// so back/forward re-applies what the entry carries even mid-typing.
|
||||
// `lastSuccessfulNavigation` is set immediately before `NavigationEnd`
|
||||
// is emitted, so it describes the navigation being handled here.
|
||||
if (
|
||||
this.searchDebounceTimeoutId !== null &&
|
||||
previousUrl !== null &&
|
||||
getRoutePath(url) === getRoutePath(previousUrl) &&
|
||||
nextTerm === this.appliedSearchQuery()
|
||||
nextTerm === this.appliedSearchQuery() &&
|
||||
this.router.lastSuccessfulNavigation()?.trigger === 'imperative'
|
||||
) {
|
||||
return;
|
||||
}
|
||||
@@ -156,13 +159,17 @@ export class WorkspaceShellSearchSyncService {
|
||||
}
|
||||
|
||||
private scheduleSearchApply(value: string): void {
|
||||
if (this.searchDebounceTimeoutId !== null) {
|
||||
clearTimeout(this.searchDebounceTimeoutId);
|
||||
}
|
||||
|
||||
this.cancelPendingSearchApply();
|
||||
this.searchDebounceTimeoutId = setTimeout(() => {
|
||||
this.searchDebounceTimeoutId = null;
|
||||
this.applySearchQuery(value);
|
||||
}, SEARCH_INPUT_DEBOUNCE_MS);
|
||||
}
|
||||
|
||||
private cancelPendingSearchApply(): void {
|
||||
if (this.searchDebounceTimeoutId !== null) {
|
||||
clearTimeout(this.searchDebounceTimeoutId);
|
||||
this.searchDebounceTimeoutId = null;
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user