Repository navigation
fix(chat): announce the gateway session id on first turn activity #983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| import { act, render, screen } from "@testing-library/react"; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import type React from "react"; | ||
| import SidebarRecentSessions, { | ||
| needsForcedSessionSync, | ||
| } from "./SidebarRecentSessions"; | ||
|
|
||
| // The list only needs `t` for section headers and row labels. | ||
| vi.mock("../../components/useI18n", () => ({ | ||
| useI18n: () => ({ | ||
| t: (key: string): string => key, | ||
| }), | ||
| })); | ||
|
|
||
| interface CachedRow { | ||
| id: string; | ||
| title: string; | ||
| contextFolder: string | null; | ||
| } | ||
|
|
||
| function installHermesAPI(rows: CachedRow[] = []): { | ||
| listCachedSessions: ReturnType<typeof vi.fn>; | ||
| syncSessionCache: ReturnType<typeof vi.fn>; | ||
| } { | ||
| const api = { | ||
| listCachedSessions: vi.fn().mockResolvedValue(rows), | ||
| syncSessionCache: vi.fn().mockResolvedValue(rows), | ||
| }; | ||
| Object.defineProperty(window, "hermesAPI", { | ||
| configurable: true, | ||
| value: api, | ||
| }); | ||
| return api; | ||
| } | ||
|
|
||
| const baseProps = { | ||
| open: true, | ||
| connectionId: "connection-one", | ||
| activeProfile: "work", | ||
| resumingSessionId: null, | ||
| onSelect: (): void => {}, | ||
| scrollRootRef: { current: null } as React.RefObject<HTMLDivElement | null>, | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| localStorage.clear(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| describe("needsForcedSessionSync", () => { | ||
| it("holds only for a live run whose session has no loaded row", () => { | ||
| expect(needsForcedSessionSync("s1", [], new Set(["s1"]))).toBe(true); | ||
| expect( | ||
| needsForcedSessionSync("s1", [{ id: "s0" }], new Set(["s1"])), | ||
| ).toBe(true); | ||
| expect( | ||
| needsForcedSessionSync("s1", [{ id: "s1" }], new Set(["s1"])), | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it("does not hold for a resumed session that is simply past the loaded page", () => { | ||
| expect(needsForcedSessionSync("s1", [{ id: "s0" }], new Set())).toBe(false); | ||
| }); | ||
|
|
||
| it("does not hold without an open conversation", () => { | ||
| expect(needsForcedSessionSync(null, [], new Set(["s1"]))).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sidebar session cache sync (#980)", () => { | ||
| it("syncs past the throttle and lists a session a live run just created", async () => { | ||
| const api = installHermesAPI(); | ||
| let cached: CachedRow[] = []; | ||
| api.syncSessionCache.mockImplementation(() => Promise.resolve(cached)); | ||
|
|
||
| const { rerender } = render( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId={null} | ||
| loadingSessionIds={new Set()} | ||
| />, | ||
| ); | ||
| // Settle the opening cache read and sync, then let the throttle arm. | ||
| await act(async () => {}); | ||
| const atMount = api.syncSessionCache.mock.calls.length; | ||
| expect(atMount).toBeGreaterThan(0); | ||
| expect(screen.queryByText("Look at the capabilities")).toBeNull(); | ||
|
|
||
| // The gateway hands out the session id and the first user message is stored, | ||
| // so the forced sync finds a titled row. | ||
| cached = [ | ||
| { id: "session-new", title: "Look at the capabilities", contextFolder: null }, | ||
| ]; | ||
| rerender( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId="session-new" | ||
| loadingSessionIds={new Set(["session-new"])} | ||
| />, | ||
| ); | ||
|
|
||
| await vi.waitFor(() => | ||
| expect(api.syncSessionCache.mock.calls.length).toBe(atMount + 1), | ||
| ); | ||
| expect(await screen.findByText("Look at the capabilities")).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("keeps the throttle when switching to a loaded session", async () => { | ||
| const api = installHermesAPI([ | ||
| { id: "session-listed", title: "Listed", contextFolder: null }, | ||
| ]); | ||
|
|
||
| const { rerender } = render( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId={null} | ||
| loadingSessionIds={new Set()} | ||
| />, | ||
| ); | ||
| await act(async () => {}); | ||
| const atMount = api.syncSessionCache.mock.calls.length; | ||
| expect(atMount).toBeGreaterThan(0); | ||
|
|
||
| rerender( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId="session-listed" | ||
| loadingSessionIds={new Set()} | ||
| />, | ||
| ); | ||
|
|
||
| expect(api.syncSessionCache.mock.calls.length).toBe(atMount); | ||
| }); | ||
|
|
||
| it("keeps the throttle when resuming a session past the loaded page", async () => { | ||
| const api = installHermesAPI([ | ||
| { id: "session-first", title: "First", contextFolder: null }, | ||
| ]); | ||
|
|
||
| const { rerender } = render( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId={null} | ||
| loadingSessionIds={new Set()} | ||
| />, | ||
| ); | ||
| await act(async () => {}); | ||
| const atMount = api.syncSessionCache.mock.calls.length; | ||
| expect(atMount).toBeGreaterThan(0); | ||
|
|
||
| // An old conversation that is not in the loaded page: resuming it must not | ||
| // turn every switch into a full state.db read. | ||
| rerender( | ||
| <SidebarRecentSessions | ||
| {...baseProps} | ||
| currentSessionId="session-deep" | ||
| loadingSessionIds={new Set()} | ||
| />, | ||
| ); | ||
|
|
||
| expect(api.syncSessionCache.mock.calls.length).toBe(atMount); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,33 @@ interface RecentSession { | |
| // ChatGPT-style paged conversation list under the pinned app navigation. | ||
| export const RECENT_SESSIONS_PAGE_SIZE = 30; | ||
|
|
||
| // @lat: [[sidebar-navigation#Provisional fresh sessions#Live first-turn rows]] | ||
| /** | ||
| * True when the open conversation belongs to a run that is still generating | ||
| * and the loaded page has no row for it, so its refresh must skip the | ||
| * throttle. | ||
| * | ||
| * A run's session id is revealed the moment the agent produces its first | ||
| * visible token, reasoning, or tool call, so the sidebar can be showing a | ||
| * chat it cannot list yet. The refresh that would pick the row up is normally | ||
| * throttled, and the click that opened "New Chat" usually still sits inside | ||
| * that window, which is what kept a new conversation out of the list for the | ||
| * rest of its first run (issue #980). | ||
| * | ||
| * The live-run requirement is what keeps an older resumed conversation out of | ||
| * it: such a session is also missing from the loaded page, simply because it | ||
| * sits past the first rows, and forcing a full sync for every switch into one | ||
| * would defeat the throttle the cache read exists to respect. | ||
| */ | ||
| export function needsForcedSessionSync( | ||
| sessionId: string | null, | ||
| listed: ReadonlyArray<{ id: string }>, | ||
| liveSessionIds: ReadonlySet<string>, | ||
| ): boolean { | ||
| if (!sessionId || !liveSessionIds.has(sessionId)) return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If a first turn finishes before the renderer processes its announced session ID, the run is no longer in |
||
| return !listed.some((row) => row.id === sessionId); | ||
| } | ||
|
|
||
| // Re-sync cadence while the list is visible. Deliberately slower than the | ||
| // Sessions screen (30s) — the sidebar is always on screen, so this interval | ||
| // runs for the whole app lifetime when the section is expanded. | ||
|
|
@@ -318,6 +345,15 @@ const SidebarRecentSessions = memo(function SidebarRecentSessions({ | |
| [normalizeRows], | ||
| ); | ||
|
|
||
| // A conversation whose first run is still generating can be missing from the | ||
| // loaded page; that is the one case the refresh throttle below must not | ||
| // swallow. | ||
| const forceSessionSync = useMemo( | ||
| () => | ||
| needsForcedSessionSync(currentSessionId, sessions, loadingSessionIds), | ||
| [currentSessionId, loadingSessionIds, sessions], | ||
| ); | ||
|
|
||
| const refresh = useCallback( | ||
| async (force = false): Promise<void> => { | ||
| const now = Date.now(); | ||
|
|
@@ -450,9 +486,14 @@ const SidebarRecentSessions = memo(function SidebarRecentSessions({ | |
| // Resuming/switching sessions reorders recency — refresh (throttled). | ||
| // Also refreshes when going to "New Chat" (currentSessionId becomes null) | ||
| // so the just-left session appears in the list immediately. | ||
| // | ||
| // A live run with no loaded row skips the throttle: the click that opened | ||
| // "New Chat" is usually still inside the 5s window, so a throttled refresh | ||
| // here is dropped and the new row stays invisible until the 60s poll (issue | ||
| // #980). Every other session switch stays throttled. | ||
| useEffect(() => { | ||
| if (open) void refresh(); | ||
| }, [open, currentSessionId, refresh]); | ||
| if (open) void refresh(forceSessionSync); | ||
| }, [forceSessionSync, open, currentSessionId, refresh]); | ||
|
|
||
| // Switching agent points the list at a different profile's DB. Force a | ||
| // reload immediately (bypassing the throttle) so the list isn't stale. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.