mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-10 18:36:15 -08:00
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
2af67f5dae
commit
935200c4b8
4 files changed
+48
-9
No files matched your search
@@ -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.
|
||||
|
||||
+18
@@ -269,6 +269,24 @@ describe('WorkspaceShellSearchSyncService', () => {
|
||||
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`);
|
||||
|
||||
|
||||
+12
-2
@@ -109,10 +109,20 @@ 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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+6
-1
@@ -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<typeof of>;
|
||||
@@ -161,6 +165,7 @@ describe('WorkspaceShellFacade', () => {
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
navigationTrigger = 'imperative';
|
||||
showDashboardSignal = signal(true);
|
||||
runtime = {
|
||||
isElectron: true,
|
||||
@@ -197,7 +202,7 @@ describe('WorkspaceShellFacade', () => {
|
||||
parseUrl: jest.fn((url: string) => createParseUrl(url)),
|
||||
createUrlTree: jest.fn(),
|
||||
isActive: jest.fn(),
|
||||
lastSuccessfulNavigation: () => ({ trigger: 'imperative' }),
|
||||
lastSuccessfulNavigation: () => ({ trigger: navigationTrigger }),
|
||||
};
|
||||
playlistsService = {
|
||||
clearPortalRecentlyViewed: jest
|
||||
|
||||
Reference in new issue
Block a user