From 7efb8c4f668b64f5168c3a7128342a48807f3f02 Mon Sep 17 00:00:00 2001 From: 4gray <4gray@users.noreply.github.com> Date: Sun, 23 Aug 2026 11:47:43 +0200 Subject: [PATCH] fix(search): keep a trailing space when the trimmed q echo lands after the debounce (#1474) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(search): keep a trailing space when the trimmed q echo lands after the debounce The #1432 guard only held while a debounce was still pending. Once it fired, the router echo of our own trimmed q (replaceUrl navigation) reset the one-way-bound search box to the trimmed term, deleting the just-typed trailing space — typing "Bein Sports" collapsed into "BeinSports". Applied terms are now always trimmed at the apply choke point (URL sync and portal stores only ever act on the trimmed form anyway), so the echoed q compares directly, and the echo guard no longer requires a pending debounce. Back/forward stays authoritative via the untouched imperative-trigger check. Residual part of #1338. Co-Authored-By: Claude Fable 5 * fix(search): make the trimmed-applied-term invariant structural and update the shell contract doc Review follow-ups on the echo-guard widening: - setSearchState now trims too, so URL-sourced terms (deep links with ?q=Bein%20, actor/discover prefills passing raw provider titles) cannot put an untrimmed term into appliedSearchQuery — previously that path failed the echo guard's equality check, snapped the box, and dispatched the portal search twice. Regression spec added. - docs/architecture/workspace-shell.md item 8 updated: the applied-term echo is now always ignored, not only while input is still debouncing. - The facade spec's router mock exposes a mutable navigation trigger so facade-level tests can exercise the popstate branch. Co-Authored-By: Claude Fable 5 * fix(search): compare the echoed q in trimmed form in the echo guard Adoption trims, so a same-page imperative navigation still carrying a not-yet-rewritten untrimmed q adopts to exactly the applied state — syncing it could only cancel a pending debounce. Comparing the trimmed form closes that window. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: 4gray Co-authored-by: Claude Fable 5 --- .changes/search-trailing-space-typing.md | 9 +++ docs/architecture/workspace-shell.md | 18 ++++-- ...orkspace-shell-search-sync.service.spec.ts | 59 +++++++++++++++++++ .../workspace-shell-search-sync.service.ts | 40 +++++++++---- .../services/workspace-shell.facade.spec.ts | 7 +++ 5 files changed, 117 insertions(+), 16 deletions(-) create mode 100644 .changes/search-trailing-space-typing.md 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