mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 20:53:06 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
+69
@@ -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> = {}): 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]);
|
||||
});
|
||||
});
|
||||
@@ -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<Session | null>;
|
||||
sessionIdRef: React.MutableRefObject<string>;
|
||||
fetchSessionDetails: () => Promise<void>;
|
||||
setLoading: React.Dispatch<React.SetStateAction<boolean>>;
|
||||
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) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user