From f69f12f8dddd4c53b7aa9784b5695d9f4884ed83 Mon Sep 17 00:00:00 2001 From: Cecilia Krum Date: Sun, 2 Aug 2026 07:48:27 -0500 Subject: [PATCH 1/4] PER-10580: First thumbnail appears without refresh This only works for the first thumbnail of an uploaded set. --- .../file-list-item.component.spec.ts | 36 +++++++++++++++++++ .../file-list-item.component.ts | 25 ++++++++++--- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts index 4140b020b..42a1bd4a4 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts @@ -333,6 +333,27 @@ describe('FileListItemComponent', () => { (router.routerState.snapshot as any).url = '/'; }); + it('should keep the same random preview thumbnail across reads', async () => { + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + Promise.resolve(false), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + const firstRead = component.recordThumbnailUrl; + + expect(component.recordThumbnailUrl).toBe(firstRead); + expect(component.recordThumbnailUrl).toBe(firstRead); + + (router.routerState.snapshot as any).url = '/'; + }); + it('should always set real thumbnail URL on init', async () => { component.item.isRecord = true; component.item.type = 'type.record.image'; @@ -343,6 +364,21 @@ describe('FileListItemComponent', () => { expect(component.recordThumbnailUrl).toBe('https://example.com/thumb.jpg'); }); + it('should pick up a thumbnail added to the item after init', async () => { + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBeUndefined(); + + // The thumbnail refresh poll in DataService mutates the existing item + // rather than replacing it. + component.item.thumbnail256 = 'https://example.com/256'; + + expect(component.recordThumbnailUrl).toBe('https://example.com/256'); + }); + it('should display displayTime instead of displayDT when displayTime is set', () => { component.item.displayTime = '2020-06-10'; component.item.displayDT = '2023-01-01T00:00:00.000Z'; diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.ts index d67939338..25fff440a 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.ts @@ -205,11 +205,26 @@ export class FileListItemComponent public isZip = false; public date: string = ''; public isUnlistedShare = false; - public recordThumbnailUrl: string | undefined; private folderThumb: string; private folderContentsType: FolderContentsType = FolderContentsType.NORMAL; + private isSharePreviewRoute = false; + private previewImageOverride: string | undefined; + private previewImageResolved = false; + + // Read live rather than snapshotted in ngOnInit: the thumbnail refresh poll in + // DataService writes new thumbnail URLs onto the existing item, so a snapshot + // would leave newly uploaded files without a thumbnail until a manual refresh. + public get recordThumbnailUrl(): string | undefined { + // Don't leak a real thumbnail before we know whether this share is unlisted. + if (this.isSharePreviewRoute && !this.previewImageResolved) { + return undefined; + } + + return this.previewImageOverride ?? GetThumbnail(this.item); + } + private getRandomPreviewImage(): string { const previewCount = 10; const randomIndex = Math.floor(Math.random() * previewCount) + 1; @@ -259,7 +274,8 @@ export class FileListItemComponent } async ngOnInit() { - this.recordThumbnailUrl = GetThumbnail(this.item); + this.isSharePreviewRoute = + this.router.routerState.snapshot.url.includes('/share/'); const date = new Date(this.startDisplayTime); this.date = getFormattedDate(date); @@ -275,10 +291,11 @@ export class FileListItemComponent this.isPublicArchive = true; } - if (this.router.routerState.snapshot.url.includes('/share/')) { + if (this.isSharePreviewRoute) { if (!this.isUnlistedShare) { - this.recordThumbnailUrl = this.getRandomPreviewImage(); + this.previewImageOverride = this.getRandomPreviewImage(); } + this.previewImageResolved = true; this.allowActions = false; this.isInSharePreview = true; } From 7f69fba58209f673478b93ba011b3d83cafebdf8 Mon Sep 17 00:00:00 2001 From: Cecilia Krum Date: Sun, 2 Aug 2026 08:04:14 -0500 Subject: [PATCH 2/4] PER-10580: Multiple thumbnails appear without refresh This is a more complex fix that should get in-depth review, since it changes the data service and I am not confident in it. Claude suggests that in the switch to the stela "children" endpoint we missed a couple points. We weren't correctly tracking the responses because of the string/number id mismatch, so there are some workarounds for that in here. --- .../file-list-item.component.spec.ts | 63 +++++++ .../shared/services/data/data.service.spec.ts | 160 ++++++++++++++++++ src/app/shared/services/data/data.service.ts | 92 ++++++++-- 3 files changed, 300 insertions(+), 15 deletions(-) diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts index 42a1bd4a4..dad79cd4f 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts @@ -348,12 +348,75 @@ describe('FileListItemComponent', () => { const firstRead = component.recordThumbnailUrl; + expect(firstRead).toMatch(/^assets\/img\/preview\/preview-\d+\.jpg$/); expect(component.recordThumbnailUrl).toBe(firstRead); expect(component.recordThumbnailUrl).toBe(firstRead); (router.routerState.snapshot as any).url = '/'; }); + it('should not expose the real thumbnail until the share type is known', async () => { + // detectChanges() in beforeEach left an ngOnInit awaiting isUnlistedShare(). + // Drain it before changing the route, or its tail resumes against the route + // this test sets and overwrites the preview state with its own stale answer. + await new Promise((resolve) => { + setTimeout(resolve); + }); + + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + let resolveIsUnlistedShare: (isUnlisted: boolean) => void = () => {}; + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + new Promise((resolve) => { + resolveIsUnlistedShare = resolve; + }), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + + // Deliberately not awaited: a listed share must not show the real + // thumbnail in the window before isUnlistedShare() settles. + const init = component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBeUndefined(); + + resolveIsUnlistedShare(false); + await init; + + expect(component.recordThumbnailUrl).toMatch( + /^assets\/img\/preview\/preview-\d+\.jpg$/, + ); + + (router.routerState.snapshot as any).url = '/'; + }); + + it('should show the real thumbnail on an unlisted share', async () => { + // See above: let the ngOnInit started by beforeEach settle first. + await new Promise((resolve) => { + setTimeout(resolve); + }); + + const router = TestBed.inject(Router); + (router.routerState.snapshot as any).url = '/share/test'; + + const shareLinksService = TestBed.inject(ShareLinksService); + spyOn(shareLinksService, 'isUnlistedShare').and.returnValue( + Promise.resolve(true), + ); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + + await component.ngOnInit(); + + expect(component.recordThumbnailUrl).toBe('https://example.com/thumb.jpg'); + + (router.routerState.snapshot as any).url = '/'; + }); + it('should always set real thumbnail URL on init', async () => { component.item.isRecord = true; component.item.type = 'type.record.image'; diff --git a/src/app/shared/services/data/data.service.spec.ts b/src/app/shared/services/data/data.service.spec.ts index f62bc9afa..33479bd43 100644 --- a/src/app/shared/services/data/data.service.spec.ts +++ b/src/app/shared/services/data/data.service.spec.ts @@ -321,6 +321,166 @@ describe('DataService', () => { .catch(done.fail); }); + it('should update every child when the response is larger than the request', async () => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const allChildren = currentFolder.ChildItemVOs as (RecordVO | FolderVO)[]; + allChildren.forEach((item) => { + service.registerItem(item); + }); + + // Ask about two items but answer with every child of the folder. This is + // what stela's children endpoint does regardless of what was requested, so + // the response cannot be matched to the request by position. + const requested = allChildren.slice(0, 2); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: allChildren.map((item) => ({ + folder_linkId: item.folder_linkId, + archiveNbr: item.archiveNbr, + parentFolderId: currentFolder.folderId, + thumbURL500: `https://example.com/${item.folder_linkId}`, + })), + }), + } as unknown as FolderResponse), + ); + + const count = await service.fetchLeanItems(requested); + + expect(count).toBe(allChildren.length); + allChildren.forEach((item) => { + expect(item.dataStatus).toEqual(DataStatus.Lean); + expect(item.isFetching).toBeFalse(); + expect(item.thumbURL500).toBe( + `https://example.com/${item.folder_linkId}`, + ); + }); + }); + + it('should settle a requested item that is absent from the response', async () => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + // The record is no longer a child of the folder: it was moved or deleted + // between the fetch being issued and the response arriving. + const getWithChildren = spyOn( + api.folder, + 'getWithChildren', + ).and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ ChildItemVOs: [] }), + } as unknown as FolderResponse), + ); + + const inFlight = service.fetchLeanItems([record]); + const fetched = record.fetched; + await inFlight; + + await expectAsync(fetched).toBeRejected(); + + expect(record.isFetching).toBeFalse(); + expect(record.fetched).toBeNull(); + + // A stuck isFetching flag would filter the record out of every later + // fetch, leaving it permanently stale. + getWithChildren.calls.reset(); + await service.fetchLeanItems([record]); + + expect(getWithChildren).toHaveBeenCalled(); + }); + + it('should add a lean item to thumbRefreshQueue when the ids differ in type', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + // The folder came from the PHP API, so its folderId is a number, while + // stela reports the record's parentFolderId as a string. + expect(typeof currentFolder.folderId).toBe('number'); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: String(currentFolder.folderId), + }, + ], + }), + } as unknown as FolderResponse), + ); + + service + .fetchLeanItems([record]) + .then(() => { + expect(service.getThumbRefreshQueue()).toContain(record); + done(); + }) + .catch(done.fail); + }); + + it('should not add a lean item to thumbRefreshQueue when it belongs to another folder', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: `${currentFolder.folderId}0`, + }, + ], + }), + } as unknown as FolderResponse), + ); + + service + .fetchLeanItems([record]) + .then(() => { + expect(service.getThumbRefreshQueue()).not.toContain(record); + done(); + }) + .catch(done.fail); + }); + it('should add a lean item to thumbRefreshQueue when no thumbnail size is present', (done) => { const service = TestBed.inject(DataService); const api = TestBed.inject(ApiService); diff --git a/src/app/shared/services/data/data.service.ts b/src/app/shared/services/data/data.service.ts index 4a7ba8171..dfa5e046d 100644 --- a/src/app/shared/services/data/data.service.ts +++ b/src/app/shared/services/data/data.service.ts @@ -1,6 +1,6 @@ import { Injectable, EventEmitter } from '@angular/core'; import { map } from 'rxjs/operators'; -import { remove, find, findIndex } from 'lodash'; +import { remove, find, findIndex, noop } from 'lodash'; import { ApiService } from '@shared/services/api/api.service'; import { @@ -23,6 +23,21 @@ import { TagsService } from '@core/services/tags/tags.service'; const THUMBNAIL_REFRESH_INTERVAL = 3000; +// Identifiers reach us as numbers from the PHP API and as strings from stela, and +// a single item can carry both over its lifetime: a record loaded via navigateLean +// has a numeric parentFolderId until update() overwrites it with stela's string. +// Compare them as strings so the source of the id does not change the answer. +// Null and undefined never match, including each other. +type ItemId = string | number | null | undefined; + +const isSameId = (a: ItemId, b: ItemId): boolean => { + if (a === null || a === undefined || b === null || b === undefined) { + return false; + } + + return String(a) === String(b); +}; + export type SelectedItemsSet = Set; export interface SelectKeyEvent { @@ -190,8 +205,22 @@ export class DataService { ): Promise { this.debug('fetchLeanItems %d items requested', items.length); - const itemResolves = []; - const itemRejects = []; + // Keyed by folder_linkId rather than by position: getWithChildren returns + // every child of the folder, not just the ones we asked about, so the + // response order tells us nothing about which resolver belongs to which + // item. Keys are stringified because folder_linkId reaches us as a number + // from stela and as a string from some PHP responses. The item reference is + // held here rather than looked up in byFolderLinkId on settle, so that an + // item whose row was destroyed (and therefore unregistered) mid-request + // still gets its isFetching flag cleared. + const itemResolvers = new Map< + string, + { + item: ItemVO; + resolve: (value: boolean) => void; + reject: () => void; + } + >(); let handleItemRegistration = false; if (currentFolder) { @@ -213,10 +242,18 @@ export class DataService { } item.isFetching = true; - item.fetched = new Promise((resolve, reject) => { - itemResolves.push(resolve); - itemRejects.push(reject); + item.fetched = new Promise((resolve, reject) => { + itemResolvers.set(String(item.folder_linkId), { + item, + resolve, + reject, + }); }); + // Not every caller awaits `fetched`, and the thumbnail refresh poll + // never does. Swallow the unawaited case so settling a missing item + // does not surface as an unhandled promise rejection. Callers that + // do chain onto `fetched` still see the rejection. + item.fetched.catch(noop); return true; }) .map((item) => ({ @@ -240,7 +277,7 @@ export class DataService { return fetchedFolder.ChildItemVOs; }) .then(async (leanItems) => { - leanItems.forEach((leanItem, index) => { + leanItems.forEach((leanItem) => { const item = this.byFolderLinkId[leanItem.folder_linkId]; if (item) { this.byArchiveNbr[leanItem.archiveNbr] = item; @@ -248,13 +285,18 @@ export class DataService { item.dataStatus = DataStatus.Lean; item.isFetching = false; - itemResolves[index](); + + const resolver = itemResolvers.get(String(leanItem.folder_linkId)); + if (resolver) { + itemResolvers.delete(String(leanItem.folder_linkId)); + resolver.resolve(true); + } item.fetched = null; if ( !item.isFolder && !GetThumbnail(item) && - item.parentFolderId === this.currentFolder.folderId + isSameId(item.parentFolderId, this.currentFolder.folderId) ) { this.debug('thumbRefreshQueue push %s', item.archiveNbr); this.thumbRefreshQueue.push(item); @@ -262,6 +304,11 @@ export class DataService { } }); + // Anything we asked for but did not get back still has to be settled. + // A stuck isFetching flag excludes an item from every future + // fetchLeanItems call, so it would never refresh again. + this.settleUnresolvedItems(itemResolvers); + if (handleItemRegistration) { items.forEach((item) => { this.unregisterItem(item); @@ -272,16 +319,31 @@ export class DataService { return await Promise.resolve(leanItems.length); }) - .catch((response) => { - itemRejects.forEach((reject, index) => { - items[index].isFetching = false; - items[index].fetched = null; - reject(); - }); + .catch(() => { + this.settleUnresolvedItems(itemResolvers); return 0; }); } + private settleUnresolvedItems( + itemResolvers: Map< + string, + { + item: ItemVO; + resolve: (value: boolean) => void; + reject: () => void; + } + >, + ) { + for (const { item, reject } of itemResolvers.values()) { + item.isFetching = false; + item.fetched = null; + reject(); + } + + itemResolvers.clear(); + } + public async fetchFullItems(items: Array, withChildren?: boolean) { this.debug('fetchFullItems %d items requested', items.length); From 00fb116c0db49ad4585c13e940ce1ef9ae56d203 Mon Sep 17 00:00:00 2001 From: aasandei-vsp Date: Tue, 4 Aug 2026 14:19:54 +0300 Subject: [PATCH 3/4] Add a subscription for populating the thumbnail after it is retrieved from BE After upload, the thumbnail is not immediatelly generated, so we keep calling the BE until it is available. So the list item will subscribe to this refresh and populate the thumbnail whenever is available. One important thing to mention is that thumbnails should not be available for restricted shares, so making sure that we only show it after we check if the share is restricted or not is vital, that's why the isUnlistedShare variable is needed. Issue: PER-10580 --- .../file-list-item.component.spec.ts | 63 +++++++++++++-- .../file-list-item.component.ts | 55 +++++++------ .../shared/services/data/data.service.spec.ts | 81 +++++++++++++++++++ src/app/shared/services/data/data.service.ts | 20 ++++- 4 files changed, 187 insertions(+), 32 deletions(-) diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts index dad79cd4f..87c3e6ec4 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.spec.ts @@ -1,6 +1,6 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ElementRef, Pipe, PipeTransform } from '@angular/core'; -import { of } from 'rxjs'; +import { Subject, of } from 'rxjs'; import { ActivatedRoute, Router, provideRouter } from '@angular/router'; import { DataService } from '@shared/services/data/data.service'; @@ -41,6 +41,7 @@ describe('FileListItemComponent', () => { let component: FileListItemComponent; let fixture: ComponentFixture; let editService: EditService; + let thumbnailUpdatedSubject: Subject; const activatedRouteMock = { snapshot: { @@ -67,6 +68,8 @@ describe('FileListItemComponent', () => { }; beforeEach(async () => { + thumbnailUpdatedSubject = new Subject(); + await TestBed.configureTestingModule({ imports: [MockItemTypeIconPipe, MockPrDatePipe, MockPrConstantsPipe], declarations: [FileListItemComponent, GetThumbnailPipe], @@ -84,6 +87,7 @@ describe('FileListItemComponent', () => { beginPreparingForNavigate: jasmine.createSpy(), fetchLeanItems: jasmine.createSpy(), setItemMultiSelectStatus: jasmine.createSpy(), + thumbnailUpdated$: () => thumbnailUpdatedSubject.asObservable(), currentFolder: { type: '' }, }, }, @@ -333,7 +337,15 @@ describe('FileListItemComponent', () => { (router.routerState.snapshot as any).url = '/'; }); - it('should keep the same random preview thumbnail across reads', async () => { + it('should not replace the stock preview when a thumbnail arrives later', async () => { + // detectChanges() in beforeEach left an ngOnInit awaiting isUnlistedShare(). + // Drain it and tear the component down so this test starts from a single + // subscription, the way a real component instance does. + await new Promise((resolve) => { + setTimeout(resolve); + }); + component.ngOnDestroy(); + const router = TestBed.inject(Router); (router.routerState.snapshot as any).url = '/share/test'; @@ -346,11 +358,14 @@ describe('FileListItemComponent', () => { await component.ngOnInit(); - const firstRead = component.recordThumbnailUrl; + const stockPreview = component.recordThumbnailUrl; + + expect(stockPreview).toMatch(/^assets\/img\/preview\/preview-\d+\.jpg$/); + + component.item.thumbURL200 = 'https://example.com/thumb.jpg'; + thumbnailUpdatedSubject.next(component.item); - expect(firstRead).toMatch(/^assets\/img\/preview\/preview-\d+\.jpg$/); - expect(component.recordThumbnailUrl).toBe(firstRead); - expect(component.recordThumbnailUrl).toBe(firstRead); + expect(component.recordThumbnailUrl).toBe(stockPreview); (router.routerState.snapshot as any).url = '/'; }); @@ -428,6 +443,7 @@ describe('FileListItemComponent', () => { }); it('should pick up a thumbnail added to the item after init', async () => { + component.ngOnDestroy(); component.item.isRecord = true; component.item.type = 'type.record.image'; @@ -436,12 +452,45 @@ describe('FileListItemComponent', () => { expect(component.recordThumbnailUrl).toBeUndefined(); // The thumbnail refresh poll in DataService mutates the existing item - // rather than replacing it. + // rather than replacing it, then announces the item it wrote to. component.item.thumbnail256 = 'https://example.com/256'; + thumbnailUpdatedSubject.next(component.item); expect(component.recordThumbnailUrl).toBe('https://example.com/256'); }); + it('should ignore a thumbnail update for a different item', async () => { + component.ngOnDestroy(); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + + component.item.thumbnail256 = 'https://example.com/256'; + // Same folder_linkId, different instance: only the item this row renders + // counts, so the update belongs to some other row. + thumbnailUpdatedSubject.next({ + folder_linkId: component.item.folder_linkId, + thumbnail256: 'https://example.com/other', + }); + + expect(component.recordThumbnailUrl).toBeUndefined(); + }); + + it('should stop applying thumbnail updates once destroyed', async () => { + component.ngOnDestroy(); + component.item.isRecord = true; + component.item.type = 'type.record.image'; + + await component.ngOnInit(); + component.ngOnDestroy(); + + component.item.thumbnail256 = 'https://example.com/256'; + thumbnailUpdatedSubject.next(component.item); + + expect(component.recordThumbnailUrl).toBeUndefined(); + }); + it('should display displayTime instead of displayDT when displayTime is set', () => { component.item.displayTime = '2020-06-10'; component.item.displayDT = '2023-01-01T00:00:00.000Z'; diff --git a/src/app/file-browser/components/file-list-item/file-list-item.component.ts b/src/app/file-browser/components/file-list-item/file-list-item.component.ts index 25fff440a..0b36a9c01 100644 --- a/src/app/file-browser/components/file-list-item/file-list-item.component.ts +++ b/src/app/file-browser/components/file-list-item/file-list-item.component.ts @@ -62,6 +62,7 @@ import { unsubscribeAll, } from '@shared/utilities/hasSubscriptions'; import { Subscription } from 'rxjs'; +import { filter } from 'rxjs/operators'; import { ngIfFadeInAnimation } from '@shared/animations'; import { RouteData } from '@root/app/app.routes'; @@ -205,26 +206,11 @@ export class FileListItemComponent public isZip = false; public date: string = ''; public isUnlistedShare = false; + public recordThumbnailUrl: string | undefined; private folderThumb: string; private folderContentsType: FolderContentsType = FolderContentsType.NORMAL; - private isSharePreviewRoute = false; - private previewImageOverride: string | undefined; - private previewImageResolved = false; - - // Read live rather than snapshotted in ngOnInit: the thumbnail refresh poll in - // DataService writes new thumbnail URLs onto the existing item, so a snapshot - // would leave newly uploaded files without a thumbnail until a manual refresh. - public get recordThumbnailUrl(): string | undefined { - // Don't leak a real thumbnail before we know whether this share is unlisted. - if (this.isSharePreviewRoute && !this.previewImageResolved) { - return undefined; - } - - return this.previewImageOverride ?? GetThumbnail(this.item); - } - private getRandomPreviewImage(): string { const previewCount = 10; const randomIndex = Math.floor(Math.random() * previewCount) + 1; @@ -274,12 +260,15 @@ export class FileListItemComponent } async ngOnInit() { - this.isSharePreviewRoute = + this.isInSharePreview = this.router.routerState.snapshot.url.includes('/share/'); const date = new Date(this.startDisplayTime); this.date = getFormattedDate(date); - this.isUnlistedShare = await this.shareLinksService.isUnlistedShare(); + const isUnlistedShare = await this.shareLinksService.isUnlistedShare(); + this.isUnlistedShare = isUnlistedShare; + + this.initializeThumbnail(isUnlistedShare); this.dataService.registerItem(this.item); if (this.item.type.includes('app')) { @@ -291,13 +280,8 @@ export class FileListItemComponent this.isPublicArchive = true; } - if (this.isSharePreviewRoute) { - if (!this.isUnlistedShare) { - this.previewImageOverride = this.getRandomPreviewImage(); - } - this.previewImageResolved = true; + if (this.isInSharePreview) { this.allowActions = false; - this.isInSharePreview = true; } if (this.router.routerState.snapshot.url.includes('/apps')) { @@ -1069,6 +1053,29 @@ export class FileListItemComponent }); } + // A listed share preview must never show the real content, only a stock image. + // The answer arrives asynchronously, so it is taken as an argument: the + // thumbnail cannot be resolved before the share type is known. + private initializeThumbnail(isUnlistedShare: boolean): void { + if (this.isInSharePreview && !isUnlistedShare) { + this.recordThumbnailUrl = this.getRandomPreviewImage(); + return; + } + + this.recordThumbnailUrl = GetThumbnail(this.item); + + // The thumbnail refresh poll in DataService writes new URLs onto this same + // item instance, which no binding can observe on its own. + this.subscriptions.push( + this.dataService + .thumbnailUpdated$() + .pipe(filter((updatedItem) => updatedItem === this.item)) + .subscribe(() => { + this.recordThumbnailUrl = GetThumbnail(this.item); + }), + ); + } + private getFolderThumbnail(): void { if (!this.showFolderThumbnails) { this.folderContentsType = FolderContentsType.BROKEN_THUMBNAILS; diff --git a/src/app/shared/services/data/data.service.spec.ts b/src/app/shared/services/data/data.service.spec.ts index 33479bd43..20991ddaa 100644 --- a/src/app/shared/services/data/data.service.spec.ts +++ b/src/app/shared/services/data/data.service.spec.ts @@ -321,6 +321,87 @@ describe('DataService', () => { .catch(done.fail); }); + it('should announce the refreshed item when a thumbnail arrives', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: currentFolder.folderId, + thumbURL500: 'https://example.com/500', + }, + ], + }), + } as unknown as FolderResponse), + ); + + // The emitted reference is what lets a consumer tell its own item apart + // from every other row's. + service.thumbnailUpdated$().subscribe((updatedItem) => { + expect(updatedItem).toBe(record); + expect((updatedItem as RecordVO).thumbURL500).toBe( + 'https://example.com/500', + ); + done(); + }); + + service.fetchLeanItems([record]).catch(done.fail); + }); + + it('should not announce a refreshed item that still has no thumbnail', (done) => { + const service = TestBed.inject(DataService); + const api = TestBed.inject(ApiService); + const navigateResponse = new FolderResponse(navigateMinData); + const currentFolder = navigateResponse.getFolderVO(true) as FolderVO; + service.setCurrentFolder(currentFolder); + + const record = currentFolder.ChildItemVOs.find( + (item) => item.isRecord, + ) as RecordVO; + service.registerItem(record); + + spyOn(api.folder, 'getWithChildren').and.returnValue( + Promise.resolve({ + isSuccessful: true, + getFolderVO: () => ({ + ChildItemVOs: [ + { + folder_linkId: record.folder_linkId, + archiveNbr: record.archiveNbr, + parentFolderId: currentFolder.folderId, + }, + ], + }), + } as unknown as FolderResponse), + ); + + const thumbnailUpdated = jasmine.createSpy(); + service.thumbnailUpdated$().subscribe(thumbnailUpdated); + + service + .fetchLeanItems([record]) + .then(() => { + expect(thumbnailUpdated).not.toHaveBeenCalled(); + expect(service.getThumbRefreshQueue()).toContain(record); + done(); + }) + .catch(done.fail); + }); + it('should update every child when the response is larger than the request', async () => { const service = TestBed.inject(DataService); const api = TestBed.inject(ApiService); diff --git a/src/app/shared/services/data/data.service.ts b/src/app/shared/services/data/data.service.ts index dfa5e046d..15c855512 100644 --- a/src/app/shared/services/data/data.service.ts +++ b/src/app/shared/services/data/data.service.ts @@ -93,6 +93,14 @@ export class DataService { private unsharedItemSubject = new Subject(); + // Emits the item whose thumbnail URLs were just written by a lean fetch. + // Subscribers match on object identity: this service mutates the very same + // ItemVO instances its consumers render, so the emitted reference is the only + // unambiguous way to tell which consumer the update belongs to. Matching on + // folder_linkId would not be, since that id arrives as a number from stela + // and as a string from some PHP responses. + private thumbnailUpdatedSubject = new Subject(); + private eventSubject: Subject = new Subject(); public events: Observable = this.eventSubject.asObservable(); @@ -293,9 +301,15 @@ export class DataService { } item.fetched = null; + const thumbnailUrl = GetThumbnail(item); + + if (thumbnailUrl) { + this.thumbnailUpdatedSubject.next(item); + } + if ( !item.isFolder && - !GetThumbnail(item) && + !thumbnailUrl && isSameId(item.parentFolderId, this.currentFolder.folderId) ) { this.debug('thumbRefreshQueue push %s', item.archiveNbr); @@ -757,6 +771,10 @@ export class DataService { return this.unsharedItemSubject.asObservable(); } + public thumbnailUpdated$(): Observable { + return this.thumbnailUpdatedSubject.asObservable(); + } + public itemUnshared(item: ItemVO) { this.clearSelectedItems(); this.unsharedItemSubject.next(item); From 51aa47bf9faf89bf6686523639b2fd6e424bd360 Mon Sep 17 00:00:00 2001 From: aasandei-vsp Date: Tue, 4 Aug 2026 17:20:31 +0300 Subject: [PATCH 4/4] Update thumbnails in folder picker and profile edit when they are available This fix just bypasses a big problem we are having accross the app. We are mutating objects in place instead of replacing them, so even if the reference is the same, the object has changed. This makes any object highly unreliable for using it inside an Angular context. In this situation, we were using a pipe for rendering the thumbnails, which is the correct approach. The issue is the pipe would update when the object reference changes, which never happens in our case, even though the thumbnailUrls do. Issue: PER-10580 --- .../folder-picker.component.html | 4 +- .../folder-picker.component.spec.ts | 50 ++++++++++++++++++- .../folder-picker/folder-picker.component.ts | 10 ++++ .../profile-edit/profile-edit.component.html | 2 +- .../profile-edit.component.spec.ts | 14 +++++- .../profile-edit/profile-edit.component.ts | 14 +++++- 6 files changed, 88 insertions(+), 6 deletions(-) diff --git a/src/app/core/components/folder-picker/folder-picker.component.html b/src/app/core/components/folder-picker/folder-picker.component.html index 13afb9ce8..8cf3e1335 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.html +++ b/src/app/core/components/folder-picker/folder-picker.component.html @@ -33,7 +33,7 @@ } @default { -
+
} }
{{ item.displayName }}
@@ -50,7 +50,7 @@ } @if (selectedRecord) {
-
+
}
diff --git a/src/app/core/components/folder-picker/folder-picker.component.spec.ts b/src/app/core/components/folder-picker/folder-picker.component.spec.ts index 93316a970..976b35c17 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.spec.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.spec.ts @@ -6,7 +6,9 @@ import { DataService } from '@shared/services/data/data.service'; import { ApiService } from '@shared/services/api/api.service'; import { FolderResponse } from '@shared/services/api/index.repo'; import { SharedModule } from '@shared/shared.module'; -import { FolderVO } from '@root/app/models'; +import { By } from '@angular/platform-browser'; +import { BgImageSrcDirective } from '@shared/directives/bg-image-src.directive'; +import { FolderVO, RecordVO } from '@root/app/models'; import { HttpTestingController } from '@angular/common/http/testing'; import { FolderPickerService } from '@core/services/folder-picker/folder-picker.service'; import { DataStatus } from '@models/data-status.enum'; @@ -80,4 +82,50 @@ describe('FolderPickerComponent', () => { ), ).toBeFalsy(); }); + + it('should read the thumbnail currently on the item', () => { + const record = new RecordVO({ folder_linkId: 2, archiveNbr: 'a-2' }); + + expect(component.getThumbnailUrl(record)).toBeUndefined(); + + record.thumbnail256 = 'https://example.com/256'; + + expect(component.getThumbnailUrl(record)).toBe('https://example.com/256'); + }); + + it('should show a thumbnail that arrives after the row is rendered', () => { + const record = new RecordVO({ + folder_linkId: 1, + archiveNbr: 'a-1', + displayName: 'photo.jpg', + }); + const folder = new FolderVO({ + folder_linkId: 9, + folderId: 9, + displayName: 'Photos', + type: 'type.folder.private.folder', + }); + folder.ChildItemVOs = [record]; + + component.allowRecords = true; + component.currentFolder = folder; + fixture.detectChanges(); + + const backgrounds = fixture.debugElement.queryAll( + By.directive(BgImageSrcDirective), + ); + + expect(backgrounds.length).toBe(1); + + const background = backgrounds[0].injector.get(BgImageSrcDirective); + + expect(background.bgSrc).toBeFalsy(); + + // loadCurrentFolderChildData() writes the URL onto this same instance, so + // the row has to notice a mutation that leaves the reference unchanged. + record.thumbURL200 = 'https://example.com/thumb.jpg'; + fixture.detectChanges(); + + expect(background.bgSrc).toBe('https://example.com/thumb.jpg'); + }); }); diff --git a/src/app/core/components/folder-picker/folder-picker.component.ts b/src/app/core/components/folder-picker/folder-picker.component.ts index 3451f3c45..067921a16 100644 --- a/src/app/core/components/folder-picker/folder-picker.component.ts +++ b/src/app/core/components/folder-picker/folder-picker.component.ts @@ -2,6 +2,7 @@ import { Component, OnDestroy } from '@angular/core'; import { remove } from 'lodash'; import { DataService } from '@shared/services/data/data.service'; import { FolderVO, ItemVO, RecordVO } from '@root/app/models/index'; +import { GetThumbnail } from '@models/get-thumbnail'; import { ApiService } from '@shared/services/api/api.service'; import { FolderResponse } from '@shared/services/api/index.repo'; import { FolderPickerService } from '@core/services/folder-picker/folder-picker.service'; @@ -113,6 +114,15 @@ export class FolderPickerComponent implements OnDestroy { this.selectedRecord = record; } + // Read from the item on every check rather than through the getThumbnail pipe. + // The lean fetch that follows setFolder() writes thumbnail URLs onto these same + // item instances, and a pure pipe never re-runs for a mutation that leaves the + // reference unchanged, so the picker showed no thumbnails until its rows were + // rebuilt by navigating away and back. + getThumbnailUrl(item: ItemVO): string | undefined { + return GetThumbnail(item); + } + async setFolder(folder: FolderVO) { this.waiting = true; try { diff --git a/src/app/core/components/profile-edit/profile-edit.component.html b/src/app/core/components/profile-edit/profile-edit.component.html index 0b4a645c1..9bb3be5ef 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.html +++ b/src/app/core/components/profile-edit/profile-edit.component.html @@ -136,7 +136,7 @@
diff --git a/src/app/core/components/profile-edit/profile-edit.component.spec.ts b/src/app/core/components/profile-edit/profile-edit.component.spec.ts index 692e2e20d..81118c31f 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.spec.ts +++ b/src/app/core/components/profile-edit/profile-edit.component.spec.ts @@ -9,7 +9,7 @@ import { PromptService } from '@shared/services/prompt/prompt.service'; import { EventService } from '@shared/services/event/event.service'; import { CookieService } from 'ngx-cookie-service'; import { DIALOG_DATA, DialogRef } from '@angular/cdk/dialog'; -import { FolderVO } from '@models/index'; +import { ArchiveVO, FolderVO } from '@models/index'; import { RecordVO } from '@models/record-vo'; import { FolderResponse } from '@shared/services/api/folder.repo'; import { GetThumbnailPipe } from '@shared/pipes/get-thumbnail.pipe'; @@ -143,6 +143,18 @@ describe('ProfileEditComponent', () => { expect(component.publicRoot.thumbURL2000).toBe('new2000'); }); + it('should show a profile photo written onto the archive after the picker closes', () => { + component.archive = new ArchiveVO({ archiveNbr: 'a-1' }); + + expect(component.profileThumbnail).toBeNull(); + + // promptForProfilePicture() updates this same instance rather than + // replacing it, so the binding has to re-read an unchanged reference. + component.archive.thumbURL200 = 'new200'; + + expect(component.profileThumbnail).toBe('new200'); + }); + it('should restore original thumbArchiveNbr when chooseBannerPicture throws FolderResponse', async () => { const originalValue = component.publicRoot.thumbArchiveNbr; diff --git a/src/app/core/components/profile-edit/profile-edit.component.ts b/src/app/core/components/profile-edit/profile-edit.component.ts index 4c83b082a..ac50e4e1a 100644 --- a/src/app/core/components/profile-edit/profile-edit.component.ts +++ b/src/app/core/components/profile-edit/profile-edit.component.ts @@ -23,7 +23,7 @@ import { collapseAnimation, ngIfScaleAnimationDynamic, } from '@shared/animations'; -import { GetBanner } from '@models/get-thumbnail'; +import { GetBanner, GetThumbnail } from '@models/get-thumbnail'; import debug from 'debug'; import { PromptService, @@ -77,6 +77,18 @@ export class ProfileEditComponent implements OnInit, AfterViewInit { return this.publicRoot?.thumbArchiveNbr ? GetBanner(this.publicRoot) : null; } + // Read through to the archive rather than piping it, the way bannerThumbnail + // reads through to publicRoot. Choosing a photo writes the new URLs onto this + // same ArchiveVO instance, and a pure pipe never re-runs for a mutation that + // leaves the reference unchanged. + get profileThumbnail(): string | null { + if (!this.archive) { + return null; + } + + return GetThumbnail(this.archive) ?? null; + } + private debug = debug('component:profileEdit'); constructor(