diff --git a/.changes/search-trailing-space-typing.md b/.changes/search-trailing-space-typing.md new file mode 100644 index 000000000..005d8d24a --- /dev/null +++ b/.changes/search-trailing-space-typing.md @@ -0,0 +1,9 @@ +--- +type: fix +area: search +issues: [1338] +--- + +Typing a space in the portal search box and pausing briefly no longer deletes +the space — continuing to type "Bein Sports" no longer collapses into +"BeinSports". diff --git a/docs/architecture/workspace-shell.md b/docs/architecture/workspace-shell.md index 6470d49e0..8f2e8d168 100644 --- a/docs/architecture/workspace-shell.md +++ b/docs/architecture/workspace-shell.md @@ -165,12 +165,18 @@ Search is shell-owned and route-aware: 8. The URL is authoritative for the search box only when it carries search intent. `WorkspaceShellSearchSyncService` re-reads `q` on every `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. + page and carries the term already applied is always ignored — whether or + not a debounce is still pending. Otherwise a page writing an unrelated + query param (a downloads filter chip, a refresh bump) or the router echoing + back our own trimmed `q` would reset the box to the applied term, eating + everything typed since: the whole word while the first keystroke is still + debouncing, or a just-typed trailing space once the debounce has fired + ("Bein " would snap to "Bein" and typing on would yield "BeinSports"). + Applied terms are always stored trimmed (`applySearchQuery` and + `setSearchState` both trim), so the echoed `q` compares directly; the box + keeps exactly what the user typed. 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. diff --git a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.spec.ts b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.spec.ts index 67ba19b76..f3f5471b0 100644 --- a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.spec.ts +++ b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.spec.ts @@ -228,6 +228,65 @@ describe('WorkspaceShellSearchSyncService', () => { expect(service.searchQuery()).toBe('Beta Movie'); }); + it('keeps a trailing space when its own trimmed q echo lands after the debounce', () => { + // #1338 residual: type "Bein ", pause past the debounce. The applied + // term is trimmed, written to the URL as q=Bein, and the router echoes + // that navigation back with no debounce pending anymore. The echo must + // not snap the box back to "Bein" — typing on would yield "BeinSports". + service.onSearchInput('Bein '); + jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS); + + expect(service.appliedSearchQuery()).toBe('Bein'); + expect(service.searchQuery()).toBe('Bein '); + + TestBed.flushEffects(); + expect(router.navigateByUrl).toHaveBeenCalledWith( + `${DOWNLOADS_URL}?q=Bein`, + { replaceUrl: true } + ); + + navigateTo(`${DOWNLOADS_URL}?q=Bein`); + + expect(service.searchQuery()).toBe('Bein '); + + service.onSearchInput('Bein Sports'); + jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS); + + expect(service.appliedSearchQuery()).toBe('Bein Sports'); + expect(service.searchQuery()).toBe('Bein Sports'); + }); + + it('still adopts a history entry matching the applied term after typing settles', () => { + // The echo guard must stay scoped to app-initiated navigations even + // when no debounce is pending: back/forward re-applies exactly what + // the entry carries, dropping the uncommitted trailing space. + service.onSearchInput('Bein '); + jest.advanceTimersByTime(SEARCH_INPUT_DEBOUNCE_MS); + + navigateTo(`${DOWNLOADS_URL}?q=Bein`, 'popstate'); + + expect(service.searchQuery()).toBe('Bein'); + expect(service.appliedSearchQuery()).toBe('Bein'); + }); + + it('adopts an externally crafted q with edge whitespace in trimmed form', () => { + // A deep link ?q=Bein%20 must not put an untrimmed term into the + // applied signal: the URL-sync effect rewrites the URL trimmed, and + // an untrimmed applied term would fail the echo guard's equality + // check — snapping the box and dispatching the portal search twice. + navigateTo(`${DOWNLOADS_URL}?q=Bein%20`); + + expect(service.searchQuery()).toBe('Bein'); + expect(service.appliedSearchQuery()).toBe('Bein'); + + // The trimmed rewrite's echo is our own navigation — skipped. + TestBed.flushEffects(); + navigateTo(`${DOWNLOADS_URL}?q=Bein`); + + expect(service.searchQuery()).toBe('Bein'); + expect(service.appliedSearchQuery()).toBe('Bein'); + }); + it('syncs the search box from the url when nothing is being typed', () => { navigateTo(`${DOWNLOADS_URL}?q=Gamma`); diff --git a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.ts b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.ts index c75b48d1e..e41edeae1 100644 --- a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.ts +++ b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell-search-sync.service.ts @@ -109,20 +109,34 @@ export class WorkspaceShellSearchSyncService { this.syncSearchFromUrl(this.routeState.currentUrl()); } + /** + * Adopts a term from outside the input box (URL `q`, command palette) into + * both signals. The trim keeps the applied-terms-are-always-trimmed + * invariant structural for URL-sourced values too: an externally crafted + * `?q=Bein%20` must not put an untrimmed term into `appliedSearchQuery`, + * or the URL-sync effect's trimmed rewrite would fail the echo guard's + * equality check and snap the box — the same eaten-keystroke cycle the + * guard exists to prevent — while the portal stores dispatch twice. + */ setSearchState(value: string): void { this.cancelPendingSearchApply(); - this.searchQuery.set(value); - this.appliedSearchQuery.set(value); + const trimmed = value.trim(); + this.searchQuery.set(trimmed); + this.appliedSearchQuery.set(trimmed); } /** - * 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. + * Applies a term now, always in its trimmed form: the URL sync and the + * portal stores only ever act on the trimmed term, and an untrimmed + * applied term would make the router echo of our own `q` look like + * foreign search intent. The box (`searchQuery`) keeps what was actually + * typed, trailing whitespace included. This supersedes a queued debounce + * — pressing Enter commits the term, and the keystroke that is still + * waiting must not reapply the older one behind it. */ applySearchQuery(value: string): void { this.cancelPendingSearchApply(); - this.appliedSearchQuery.set(value); + this.appliedSearchQuery.set(value.trim()); } private syncSearchFromUrl(url: string): void { @@ -138,18 +152,24 @@ export class WorkspaceShellSearchSyncService { // 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. + // term, silently eating everything typed since — the whole word when + // the first keystroke has not been applied yet, or just-typed trailing + // whitespace once the debounce has fired ("Bein " snaps to "Bein" and + // typing on yields "BeinSports"). Applied terms are always trimmed, so + // the echoed `q` compares directly. // // 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. + // + // The comparison trims `nextTerm` because adoption would too: a URL + // still carrying a not-yet-rewritten untrimmed `q` adopts to exactly + // the applied state, so syncing could only cancel a pending debounce. if ( - this.searchDebounceTimeoutId !== null && previousUrl !== null && getRoutePath(url) === getRoutePath(previousUrl) && - nextTerm === this.appliedSearchQuery() && + nextTerm.trim() === this.appliedSearchQuery() && this.router.lastSuccessfulNavigation()?.trigger === 'imperative' ) { return; diff --git a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell.facade.spec.ts b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell.facade.spec.ts index e3f8cede4..29cbb46aa 100644 --- a/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell.facade.spec.ts +++ b/libs/workspace/shell/feature/src/lib/workspace-shell/services/workspace-shell.facade.spec.ts @@ -117,6 +117,10 @@ describe('WorkspaceShellFacade', () => { let playerCommands: { ensureEmbeddedMpvSupportLoaded: jest.Mock; }; + // Mirrors Navigation.trigger: 'imperative' is the app navigating, + // 'popstate' is the user moving through browser history. Mutable so a + // test can exercise the history-authoritative branch of the search sync. + let navigationTrigger: 'imperative' | 'popstate'; let router: { url: string; events: ReturnType; @@ -125,6 +129,7 @@ describe('WorkspaceShellFacade', () => { parseUrl: jest.Mock; createUrlTree: jest.Mock; isActive: jest.Mock; + lastSuccessfulNavigation: () => { trigger: string }; }; let playlistsService: { clearPortalRecentlyViewed: jest.Mock; @@ -160,6 +165,7 @@ describe('WorkspaceShellFacade', () => { }; beforeEach(() => { + navigationTrigger = 'imperative'; showDashboardSignal = signal(true); runtime = { isElectron: true, @@ -196,6 +202,7 @@ describe('WorkspaceShellFacade', () => { parseUrl: jest.fn((url: string) => createParseUrl(url)), createUrlTree: jest.fn(), isActive: jest.fn(), + lastSuccessfulNavigation: () => ({ trigger: navigationTrigger }), }; playlistsService = { clearPortalRecentlyViewed: jest