From 8acfa772f9ead74b55b2a4d8f2263ef92d25fd64 Mon Sep 17 00:00:00 2001 From: Alex Sorafumo Date: Fri, 2 Oct 2026 13:12:34 +1000 Subject: [PATCH 1/2] fix(signage-manager): fix shared list, decode and picker behaviour - Load displays for the inventory and command palette with querySignageDisplays, so never-seen displays are not shown online. - decodeEntityNames keeps the class prototype, so getters such as SignageMedia.media_url work. - PagedList.reset(null) clears loading. - PagedSearch exposes error and retry; the display and zone pickers show a load error with Retry. - The command palette hides the previous term's matches while the new term is debounced. - The shared Retry button no longer submits a surrounding form. --- apps/signage-manager/USER_STORIES.md | 2 + .../app/shared/command-palette.component.ts | 8 +++- .../src/app/shared/command-palette.service.ts | 10 ++-- .../app/shared/decode-entity-names.util.ts | 19 +++++--- .../shared/display-select-modal.component.ts | 11 ++++- .../src/app/shared/paged-list.ts | 3 ++ .../src/app/shared/paged-search.ts | 22 +++++++-- .../app/shared/zone-select-tree.component.ts | 11 ++++- .../src/app/signage-inventory.service.ts | 8 ++-- .../shared/command-palette.component.spec.ts | 14 ++++++ .../shared/decode-entity-names.util.spec.ts | 12 +++++ .../display-select-modal.component.spec.ts | 47 +++++++++++++++++++ .../src/tests/shared/paged-list.spec.ts | 16 +++++++ .../src/tests/shared/paged-search.spec.ts | 14 ++++++ .../shared/zone-select-tree.component.spec.ts | 29 ++++++++++++ .../src/tests/signage-display-search.spec.ts | 12 +++-- .../tests/signage-inventory.service.spec.ts | 30 ++++++++++++ .../src/lib/load-error.component.ts | 9 +++- .../src/tests/load-error.component.spec.ts | 32 +++++++++++++ 19 files changed, 278 insertions(+), 31 deletions(-) create mode 100644 libs/components/src/tests/load-error.component.spec.ts diff --git a/apps/signage-manager/USER_STORIES.md b/apps/signage-manager/USER_STORIES.md index 1de95822b69..4bfbdebcdc6 100644 --- a/apps/signage-manager/USER_STORIES.md +++ b/apps/signage-manager/USER_STORIES.md @@ -236,6 +236,7 @@ These stories cover the current app workflows: - Users with update permission can add or remove playlists from the zone. - The display tab shows displays assigned to the zone. - Users with update permission can add displays to the zone. +- When the display or zone search in an add dialog fails, the dialog shows an error with a retry, not an empty result. --- @@ -272,6 +273,7 @@ These stories cover the current app workflows: - The current time indicator appears when the selected date is today. - Users can search schedules by display, zone, playlist, and source labels where applicable. - Timeline rows link to the related display or zone detail page. +- Display rows show the same online, offline, or never seen status as the displays page. - Empty and filtered states explain when no rows are available. - Users must confirm a change that makes two takeover playlists play at the same time on a display in the next 14 days. The check runs when users save playlist schedules and when they assign a playlist to a display or zone. The warning names the display, the other playlist, and the start time of the overlap. diff --git a/apps/signage-manager/src/app/shared/command-palette.component.ts b/apps/signage-manager/src/app/shared/command-palette.component.ts index 70da222cbb6..4fe4c933711 100644 --- a/apps/signage-manager/src/app/shared/command-palette.component.ts +++ b/apps/signage-manager/src/app/shared/command-palette.component.ts @@ -186,8 +186,12 @@ export class CommandPaletteComponent { detail: '', select: () => this._open([item.route]), })); - // Keep old matches out once the term is cleared - const matches = term ? this._matches.value() : undefined; + // Keep out matches for an older term, such as while the new term + // waits for its debounce, so Enter cannot open a stale match + const current = + !!term && + term === this._search_debounced.value().trim().toLowerCase(); + const matches = current ? this._matches.value() : undefined; if (!matches) return pages; return [ ...pages, diff --git a/apps/signage-manager/src/app/shared/command-palette.service.ts b/apps/signage-manager/src/app/shared/command-palette.service.ts index daabe95316f..a9be298ce91 100644 --- a/apps/signage-manager/src/app/shared/command-palette.service.ts +++ b/apps/signage-manager/src/app/shared/command-palette.service.ts @@ -7,12 +7,12 @@ import { querySignageMedia, querySignagePlaylists, querySignageTemplates, - querySystems, queryZones, SignageMedia, SignagePlaylist, SignageTemplate, } from '@placeos/ts-client'; +import { querySignageDisplays } from '../displays/signage-display'; import { SignageContextService } from '../signage-context.service'; import { searchParam } from '../signage-service.util'; import { decodeEntityNames } from './decode-entity-names.util'; @@ -90,16 +90,12 @@ export class CommandPaletteService { }; const [displays, playlists, templates, zones, media] = await Promise.all([ - settle( - querySystems({ ...params, signage: true } as any), - ), + settle(querySignageDisplays({ ...params, signage: true })), settle(querySignagePlaylists(params)), this._context.templates_enabled() ? settle(querySignageTemplates(group_params)) : Promise.resolve([] as SignageTemplate[]), - settle( - queryZones({ ...group_params, tags: 'signage' } as any), - ), + settle(queryZones({ ...group_params, tags: 'signage' })), settle(querySignageMedia(params)), ]); return { displays, playlists, templates, zones, media }; diff --git a/apps/signage-manager/src/app/shared/decode-entity-names.util.ts b/apps/signage-manager/src/app/shared/decode-entity-names.util.ts index 8ccf91b8a28..f2f23280e1a 100644 --- a/apps/signage-manager/src/app/shared/decode-entity-names.util.ts +++ b/apps/signage-manager/src/app/shared/decode-entity-names.util.ts @@ -18,18 +18,23 @@ const NAME_FIELDS = ['name', 'display_name']; const NESTED_FIELDS = ['group', 'user', 'zone']; /** Returns a shallow copy of `item` with name fields (and one nested level of - * group/user/zone) HTML-entity decoded. Idempotent for already-decoded values. */ + * group/user/zone) HTML-entity decoded. The copy keeps the prototype of + * `item`, so getters such as `SignageMedia.media_url` still work. */ export function decodeEntityNames(item: T): T { if (!item || typeof item !== 'object') return item; - const copy: any = { ...item }; + const copy: T = Object.assign( + Object.create(Object.getPrototypeOf(item)), + item, + ); + const fields = copy as Record; for (const field of NAME_FIELDS) { - if (typeof copy[field] === 'string') { - copy[field] = decodeEntities(copy[field]); - } + const value = fields[field]; + if (typeof value === 'string') fields[field] = decodeEntities(value); } for (const field of NESTED_FIELDS) { - if (copy[field] && typeof copy[field] === 'object') { - copy[field] = decodeEntityNames(copy[field]); + const value = fields[field]; + if (value && typeof value === 'object') { + fields[field] = decodeEntityNames(value); } } return copy; diff --git a/apps/signage-manager/src/app/shared/display-select-modal.component.ts b/apps/signage-manager/src/app/shared/display-select-modal.component.ts index fd502f214bd..ab4eb5d14f8 100644 --- a/apps/signage-manager/src/app/shared/display-select-modal.component.ts +++ b/apps/signage-manager/src/app/shared/display-select-modal.component.ts @@ -4,7 +4,11 @@ import { MatRippleModule } from '@angular/material/core'; import { MatDialogModule } from '@angular/material/dialog'; import { MatFormFieldModule } from '@angular/material/form-field'; import { MatInputModule } from '@angular/material/input'; -import { IconComponent, TranslatePipe } from '@placeos/components'; +import { + IconComponent, + LoadErrorComponent, + TranslatePipe, +} from '@placeos/components'; import { PlaceSystem } from '@placeos/ts-client'; import { SignageDisplayService } from '../displays/signage-display.service'; import { IntersectDirective } from './intersect.directive'; @@ -81,6 +85,8 @@ import { byDisplayName, PagedSearch } from './paged-search'; intersect (intersect)="list.loadMore()" > + } @else if (list.error()) { + } } @else if (list.loading()) {
+ } @else if (list.error()) { + } @else {
{ this._next = null; this._has_more.set(false); this._error.set(false); + // A page of the old query may still be in flight. It will not clear + // the flag, as its token is stale. + this._loading.set(false); this._replace = !!query && keep_items; if (!this._replace) { this._items.set([]); diff --git a/apps/signage-manager/src/app/shared/paged-search.ts b/apps/signage-manager/src/app/shared/paged-search.ts index 1852bd51651..7351fb6ce1e 100644 --- a/apps/signage-manager/src/app/shared/paged-search.ts +++ b/apps/signage-manager/src/app/shared/paged-search.ts @@ -15,10 +15,14 @@ export class PagedSearch { public readonly items: Signal; public readonly loading: Signal; public readonly has_more: Signal; + /** Whether the last page failed to load. `retry` loads it again. */ + public readonly error: Signal; + // Term of the current query, so a retry can run it again + private _term = ''; constructor( /** Builds the first page of results, null when the user may not query */ - query: (search: string) => QueryResponse | null, + private readonly _query: (search: string) => QueryResponse | null, sort?: (a: T, b: T) => number, debounce_ms = 400, ) { @@ -26,20 +30,32 @@ export class PagedSearch { this.items = this._list.items; this.loading = this._list.loading; this.has_more = this._list.has_more; + this.error = this._list.error; const search_debounced = debounced(this.search, debounce_ms); effect(() => { const term = search_debounced.value(); - untracked(() => this._list.reset(query(term))); + untracked(() => { + this._term = term; + this._list.reset(this._query(term)); + }); }); } public loadMore() { this._list.loadMore(); } + + /** Load the page that failed again, or the first page when it failed */ + public retry() { + if (!this._list.retry()) this._list.reset(this._query(this._term)); + } } /** Displays and zones show a display_name in preference to their name */ -export function byDisplayName(a: any, b: any) { +export function byDisplayName( + a: { name: string; display_name?: string }, + b: { name: string; display_name?: string }, +) { return (a.display_name || a.name).localeCompare(b.display_name || b.name); } diff --git a/apps/signage-manager/src/app/shared/zone-select-tree.component.ts b/apps/signage-manager/src/app/shared/zone-select-tree.component.ts index bcbf40b23d4..e3f6ed7472a 100644 --- a/apps/signage-manager/src/app/shared/zone-select-tree.component.ts +++ b/apps/signage-manager/src/app/shared/zone-select-tree.component.ts @@ -15,7 +15,11 @@ import { MatRippleModule } from '@angular/material/core'; import { MatFormFieldModule } from '@angular/material/form-field'; import { MatInputModule } from '@angular/material/input'; import { MatTooltipModule } from '@angular/material/tooltip'; -import { IconComponent, TranslatePipe } from '@placeos/components'; +import { + IconComponent, + LoadErrorComponent, + TranslatePipe, +} from '@placeos/components'; import { PlaceZone } from '@placeos/ts-client'; import { IntersectDirective } from './intersect.directive'; import { PagedSearch } from './paged-search'; @@ -203,6 +207,8 @@ interface ZoneSelectTreeNode { intersect (intersect)="list().loadMore()" >
+ } @else if (list().error()) { + } } @else if (list().loading()) {
+ } @else if (list().error()) { + } @else {
{ ]); }); + // Enter before the debounce ends would open a match for the old term + it('hides matches for the previous term while the new term waits', async () => { + search_all.mockResolvedValue({ + ...EMPTY, + displays: [{ id: 'd1', name: 'SIGNAGE 1', display_name: 'Lobby' }], + }); + const component = make(); + await type(component, 'lob'); + + component.search.set('xyz'); + + expect(labels(component)).toEqual([]); + }); + it('moves the highlight with the arrow keys and wraps', async () => { const component = make(); const key = (key: string) => diff --git a/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts b/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts index 4dc1999d809..6681140a4ae 100644 --- a/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts +++ b/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts @@ -1,3 +1,4 @@ +import { SignageMedia } from '@placeos/ts-client'; import { decodeEntities, decodeEntityNames, @@ -37,6 +38,17 @@ describe('decodeEntityNames', () => { expect(result.user.name).toBe('Tom & Jerry'); }); + // Edit and preview build the media address from these getters + it('keeps the class of the item, so its getters still work', () => { + const media = decodeEntityNames( + new SignageMedia({ name: 'R&D', media_id: 'upload-1' }), + ); + + expect(media).toBeInstanceOf(SignageMedia); + expect(media.name).toBe('R&D'); + expect(media.media_url).toBe('/api/engine/v2/uploads/upload-1/url'); + }); + it('passes through non-objects', () => { expect(decodeEntityNames(null as any)).toBeNull(); expect(decodeEntityNames('x' as any)).toBe('x'); diff --git a/apps/signage-manager/src/tests/shared/display-select-modal.component.spec.ts b/apps/signage-manager/src/tests/shared/display-select-modal.component.spec.ts index 204ffabdd65..bf07a313b37 100644 --- a/apps/signage-manager/src/tests/shared/display-select-modal.component.spec.ts +++ b/apps/signage-manager/src/tests/shared/display-select-modal.component.spec.ts @@ -44,3 +44,50 @@ describe('DisplaySelectModalComponent', () => { ).toEqual(['d2', 'd1']); }); }); + +// The real template, so a failed search shows an error and not "no displays" +describe('DisplaySelectModalComponent errors', () => { + const flush = () => new Promise((resolve) => setTimeout(resolve)); + const queryDisplays = vi.fn(); + + beforeEach(async () => { + vi.clearAllMocks(); + await TestBed.configureTestingModule({ + imports: [DisplaySelectModalComponent], + providers: [ + { provide: MAT_DIALOG_DATA, useValue: {} }, + { provide: SignageDisplayService, useValue: { queryDisplays } }, + ], + }).compileComponents(); + }); + + it('shows a load error with a retry that queries again', async () => { + queryDisplays + .mockReturnValueOnce(Promise.reject(new Error('offline'))) + .mockReturnValueOnce( + Promise.resolve({ + data: [{ id: 'd1', name: 'Lobby' }], + total: 1, + next: null, + }), + ); + const fixture = TestBed.createComponent(DisplaySelectModalComponent); + fixture.detectChanges(); + await flush(); + fixture.detectChanges(); + + const element: HTMLElement = fixture.nativeElement; + const retry = element.querySelector( + 'load-error button', + ); + expect(retry).toBeTruthy(); + + retry?.click(); + await flush(); + fixture.detectChanges(); + + expect(queryDisplays).toHaveBeenCalledTimes(2); + expect(element.querySelector('load-error')).toBeNull(); + expect(element.textContent).toContain('Lobby'); + }); +}); diff --git a/apps/signage-manager/src/tests/shared/paged-list.spec.ts b/apps/signage-manager/src/tests/shared/paged-list.spec.ts index 73726f749bc..661da240f86 100644 --- a/apps/signage-manager/src/tests/shared/paged-list.spec.ts +++ b/apps/signage-manager/src/tests/shared/paged-list.spec.ts @@ -140,6 +140,22 @@ describe('PagedList', () => { expect(idsOf(list)).toEqual(['fresh']); }); + // The stale page does not clear the flag, so the list would show + // loading forever + it('stops loading when cleared while a page is in flight', async () => { + const list = new PagedList(); + let resolveStale: (page: unknown) => void = () => {}; + list.reset(new Promise((resolve) => (resolveStale = resolve))); + expect(list.loading()).toBe(true); + + list.reset(null); + resolveStale(pageOf(['stale'], 1)); + await flush(); + + expect(list.loading()).toBe(false); + expect(idsOf(list)).toEqual([]); + }); + it('keeps the items on screen until a reload of the same query lands', async () => { const list = await load(pageOf(['a', 'b'], 2)); let resolveReload: (page: unknown) => void = () => {}; diff --git a/apps/signage-manager/src/tests/shared/paged-search.spec.ts b/apps/signage-manager/src/tests/shared/paged-search.spec.ts index e771cec0d1d..57efe7ef2be 100644 --- a/apps/signage-manager/src/tests/shared/paged-search.spec.ts +++ b/apps/signage-manager/src/tests/shared/paged-search.spec.ts @@ -119,6 +119,20 @@ describe('PagedSearch', () => { expect(list.loading()).toBe(false); }); + it('flags a failed first page and runs the query again on retry', async () => { + query.mockReturnValueOnce(Promise.reject(new Error('offline'))); + const list = await make(); + expect(list.error()).toBe(true); + + list.retry(); + await flush(); + + expect(query).toHaveBeenCalledTimes(2); + expect(query).toHaveBeenLastCalledWith(''); + expect(list.error()).toBe(false); + expect(list.items().map((_) => _.id)).toEqual(['b', 'a']); + }); + it('stays empty when the query is not allowed', async () => { query.mockReturnValue(null); const list = await make(); diff --git a/apps/signage-manager/src/tests/shared/zone-select-tree.component.spec.ts b/apps/signage-manager/src/tests/shared/zone-select-tree.component.spec.ts index 0a4ed56fb3d..83f10c0db31 100644 --- a/apps/signage-manager/src/tests/shared/zone-select-tree.component.spec.ts +++ b/apps/signage-manager/src/tests/shared/zone-select-tree.component.spec.ts @@ -30,6 +30,7 @@ describe('ZoneSelectTreeComponent', () => { items: signal(zones), loading: signal(false), has_more: signal(false), + error: signal(false), loadMore: vi.fn(), } as unknown as PagedSearch; fixture.componentRef.setInput('list', list); @@ -86,6 +87,7 @@ describe('ZoneSelectTreeComponent', () => { ]), loading: signal(false), has_more: signal(false), + error: signal(false), loadMore: vi.fn(), } as unknown as PagedSearch); const selected = vi.fn(); @@ -243,3 +245,30 @@ describe('ZoneSelectTreeComponent', () => { ]); }); }); + +// The real template, so a failed search shows an error and not "no zones" +describe('ZoneSelectTreeComponent errors', () => { + it('shows a load error that retries the search', async () => { + await TestBed.configureTestingModule({ + imports: [ZoneSelectTreeComponent], + }).compileComponents(); + const fixture = TestBed.createComponent(ZoneSelectTreeComponent); + const retry = vi.fn(); + const list = { + search: signal(''), + items: signal([]), + loading: signal(false), + has_more: signal(false), + error: signal(true), + loadMore: vi.fn(), + retry, + } as unknown as PagedSearch; + fixture.componentRef.setInput('list', list); + fixture.detectChanges(); + + const element: HTMLElement = fixture.nativeElement; + element.querySelector('load-error button')?.click(); + + expect(retry).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/signage-manager/src/tests/signage-display-search.spec.ts b/apps/signage-manager/src/tests/signage-display-search.spec.ts index bb4858ca57b..b49835e8352 100644 --- a/apps/signage-manager/src/tests/signage-display-search.spec.ts +++ b/apps/signage-manager/src/tests/signage-display-search.spec.ts @@ -13,7 +13,6 @@ import { querySignageMedia, querySignagePlaylists, querySignageTemplates, - querySystems, queryZones, } from '@placeos/ts-client'; @@ -165,7 +164,7 @@ describe('SignageDisplayService display search', () => { await init(); const palette = TestBed.inject(CommandPaletteService); vi.clearAllMocks(); - (querySystems as any).mockResolvedValue(pageOf(['lobby'])); + mockDisplayList(pageOf(['lobby'])); for (const query of [ querySignagePlaylists, querySignageMedia, @@ -176,12 +175,15 @@ describe('SignageDisplayService display search', () => { } expect(await palette.searchAll(' ')).toMatchObject({ displays: [] }); - expect(querySystems).not.toHaveBeenCalled(); + expect(query).not.toHaveBeenCalled(); const results = await palette.searchAll('lobby'); expect(results.displays.map(({ id }) => id)).toEqual(['lobby']); - const params = (querySystems as any).mock.calls[0][0]; - expect(params).toMatchObject({ q: 'lobby', limit: 5, signage: true }); + expect(lastDisplayQuery()).toMatchObject({ + q: 'lobby', + limit: 5, + signage: true, + }); }); }); diff --git a/apps/signage-manager/src/tests/signage-inventory.service.spec.ts b/apps/signage-manager/src/tests/signage-inventory.service.spec.ts index d990cccbdb7..06be3ca3837 100644 --- a/apps/signage-manager/src/tests/signage-inventory.service.spec.ts +++ b/apps/signage-manager/src/tests/signage-inventory.service.spec.ts @@ -4,7 +4,10 @@ import { MatDialog } from '@angular/material/dialog'; import { OrganisationService, SettingsService } from '@placeos/common'; import { PlaceSystem, + query, querySignageMedia, + querySignagePlaylists, + queryZones, showSignageMedia, SignageMedia, SignagePlaylist, @@ -46,6 +49,33 @@ describe('SignageInventoryService', () => { return TestBed.inject(SignageInventoryService); } + // The schedules timeline shows its online status from these displays + it('keeps a display that never checked in as never seen', async () => { + const service = createService(); + vi.spyOn( + TestBed.inject(SignageContextService), + 'canQueryLists', + ).mockReturnValue(true); + const empty_page = { data: [], total: 0, next: null }; + vi.mocked(querySignagePlaylists).mockResolvedValue( + empty_page as never, + ); + vi.mocked(queryZones).mockResolvedValue(empty_page as never); + vi.mocked(query).mockImplementation(((params: { + fn: (raw: Partial) => PlaceSystem; + }) => + Promise.resolve({ + data: [params.fn({ id: 'd1', name: 'New' })], + total: 1, + next: null, + })) as unknown as typeof query); + + const { displays } = await service.loadSignageInventory(); + + expect(displays.map(({ id }) => id)).toEqual(['d1']); + expect(displays[0].signage_last_seen).toBe(0); + }); + describe('content report', () => { const takeover = (id: string, play_cron: string) => new SignagePlaylist({ diff --git a/libs/components/src/lib/load-error.component.ts b/libs/components/src/lib/load-error.component.ts index acc3ff0b68f..10bb7b33718 100644 --- a/libs/components/src/lib/load-error.component.ts +++ b/libs/components/src/lib/load-error.component.ts @@ -18,7 +18,14 @@ import { TranslatePipe } from './translate.pipe'; > error

{{ 'COMMON.LOAD_ERROR' | translate }}

-
diff --git a/libs/components/src/tests/load-error.component.spec.ts b/libs/components/src/tests/load-error.component.spec.ts new file mode 100644 index 00000000000..777b68826ba --- /dev/null +++ b/libs/components/src/tests/load-error.component.spec.ts @@ -0,0 +1,32 @@ +import { Component } from '@angular/core'; +import { createHostFactory, SpectatorHost } from '@ngneat/spectator/vitest'; + +import { LoadErrorComponent } from '../lib/load-error.component'; + +@Component({ selector: 'test-host', template: '', standalone: false }) +class HostComponent { + public retried = 0; + public submitted = 0; +} + +describe('LoadErrorComponent', () => { + let spectator: SpectatorHost; + + const createHost = createHostFactory({ + component: LoadErrorComponent, + host: HostComponent, + }); + + it('should retry without submitting a form it sits in', () => { + spectator = createHost( + `
+ + `, + ); + + spectator.click('button'); + + expect(spectator.hostComponent.retried).toBe(1); + expect(spectator.hostComponent.submitted).toBe(0); + }); +}); From 8405ccc195bd44ed29f8acb879222a953173cd59 Mon Sep 17 00:00:00 2001 From: Alex Sorafumo Date: Fri, 2 Oct 2026 13:24:37 +1000 Subject: [PATCH 2/2] fix(signage-manager): decode names once and retry only the current search - decodeEntityNames returns its own copies unchanged, so an item that passes through querySignageDisplays and then a list helper is decoded once. A saved "&" no longer turns into "&". - PagedSearch.retry() does nothing while a new term waits for its debounce, so it cannot show the old term's matches. --- .../app/shared/decode-entity-names.util.ts | 10 ++++++++-- .../src/app/shared/paged-search.ts | 3 +++ .../shared/decode-entity-names.util.spec.ts | 7 +++++++ .../src/tests/shared/paged-search.spec.ts | 20 +++++++++++++++++++ .../tests/signage-inventory.service.spec.ts | 6 ++++-- 5 files changed, 42 insertions(+), 4 deletions(-) diff --git a/apps/signage-manager/src/app/shared/decode-entity-names.util.ts b/apps/signage-manager/src/app/shared/decode-entity-names.util.ts index f2f23280e1a..15bb6af5c6a 100644 --- a/apps/signage-manager/src/app/shared/decode-entity-names.util.ts +++ b/apps/signage-manager/src/app/shared/decode-entity-names.util.ts @@ -16,12 +16,17 @@ export function decodeEntities(value: string): string { const NAME_FIELDS = ['name', 'display_name']; const NESTED_FIELDS = ['group', 'user', 'zone']; +// Copies this module made. Some items pass the data boundary twice, such as +// displays from `querySignageDisplays` read through `queryAll`, and a second +// decode would turn a saved `&` into `&`. +const _decoded = new WeakSet(); /** Returns a shallow copy of `item` with name fields (and one nested level of * group/user/zone) HTML-entity decoded. The copy keeps the prototype of - * `item`, so getters such as `SignageMedia.media_url` still work. */ + * `item`, so getters such as `SignageMedia.media_url` still work. An item + * this function returned is returned as is, so names decode only once. */ export function decodeEntityNames(item: T): T { - if (!item || typeof item !== 'object') return item; + if (!item || typeof item !== 'object' || _decoded.has(item)) return item; const copy: T = Object.assign( Object.create(Object.getPrototypeOf(item)), item, @@ -37,5 +42,6 @@ export function decodeEntityNames(item: T): T { fields[field] = decodeEntityNames(value); } } + _decoded.add(fields); return copy; } diff --git a/apps/signage-manager/src/app/shared/paged-search.ts b/apps/signage-manager/src/app/shared/paged-search.ts index 7351fb6ce1e..e784541d02f 100644 --- a/apps/signage-manager/src/app/shared/paged-search.ts +++ b/apps/signage-manager/src/app/shared/paged-search.ts @@ -47,6 +47,9 @@ export class PagedSearch { /** Load the page that failed again, or the first page when it failed */ public retry() { + // A new term waits for its debounce. Its search replaces the failed + // one, so a retry now would only show matches for the old term. + if (this.search() !== this._term) return; if (!this._list.retry()) this._list.reset(this._query(this._term)); } } diff --git a/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts b/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts index 6681140a4ae..728baf6d483 100644 --- a/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts +++ b/apps/signage-manager/src/tests/shared/decode-entity-names.util.spec.ts @@ -49,6 +49,13 @@ describe('decodeEntityNames', () => { expect(media.media_url).toBe('/api/engine/v2/uploads/upload-1/url'); }); + // Some items pass the data boundary twice, e.g. displays through queryAll + it('decodes an item only once', () => { + const item = decodeEntityNames({ id: '1', name: 'A & B' }); + + expect(decodeEntityNames(item).name).toBe('A & B'); + }); + it('passes through non-objects', () => { expect(decodeEntityNames(null as any)).toBeNull(); expect(decodeEntityNames('x' as any)).toBe('x'); diff --git a/apps/signage-manager/src/tests/shared/paged-search.spec.ts b/apps/signage-manager/src/tests/shared/paged-search.spec.ts index 57efe7ef2be..a6eee3eacc3 100644 --- a/apps/signage-manager/src/tests/shared/paged-search.spec.ts +++ b/apps/signage-manager/src/tests/shared/paged-search.spec.ts @@ -133,6 +133,26 @@ describe('PagedSearch', () => { expect(list.items().map((_) => _.id)).toEqual(['b', 'a']); }); + // The new term's search replaces the failed one, so a retry would only + // show matches for the old term under the new text + it('does not retry the old term while a new term waits', async () => { + query.mockReturnValueOnce(Promise.reject(new Error('offline'))); + const list = await make(); + query.mockReturnValue(Promise.resolve(pageOf(['lobby'], 1))); + + list.search.set('lobby'); + list.retry(); + await flush(); + + expect(query).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(500); + await flush(); + + expect(query).toHaveBeenLastCalledWith('lobby'); + expect(list.items().map((_) => _.id)).toEqual(['lobby']); + }); + it('stays empty when the query is not allowed', async () => { query.mockReturnValue(null); const list = await make(); diff --git a/apps/signage-manager/src/tests/signage-inventory.service.spec.ts b/apps/signage-manager/src/tests/signage-inventory.service.spec.ts index 06be3ca3837..f03a6c61f63 100644 --- a/apps/signage-manager/src/tests/signage-inventory.service.spec.ts +++ b/apps/signage-manager/src/tests/signage-inventory.service.spec.ts @@ -50,7 +50,7 @@ describe('SignageInventoryService', () => { } // The schedules timeline shows its online status from these displays - it('keeps a display that never checked in as never seen', async () => { + it('keeps a display that never checked in as never seen, its name decoded once', async () => { const service = createService(); vi.spyOn( TestBed.inject(SignageContextService), @@ -65,7 +65,8 @@ describe('SignageInventoryService', () => { fn: (raw: Partial) => PlaceSystem; }) => Promise.resolve({ - data: [params.fn({ id: 'd1', name: 'New' })], + // A saved name of `R&D`, encoded once by the backend + data: [params.fn({ id: 'd1', name: 'R&D' })], total: 1, next: null, })) as unknown as typeof query); @@ -74,6 +75,7 @@ describe('SignageInventoryService', () => { expect(displays.map(({ id }) => id)).toEqual(['d1']); expect(displays[0].signage_last_seen).toBe(0); + expect(displays[0].name).toBe('R&D'); }); describe('content report', () => {