diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 1c9391e0fdcf..5516d2ae13c6 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -62,6 +62,7 @@ import { import { MessageTimeline } from "@/pages/session/timeline/message-timeline" import { createTimelineModel } from "@/pages/session/timeline/model" import { type DiffStyle, SessionReviewTab, type SessionReviewTabProps } from "@/pages/session/review-tab" +import { shouldFillLoad, shouldScrollLoad } from "@/pages/session/session-fill" import { useSessionLayout } from "@/pages/session/session-layout" import { syncSessionModel } from "@/pages/session/session-model-helpers" import { SessionSidePanel } from "@/pages/session/session-side-panel" @@ -1229,13 +1230,40 @@ export default function Page() { const el = scroller if (!el) return - if (el.scrollHeight > el.clientHeight + 1) return - if (!historyMore()) return + if ( + !shouldFillLoad({ + historyMore: historyMore(), + scrollHeight: el.scrollHeight, + clientHeight: el.clientHeight, + messageCount: messages().length, + }) + ) + return void loadOlder() }) } + makeEventListener( + () => scroller, + "scroll", + () => { + const el = scroller + if (!el) return + if (!params.id) return + if ( + !shouldScrollLoad({ + scrollTop: el.scrollTop, + historyMore: historyMore(), + historyLoading: historyLoading(), + }) + ) + return + void loadOlder() + }, + { passive: true }, + ) + createEffect( on( () => diff --git a/packages/app/src/pages/session/session-fill.test.ts b/packages/app/src/pages/session/session-fill.test.ts new file mode 100644 index 000000000000..f1a2d5f4b292 --- /dev/null +++ b/packages/app/src/pages/session/session-fill.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, test } from "bun:test" +import { shouldFillLoad, shouldScrollLoad } from "./session-fill" + +describe("shouldFillLoad", () => { + test("returns false when historyMore is false, regardless of viewport state", () => { + expect(shouldFillLoad({ historyMore: false, scrollHeight: 100, clientHeight: 200, messageCount: 1 })).toBe(false) + expect(shouldFillLoad({ historyMore: false, scrollHeight: 300, clientHeight: 200, messageCount: 10 })).toBe(false) + expect(shouldFillLoad({ historyMore: false, scrollHeight: 200, clientHeight: 200, messageCount: 0 })).toBe(false) + }) + + test("returns true when viewport is not full (scrollHeight <= clientHeight + 1)", () => { + expect(shouldFillLoad({ historyMore: true, scrollHeight: 200, clientHeight: 200, messageCount: 0 })).toBe(true) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 201, clientHeight: 200, messageCount: 0 })).toBe(true) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 100, clientHeight: 200, messageCount: 5 })).toBe(true) + }) + + test("returns true when viewport is full but messageCount is <= 2 (fix: session switch with few messages)", () => { + expect(shouldFillLoad({ historyMore: true, scrollHeight: 300, clientHeight: 200, messageCount: 0 })).toBe(true) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 300, clientHeight: 200, messageCount: 1 })).toBe(true) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 300, clientHeight: 200, messageCount: 2 })).toBe(true) + }) + + test("returns false when viewport is full and messageCount > 2 (normal case, no fill needed)", () => { + expect(shouldFillLoad({ historyMore: true, scrollHeight: 300, clientHeight: 200, messageCount: 3 })).toBe(false) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 300, clientHeight: 200, messageCount: 10 })).toBe(false) + expect(shouldFillLoad({ historyMore: true, scrollHeight: 500, clientHeight: 200, messageCount: 100 })).toBe(false) + }) +}) + +describe("shouldScrollLoad", () => { + test("returns false when scrollTop >= 200", () => { + expect(shouldScrollLoad({ scrollTop: 200, historyMore: true, historyLoading: false })).toBe(false) + expect(shouldScrollLoad({ scrollTop: 500, historyMore: true, historyLoading: false })).toBe(false) + expect(shouldScrollLoad({ scrollTop: 200, historyMore: true, historyLoading: false })).toBe(false) + }) + + test("returns true when scrollTop < 200 and historyMore is true and not loading", () => { + expect(shouldScrollLoad({ scrollTop: 0, historyMore: true, historyLoading: false })).toBe(true) + expect(shouldScrollLoad({ scrollTop: 100, historyMore: true, historyLoading: false })).toBe(true) + expect(shouldScrollLoad({ scrollTop: 199, historyMore: true, historyLoading: false })).toBe(true) + }) + + test("returns false when historyMore is false, even at the top", () => { + expect(shouldScrollLoad({ scrollTop: 0, historyMore: false, historyLoading: false })).toBe(false) + expect(shouldScrollLoad({ scrollTop: 10, historyMore: false, historyLoading: false })).toBe(false) + }) + + test("returns false when historyLoading is true, even at the top", () => { + expect(shouldScrollLoad({ scrollTop: 0, historyMore: true, historyLoading: true })).toBe(false) + expect(shouldScrollLoad({ scrollTop: 50, historyMore: true, historyLoading: true })).toBe(false) + }) + + test("returns false when both historyMore is false and historyLoading is true", () => { + expect(shouldScrollLoad({ scrollTop: 0, historyMore: false, historyLoading: true })).toBe(false) + }) +}) \ No newline at end of file diff --git a/packages/app/src/pages/session/session-fill.ts b/packages/app/src/pages/session/session-fill.ts new file mode 100644 index 000000000000..5e6e7b6731e0 --- /dev/null +++ b/packages/app/src/pages/session/session-fill.ts @@ -0,0 +1,38 @@ +/** + * Determines whether the fill mechanism should trigger loading older messages. + * Used when the scroll viewport is not fully filled with content. + * + * Returns true when: + * - There is more history to load, AND + * - Either the viewport is not full (scrollHeight <= clientHeight + 1), OR + * the message count is small (<= 2, to handle session switch scenarios) + */ +export function shouldFillLoad(input: { + historyMore: boolean + scrollHeight: number + clientHeight: number + messageCount: number +}): boolean { + if (!input.historyMore) return false + if (input.scrollHeight > input.clientHeight + 1 && input.messageCount > 2) return false + return true +} + +/** + * Determines whether scrolling near the top of the timeline should trigger + * loading older messages. Used as a guard in the scroll event handler. + * + * Returns true when: + * - The scroll position is near the top (scrollTop < 200), AND + * - There is more history to load, AND + * - History is not currently loading + */ +export function shouldScrollLoad(input: { + scrollTop: number + historyMore: boolean + historyLoading: boolean +}): boolean { + if (input.scrollTop >= 200) return false + if (!input.historyMore || input.historyLoading) return false + return true +} \ No newline at end of file