Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions packages/app/src/pages/session.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
() =>
Expand Down
56 changes: 56 additions & 0 deletions packages/app/src/pages/session/session-fill.test.ts
Original file line number Diff line number Diff line change
@@ -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)
})
})
38 changes: 38 additions & 0 deletions packages/app/src/pages/session/session-fill.ts
Original file line number Diff line number Diff line change
@@ -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
}
Loading