From da15079f34531dc1fbab9aed99e5e017518bcb39 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Mon, 8 Jun 2026 19:07:44 +0200 Subject: [PATCH] fix(web): stop session timeline scroll-jumping on event refresh The progressive paged event fetch replaced the list with the first 200 rows on every SignalR/30s refresh, then re-grew it page by page. On sessions with >200 events this shrink-then-grow changed the page height repeatedly, making the scrollbar/viewport jump while reading and dropping open "Details" panels. Make the event list append-only and monotonic via mergeNewEvents: only genuinely new events (keyed identically to the React row key) are appended at the bottom; existing rows are never replaced. When a refresh brings nothing new the same array reference is returned, so React skips the re-render and scroll position + open details stay put. Native scroll anchoring then keeps the viewport pinned as new events append below. Session telemetry is append-only/immutable, so keeping existing rows is safe. The transient-empty-refresh guard is now subsumed by the merge; the dead sessionIdRef param (only used by the removed warning) is dropped. Adds mergeNewEvents unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../hooks/__tests__/mergeNewEvents.test.ts | 69 +++++++++++++++++++ .../[sessionId]/hooks/useSessionEvents.ts | 55 +++++++++++---- .../app/sessions/[sessionId]/page.tsx | 1 - 3 files changed, 109 insertions(+), 16 deletions(-) create mode 100644 src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/__tests__/mergeNewEvents.test.ts diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/__tests__/mergeNewEvents.test.ts b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/__tests__/mergeNewEvents.test.ts new file mode 100644 index 00000000..4d688ff7 --- /dev/null +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/__tests__/mergeNewEvents.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect } from "vitest"; +import { mergeNewEvents } from "../useSessionEvents"; +import { EnrollmentEvent } from "@/types"; + +/** + * mergeNewEvents underpins the timeline's scroll stability: it must keep the + * rendered event list monotonic (append-only) so a paged refresh never wipes the + * list back to the first 200 rows and re-grows it (which made the viewport jump). + */ + +function ev(sequence: number, overrides: Partial = {}): EnrollmentEvent { + return { + eventId: overrides.eventId ?? `evt-${sequence}`, + sessionId: overrides.sessionId ?? "session-1", + timestamp: overrides.timestamp ?? "2026-06-08T10:00:00Z", + eventType: overrides.eventType ?? "info_event", + severity: overrides.severity ?? "Info", + source: overrides.source ?? "Test", + phase: overrides.phase ?? 0, + message: overrides.message ?? `event ${sequence}`, + sequence, + ...overrides, + }; +} + +describe("mergeNewEvents", () => { + it("returns all events on first load (empty prev)", () => { + const incoming = [ev(1), ev(2), ev(3)]; + expect(mergeNewEvents([], incoming)).toEqual(incoming); + }); + + it("appends only genuinely new events, in order, after existing ones", () => { + const prev = [ev(1), ev(2)]; + const incoming = [ev(1), ev(2), ev(3), ev(4)]; + const merged = mergeNewEvents(prev, incoming); + expect(merged.map(e => e.sequence)).toEqual([1, 2, 3, 4]); + }); + + it("returns the SAME reference when nothing new arrived (no re-render)", () => { + const prev = [ev(1), ev(2), ev(3)]; + // A refresh that re-returns the same first page must not allocate a new array. + const merged = mergeNewEvents(prev, [ev(1), ev(2), ev(3)]); + expect(merged).toBe(prev); + }); + + it("preserves existing event object identity (never replaces append-only rows)", () => { + const e1 = ev(1); + const e2 = ev(2); + const prev = [e1, e2]; + // Incoming carries a fresh object for e1 with a mutated message — must be ignored. + const merged = mergeNewEvents(prev, [ev(1, { message: "rewritten" }), ev(3)]); + expect(merged[0]).toBe(e1); + expect(merged[1]).toBe(e2); + expect(merged[0].message).toBe("event 1"); + expect(merged[2].sequence).toBe(3); + }); + + it("absorbs a transient empty refresh by keeping the previous list", () => { + const prev = [ev(1), ev(2)]; + expect(mergeNewEvents(prev, [])).toBe(prev); + }); + + it("falls back to sessionId+sequence when eventId is missing", () => { + const prev = [ev(1, { eventId: "" })]; + // Same sessionId+sequence → same identity → not re-added. + const merged = mergeNewEvents(prev, [ev(1, { eventId: "" }), ev(2, { eventId: "" })]); + expect(merged.map(e => e.sequence)).toEqual([1, 2]); + }); +}); diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts index a777b410..19195cdb 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/hooks/useSessionEvents.ts @@ -18,6 +18,38 @@ const TIMELINE_PAGE_SIZE = 200; // drop them until the user manually refreshes. One nachfass-fetch covers the window. const TERMINAL_TRAILING_REFETCH_DELAY_MS = 12_000; +/** + * Stable event key — mirrors the React `key` used by the timeline rows so that + * merge identity and React reconciliation identity agree. + */ +function eventKey(e: EnrollmentEvent): string { + return e.eventId || `${e.sessionId}-${e.sequence}`; +} + +/** + * Append-only merge: returns `prev` plus any events from `incoming` whose key is + * not already present. Session telemetry is append-only (events are immutable + * once written), so existing rows are never replaced. The timeline relies on two + * consequences of this: + * - The rendered list is monotonic — it never shrinks mid-refresh. The + * progressive paging fetch used to wipe the list to the first 200 rows and + * then grow it back as later pages streamed in, which changed the page + * height repeatedly and made the scrollbar/viewport jump while reading. + * - When a refresh brings nothing new, the SAME array reference is returned, + * so React bails out of the re-render entirely — open "Details" panels and + * scroll position stay exactly where the user left them. + * New events arrive at the bottom (highest Sequence) and the render-time sort in + * useSessionDerivedData keeps ordering correct regardless of append order, so + * the browser's native scroll anchoring keeps the viewport pinned. + */ +export function mergeNewEvents(prev: EnrollmentEvent[], incoming: EnrollmentEvent[]): EnrollmentEvent[] { + if (incoming.length === 0) return prev; + const seen = new Set(prev.map(eventKey)); + const additions = incoming.filter(e => !seen.has(eventKey(e))); + if (additions.length === 0) return prev; + return prev.concat(additions); +} + type AddNotification = ( type: NotificationType, title: string, @@ -32,7 +64,6 @@ interface UseSessionEventsParams { sessionStatus: string | null | undefined; resolveEffectiveTenantId: () => string | null; sessionRef: React.MutableRefObject; - sessionIdRef: React.MutableRefObject; fetchSessionDetails: () => Promise; setLoading: React.Dispatch>; isConnected: boolean; @@ -69,7 +100,6 @@ export function useSessionEvents({ sessionStatus, resolveEffectiveTenantId, sessionRef, - sessionIdRef, fetchSessionDetails, setLoading, isConnected, @@ -120,17 +150,12 @@ export function useSessionEvents({ const firstData = await firstResponse.json(); const firstBatch: EnrollmentEvent[] = Array.isArray(firstData.events) ? firstData.events : []; - setEvents(prevEvents => { - // Keep the last known-good snapshot if backend transiently returns an empty list - // AND there is no nextLink to follow (avoids wiping a populated timeline). - if (firstBatch.length === 0 && prevEvents.length > 0 && !firstData.nextLink) { - console.warn( - `[SessionDetail] Ignoring empty event refresh for session ${sessionIdRef.current} (tenant ${effectiveTenantId}); keeping ${prevEvents.length} cached events` - ); - return prevEvents; - } - return firstBatch; - }); + // Append-only merge instead of replace: the list stays monotonic so it + // never shrinks-then-grows across the paged refresh (the old wipe-to-200 + // behaviour made the scrollbar/viewport jump on every poll). A transient + // empty refresh is also naturally absorbed — mergeNewEvents returns the + // previous list unchanged when nothing new arrived. + setEvents(prev => mergeNewEvents(prev, firstBatch)); // Trigger an early loading=false now: first paint is unblocked. setLoading(false); @@ -168,7 +193,7 @@ export function useSessionEvents({ const pageData = await resp.json(); const batch: EnrollmentEvent[] = Array.isArray(pageData.events) ? pageData.events : []; if (batch.length > 0) { - setEvents(prev => prev.concat(batch)); + setEvents(prev => mergeNewEvents(prev, batch)); if (!foundTerminalEvent && batch.some(isTerminalEvent)) { foundTerminalEvent = true; } @@ -210,7 +235,7 @@ export function useSessionEvents({ fetchEvents(); } } - }, [sessionId, resolveEffectiveTenantId, sessionRef, sessionIdRef, fetchSessionDetails, setLoading, getAccessToken, addNotification]); + }, [sessionId, resolveEffectiveTenantId, sessionRef, fetchSessionDetails, setLoading, getAccessToken, addNotification]); const scheduleFetchEvents = useCallback((delayMs = 300) => { if (eventRefreshTimeoutRef.current) { diff --git a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx index 7c072661..c3c7dc66 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx +++ b/src/Web/autopilot-monitor-web/app/sessions/[sessionId]/page.tsx @@ -78,7 +78,6 @@ export default function SessionDetailPage() { sessionStatus: detail.session?.status ?? null, resolveEffectiveTenantId: detail.resolveEffectiveTenantId, sessionRef: detail.sessionRef, - sessionIdRef: detail.sessionIdRef, fetchSessionDetails: detail.fetchSessionDetails, setLoading: detail.setLoading, isConnected,