From 6a865d6c69f8a581186582b9f19c937025df5e5c Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 6 Sep 2026 13:39:08 -0600 Subject: [PATCH] fix(sidenav): show the skeleton, not "No Chats Yet", while sessions load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isLoading()` tested `sessionsResource.value() === undefined`, which stopped being true during the cold-start fetch. The loader short-circuits to `null` when `sessionsRequest` is false, so the resource *resolves* before any session is fetched. That path is now the ordinary one: `SessionService`'s constructor only enables loading if the BFF session is already authenticated, but the service is constructed during the APP_INITIALIZER pass — `AnnouncementModalService` -> `MessageMapService` -> `SessionService` — and Angular's `runInitializers` invokes every initializer synchronously before awaiting any of them, so `bootstrap()` is still in flight and `isAuthenticated()` is false. The auth effect enables loading afterwards, but `reload()` preserves the previous value, so `null` survives the whole real fetch. `isLoading()` read false, `groupedSessions()` was empty, and the sidebar rendered the empty state until the response landed. `isLoading()` now means "nothing to draw yet": no API response (`undefined` before the first load resolves, or the short-circuit `null`) and no locally cached rows. An empty `sessions` array is a real response and still falls through to the empty state. `error()` is checked first — reading `value()` on an errored resource throws, which the old ordering walked into. Deliberately not keyed on `status() === 'reloading'`: `refreshSessions()` reloads after send/rename/mark-unread, and that would flash the list to a skeleton every time. Co-Authored-By: Claude Opus 5 --- .../session-list/session-list.spec.ts | 49 ++++++++++++++++++- .../components/session-list/session-list.ts | 27 ++++++++-- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts index f9db71f7f..ce1dcb963 100644 --- a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts +++ b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts @@ -31,7 +31,7 @@ describe('SessionList', () => { mergedSessionsResource: signal({ sessions: [mockSession], nextToken: null }), currentSession: signal(mockSession), deleteSession: vi.fn().mockResolvedValue(undefined), - sessionsResource: { value: vi.fn().mockReturnValue(null), error: vi.fn().mockReturnValue(null), isPending: vi.fn().mockReturnValue(false) }, + sessionsResource: { value: vi.fn().mockReturnValue({ sessions: [mockSession], nextToken: null }), error: vi.fn().mockReturnValue(null), isPending: vi.fn().mockReturnValue(false) }, isLocallyRead: vi.fn().mockReturnValue(false), markSessionRead: vi.fn().mockResolvedValue(undefined), markSessionUnread: vi.fn().mockResolvedValue(undefined), @@ -62,6 +62,53 @@ describe('SessionList', () => { return TestBed.runInInjectionContext(() => new SessionList()); } + describe('isLoading', () => { + it('stays loading while the resource has not produced a response', async () => { + // Cold start: the loader short-circuits to `null` because sessions + // loading is not enabled until the BFF bootstrap resolves — and + // `reload()` keeps that `null` for the whole real fetch. With nothing + // cached to draw, that must read as loading (skeleton), never as + // "no conversations". + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.value.mockReturnValue(null); + const component = await createComponent(); + + expect(component.isLoading()).toBe(true); + + // Before the first load resolves at all. + mockSessionService.sessionsResource.value.mockReturnValue(undefined); + expect(component.isLoading()).toBe(true); + }); + + it('is not loading once the API answers, even with zero sessions', async () => { + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.value.mockReturnValue({ sessions: [], nextToken: null }); + const component = await createComponent(); + + // A real empty response is the empty state, not a skeleton. + expect(component.isLoading()).toBe(false); + }); + + it('renders locally cached sessions instead of a skeleton', async () => { + mockSessionService.sessionsResource.value.mockReturnValue(null); + const component = await createComponent(); + + expect(component.isLoading()).toBe(false); + }); + + it('defers to the error state without reading the resource value', async () => { + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.error.mockReturnValue(new Error('boom')); + // Angular's resource throws from `value()` when the load errored. + mockSessionService.sessionsResource.value.mockImplementation(() => { + throw new Error('should not be read'); + }); + const component = await createComponent(); + + expect(component.isLoading()).toBe(false); + }); + }); + it('should compute sessions from merged resource', async () => { const component = await createComponent(); expect(component.sessions()).toEqual([mockSession]); diff --git a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts index ba98be10f..0f4a7d8d9 100644 --- a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts +++ b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts @@ -132,11 +132,32 @@ export class SessionList { }); /** - * Computed signal for loading state. + * Computed signal for loading state — i.e. "we have nothing to draw yet". + * + * `value()` alone is not enough. The loader short-circuits to `null` while + * `sessionsRequest` is still false, and that is the ordinary cold-start path: + * `SessionService` is constructed during the APP_INITIALIZER pass (via + * `AnnouncementModalService` -> `MessageMapService`), and Angular runs every + * initializer synchronously before awaiting any of them — so the BFF + * `bootstrap()` promise is still in flight and `isAuthenticated()` is false. + * The eager-fetch branch in the constructor is skipped, the resource resolves + * `null`, and the auth effect only enables loading afterwards. `reload()` + * keeps the previous value, so `null` survives the entire real fetch: testing + * `=== undefined` reported "loaded, no sessions" and the sidebar rendered the + * "No Chats Yet" empty state instead of the skeleton. + * + * So: no API response yet (`undefined` before the first load resolves, `null` + * while it is short-circuited) means loading. An empty `sessions` array is a + * real response and must fall through to the empty state. + * + * `error()` is read first — reading `value()` on an errored resource throws. */ readonly isLoading = computed(() => { - const value = this.sessionsResource.value(); - return value === undefined; + if (this.sessionsResource.error()) return false; + if (this.sessionsResource.value() != null) return false; + // Locally created sessions can exist before the API answers; draw those + // rather than covering them with a skeleton. + return this.groupedSessions().length === 0; }); /**