mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-08 17:06:15 -08:00
fix(search): keep in-flight typing when the page rewrites its query params (#1432)
* fix(search): keep in-flight typing when the page rewrites its query params `WorkspaceShellSearchSyncService` re-read `q` on every `NavigationEnd` and unconditionally called `setSearchState(...)`, which cancels the pending debounce and overwrites the search box. Any same-page navigation that carried no search intent — a downloads filter chip writing `?filter=…`, a refresh bump, or the router echoing back our own `q` — therefore ate whatever had been typed since the last applied term. While input is debouncing, ignore a navigation that stays on the same page and carries the term already applied. Real search intent (route change, back/forward, a different `q`) still syncs as before. This is the race behind the flaky `@downloads @electron keeps global and scoped libraries truthful …` e2e test: it clicks a filter chip and fills the searchbox with nothing awaited in between, so under CI load the chip's navigation lands after the keystroke, wipes the term, and `q` is never written. Reproduced deterministically by dispatching the chip click and the `input` event in the same page task; the searchbox value goes to `""` and `q` stays `null`. The new spec fails on the old code for the two regression cases and passes for the three guard cases on both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
0cba49f3e2
commit
30d76a3a46
5 files changed
+323
-20
No files matched your search
+10
-2
@@ -29,6 +29,14 @@ export function getRouteQueryParam(
|
||||
return typeof value === 'string' ? value : '';
|
||||
}
|
||||
|
||||
/**
|
||||
* The page part of a router URL, without query params or fragment — i.e. the
|
||||
* identity of "which page am I on", as opposed to how it is parameterised.
|
||||
*/
|
||||
export function getRoutePath(url: string): string {
|
||||
return url.split('?')[0].split('#')[0];
|
||||
}
|
||||
|
||||
export function syncSearchQueryParam(
|
||||
router: Router,
|
||||
currentUrl: string,
|
||||
@@ -40,7 +48,7 @@ export function syncSearchQueryParam(
|
||||
return false;
|
||||
}
|
||||
|
||||
const routePath = currentUrl.split('?')[0];
|
||||
const routePath = getRoutePath(currentUrl);
|
||||
const queryParams = {
|
||||
...router.parseUrl(currentUrl).queryParams,
|
||||
};
|
||||
@@ -58,7 +66,7 @@ export function syncSearchQueryParam(
|
||||
}
|
||||
|
||||
export function bumpRefreshQueryParam(router: Router, currentUrl: string): void {
|
||||
const routePath = currentUrl.split('?')[0];
|
||||
const routePath = getRoutePath(currentUrl);
|
||||
const queryParams = {
|
||||
...router.parseUrl(currentUrl).queryParams,
|
||||
refresh: Date.now().toString(),
|
||||
|
||||
+243
@@ -0,0 +1,243 @@
|
||||
import { signal } from '@angular/core';
|
||||
import { TestBed } from '@angular/core/testing';
|
||||
import { NavigationEnd, Router } from '@angular/router';
|
||||
import { Store } from '@ngrx/store';
|
||||
import { TranslateService } from '@ngx-translate/core';
|
||||
import { Subject, of } from 'rxjs';
|
||||
import { PlaylistContextFacade } from '@iptvnator/playlist/shared/util';
|
||||
import { StalkerStore } from '@iptvnator/portal/stalker/data-access';
|
||||
import { XtreamStore } from '@iptvnator/portal/xtream/data-access';
|
||||
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';
|
||||
|
||||
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. `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> = {};
|
||||
parsed.searchParams.forEach((value, key) => {
|
||||
queryParams[key] = value;
|
||||
});
|
||||
return { queryParams };
|
||||
}),
|
||||
};
|
||||
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
WorkspaceShellRouteStateService,
|
||||
WorkspaceShellSearchSyncService,
|
||||
WorkspaceShellSearchService,
|
||||
{ provide: Router, useValue: router },
|
||||
{
|
||||
provide: Store,
|
||||
useValue: {
|
||||
selectSignal: jest.fn().mockReturnValue(signal([])),
|
||||
dispatch: jest.fn(),
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: PlaylistContextFacade,
|
||||
useValue: { activePlaylist: signal(null) },
|
||||
},
|
||||
{
|
||||
provide: WorkspaceStartupPreferencesService,
|
||||
useValue: {
|
||||
getFirstAvailableWorkspacePath: jest.fn(() => '/'),
|
||||
persistLastRestorablePath: jest.fn(),
|
||||
showDashboard: jest.fn(() => true),
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: TranslateService,
|
||||
useValue: {
|
||||
instant: jest.fn((key: string) => key),
|
||||
onLangChange: of(null),
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: RuntimeCapabilitiesService,
|
||||
useValue: {
|
||||
isElectron: true,
|
||||
isMacOS: true,
|
||||
supportsDownloads: true,
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: XtreamStore,
|
||||
useValue: {
|
||||
setSearchTerm: jest.fn(),
|
||||
setCategorySearchTerm: jest.fn(),
|
||||
getSelectedCategory: signal(null),
|
||||
},
|
||||
},
|
||||
{
|
||||
provide: StalkerStore,
|
||||
useValue: {
|
||||
setSearchPhrase: jest.fn(),
|
||||
getSelectedCategoryName: signal(''),
|
||||
itvFullListActive: signal(false),
|
||||
},
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
routeState = TestBed.inject(WorkspaceShellRouteStateService);
|
||||
service = TestBed.inject(WorkspaceShellSearchSyncService);
|
||||
searchService = TestBed.inject(WorkspaceShellSearchService);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.useRealTimers();
|
||||
});
|
||||
|
||||
it('keeps in-flight typing when the page writes an unrelated query param', () => {
|
||||
// The downloads filter chips write `?filter=…` with replaceUrl. Under
|
||||
// load that navigation can land after the first keystroke — it must
|
||||
// not eat the search term the user is still typing.
|
||||
service.onSearchInput('Beta Movie');
|
||||
navigateTo(DOWNLOADS_URL);
|
||||
|
||||
expect(service.searchQuery()).toBe('Beta Movie');
|
||||
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
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', () => {
|
||||
service.onSearchInput('Beta');
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
expect(service.appliedSearchQuery()).toBe('Beta');
|
||||
|
||||
// The user keeps typing while the applied term reaches the URL.
|
||||
service.onSearchInput('Beta Movie');
|
||||
navigateTo(`${DOWNLOADS_URL}?q=Beta`);
|
||||
|
||||
expect(service.searchQuery()).toBe('Beta Movie');
|
||||
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('Beta Movie');
|
||||
});
|
||||
|
||||
it('clears in-flight typing when the navigation leaves the page', () => {
|
||||
service.onSearchInput('Beta Movie');
|
||||
navigateTo('/workspace/sources');
|
||||
|
||||
expect(service.searchQuery()).toBe('');
|
||||
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
expect(service.appliedSearchQuery()).toBe('');
|
||||
expect(service.searchQuery()).toBe('');
|
||||
});
|
||||
|
||||
it('adopts a same-page q that differs from the applied term', () => {
|
||||
// Back/forward and the command palette change `q` in place; that is
|
||||
// real search intent and wins over whatever is being typed.
|
||||
service.onSearchInput('Beta Movie');
|
||||
navigateTo(`${DOWNLOADS_URL}?q=Alpha`);
|
||||
|
||||
expect(service.searchQuery()).toBe('Alpha');
|
||||
|
||||
jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS);
|
||||
|
||||
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`);
|
||||
|
||||
expect(service.searchQuery()).toBe('Gamma');
|
||||
expect(service.appliedSearchQuery()).toBe('Gamma');
|
||||
expect(routeState.currentUrl()).toBe(`${DOWNLOADS_URL}?q=Gamma`);
|
||||
|
||||
navigateTo('/workspace/settings/general');
|
||||
|
||||
expect(service.searchQuery()).toBe('');
|
||||
expect(service.appliedSearchQuery()).toBe('');
|
||||
});
|
||||
});
|
||||
+45
-18
@@ -7,6 +7,7 @@ import { XtreamStore } from '@iptvnator/portal/xtream/data-access';
|
||||
import { parseWorkspaceShellRoute } from '@iptvnator/workspace/shell/util';
|
||||
import { SEARCH_INPUT_DEBOUNCE_MS } from './helpers/workspace-shell-constants';
|
||||
import {
|
||||
getRoutePath,
|
||||
getRouteQueryParam,
|
||||
syncSearchQueryParam,
|
||||
} from './helpers/workspace-shell-route-utils';
|
||||
@@ -22,17 +23,13 @@ export class WorkspaceShellSearchSyncService {
|
||||
|
||||
private searchDebounceTimeoutId: ReturnType<typeof setTimeout> | null =
|
||||
null;
|
||||
private lastSyncedUrl: string | null = null;
|
||||
|
||||
readonly searchQuery = signal('');
|
||||
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(
|
||||
@@ -113,36 +110,66 @@ 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);
|
||||
}
|
||||
|
||||
private syncSearchFromUrl(url: string): void {
|
||||
if (parseWorkspaceShellRoute(url).usesQuerySearch) {
|
||||
this.setSearchState(getRouteQueryParam(this.router, url, 'q'));
|
||||
const previousUrl = this.lastSyncedUrl;
|
||||
this.lastSyncedUrl = url;
|
||||
|
||||
const nextTerm = parseWorkspaceShellRoute(url).usesQuerySearch
|
||||
? getRouteQueryParam(this.router, url, 'q')
|
||||
: '';
|
||||
|
||||
// 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() &&
|
||||
this.router.lastSuccessfulNavigation()?.trigger === 'imperative'
|
||||
) {
|
||||
return;
|
||||
}
|
||||
|
||||
this.setSearchState('');
|
||||
this.setSearchState(nextTerm);
|
||||
}
|
||||
|
||||
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