diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 979802c9d7..f67dfa7276 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -111,6 +111,7 @@ export default defineConfig({ "**/cold-switch-longtask.perf.ts", "**/timeline-no-shift.spec.ts", "**/thread-summary-stability.spec.ts", + "**/channel-revisit-no-skeleton.spec.ts", "**/human-edit-agent-content.spec.ts", "**/empty-edit-delete.spec.ts", "**/reaction-order.spec.ts", diff --git a/desktop/src/features/channels/useChannelTimelineLoading.ts b/desktop/src/features/channels/useChannelTimelineLoading.ts index 0427c58680..837c3d2c68 100644 --- a/desktop/src/features/channels/useChannelTimelineLoading.ts +++ b/desktop/src/features/channels/useChannelTimelineLoading.ts @@ -2,6 +2,10 @@ import * as React from "react"; import { useQueryClient } from "@tanstack/react-query"; import { hasPersistedHydratedChannel } from "@/features/messages/lib/channelHeadCache"; +import { + hasTimelineSettledThisSession, + markTimelineSettledThisSession, +} from "@/features/messages/lib/settledTimelineChannels"; import { resolveTimelineLoadingLatch, selectTimelineLoadingState, @@ -25,8 +29,15 @@ export function useChannelTimelineLoading( const queryClient = useQueryClient(); const activeChannelId = activeChannel?.id ?? null; const settledChannelIdRef = React.useRef(null); + // A channel that settled at any point this session keeps its settled + // status across switches: its cache holds an authoritative window (live + // updates merge into it while away), so a stale revisit renders those rows + // stale-while-revalidate instead of flashing the skeleton for the whole + // refetch round-trip. const hasSettledThisChannel = - activeChannelId !== null && settledChannelIdRef.current === activeChannelId; + activeChannelId !== null && + (settledChannelIdRef.current === activeChannelId || + hasTimelineSettledThisSession(activeChannelId)); const timelineLoadingNow = activeChannel !== null && activeChannel.channelType !== "forum" && @@ -51,5 +62,14 @@ export function useChannelTimelineLoading( timelineLoadingNow, ); settledChannelIdRef.current = settledChannelId; + // Record the settle for the rest of the session. Forum channels are + // excluded: ForumView owns their loading, so this latch never observes a + // real settle for them and would mark them settled on the first render. + const isForum = activeChannel?.channelType === "forum"; + React.useEffect(() => { + if (activeChannelId && !isForum && !isTimelineLoading) { + markTimelineSettledThisSession(activeChannelId); + } + }, [activeChannelId, isForum, isTimelineLoading]); return isTimelineLoading; } diff --git a/desktop/src/features/communities/useCommunityInit.ts b/desktop/src/features/communities/useCommunityInit.ts index c565ee0f7b..5cefd1ca8d 100644 --- a/desktop/src/features/communities/useCommunityInit.ts +++ b/desktop/src/features/communities/useCommunityInit.ts @@ -34,6 +34,7 @@ import { resetAgentObserverStore } from "@/features/agents/observerRelayStore"; import { resetAvatarPresentations } from "@/features/profile/avatarPresentationStore"; import { resetAvatarProfileSync } from "@/features/profile/avatarProfileSync"; import { resetSidebarRelayConnectionCardState } from "@/features/sidebar/ui/useSidebarRelayConnectionCard"; +import { resetSettledTimelineChannels } from "@/features/messages/lib/settledTimelineChannels"; import { clearMarkdownNodeCache } from "@/shared/ui/markdown/nodeCache"; import { resetMessageLinkMetadataCache } from "@/shared/ui/markdown/useMessageLinkMetadata"; import { resetVideoPlayerState } from "@/shared/ui/videoPlayerState"; @@ -81,6 +82,7 @@ async function resetCommunityState({ clearSearchHitEventCache(); clearMarkdownNodeCache(); resetMessageLinkMetadataCache(); + resetSettledTimelineChannels(); } type CommunityInitResult = diff --git a/desktop/src/features/messages/lib/settledTimelineChannels.test.mjs b/desktop/src/features/messages/lib/settledTimelineChannels.test.mjs new file mode 100644 index 0000000000..df9f6650df --- /dev/null +++ b/desktop/src/features/messages/lib/settledTimelineChannels.test.mjs @@ -0,0 +1,25 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + hasTimelineSettledThisSession, + markTimelineSettledThisSession, + resetSettledTimelineChannels, +} from "./settledTimelineChannels.ts"; + +test("settled channels are remembered for the session", () => { + resetSettledTimelineChannels(); + assert.equal(hasTimelineSettledThisSession("ch-a"), false); + markTimelineSettledThisSession("ch-a"); + assert.equal(hasTimelineSettledThisSession("ch-a"), true); + assert.equal(hasTimelineSettledThisSession("ch-b"), false); +}); + +test("community reset clears every settled channel", () => { + resetSettledTimelineChannels(); + markTimelineSettledThisSession("ch-a"); + markTimelineSettledThisSession("ch-b"); + resetSettledTimelineChannels(); + assert.equal(hasTimelineSettledThisSession("ch-a"), false); + assert.equal(hasTimelineSettledThisSession("ch-b"), false); +}); diff --git a/desktop/src/features/messages/lib/settledTimelineChannels.ts b/desktop/src/features/messages/lib/settledTimelineChannels.ts new file mode 100644 index 0000000000..28396f8752 --- /dev/null +++ b/desktop/src/features/messages/lib/settledTimelineChannels.ts @@ -0,0 +1,28 @@ +/** + * Session registry of channels whose timeline has settled at least once. + * + * The per-mount loading latch (resolveTimelineLoadingLatch) resets on every + * channel switch, so a revisit past the messages query's staleTime used to + * refetch on mount with the "unsettled" skeleton branch active — flashing a + * skeleton over a fully cached timeline for the whole relay round-trip. A + * channel that settled once this session has an authoritative window in the + * query cache (live updates merge into it while away), so revisits render + * stale-while-revalidate instead. + * + * Community-scoped module state: reset via resetCommunityState() + * (useCommunityInit.ts), like every other community-scoped singleton. + */ + +const settledChannelIds = new Set(); + +export function markTimelineSettledThisSession(channelId: string): void { + settledChannelIds.add(channelId); +} + +export function hasTimelineSettledThisSession(channelId: string): boolean { + return settledChannelIds.has(channelId); +} + +export function resetSettledTimelineChannels(): void { + settledChannelIds.clear(); +} diff --git a/desktop/src/features/messages/lib/timelineLoadingState.test.mjs b/desktop/src/features/messages/lib/timelineLoadingState.test.mjs index fe6960f74c..712ab798c4 100644 --- a/desktop/src/features/messages/lib/timelineLoadingState.test.mjs +++ b/desktop/src/features/messages/lib/timelineLoadingState.test.mjs @@ -33,14 +33,20 @@ test("stale placeholder while refetching is loading", () => { test("subscription-seeded empty cache while fetching is loading", () => { // The live subscription's setQueryData seeds [] before history settles, so - // data is defined but empty and a fetch is still in flight. + // data is defined but empty and a fetch is still in flight. This is a + // cold-load shape: the channel has not settled this session (a channel + // that HAS settled owns an authoritative — possibly empty — window and + // paints it instead; see the settled-empty-revisit test below). assert.equal( - selectTimelineLoadingState({ - ...settled, - isFetching: true, - isPlaceholderData: false, - dataLength: 0, - }), + selectTimelineLoadingState( + { + ...settled, + isFetching: true, + isPlaceholderData: false, + dataLength: 0, + }, + false, + ), true, ); }); @@ -158,3 +164,21 @@ test("latch: no active channel passes loadingNow through untouched", () => { false, ); }); + +test("settled empty channel revisit paints the empty state, not a skeleton", () => { + // The channel settled this session with zero rows: its cache holds an + // authoritative empty window, so a stale-revisit refetch must render the + // empty state stale-while-revalidate — same contract as populated rows. + assert.equal( + selectTimelineLoadingState( + { + isPending: false, + isFetching: true, + isPlaceholderData: false, + dataLength: 0, + }, + true, + ), + false, + ); +}); diff --git a/desktop/src/features/messages/lib/timelineLoadingState.ts b/desktop/src/features/messages/lib/timelineLoadingState.ts index ea46168d25..2ef3cbc96e 100644 --- a/desktop/src/features/messages/lib/timelineLoadingState.ts +++ b/desktop/src/features/messages/lib/timelineLoadingState.ts @@ -34,10 +34,11 @@ export function selectTimelineLoadingState( // painting those as if loaded flashes a near-empty timeline. return status.isFetching; } - return ( - status.isFetching && - (status.isPlaceholderData || (status.dataLength ?? 0) === 0) - ); + // Settled this session: the cache holds an authoritative window — possibly + // authoritatively EMPTY — so refetches paint cached content (or the empty + // state) stale-while-revalidate. Only placeholder data still skeletons: it + // is not this channel's own settled window. + return status.isFetching && status.isPlaceholderData; } /** diff --git a/desktop/src/features/messages/lib/timelineSnapshot.test.mjs b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs index a8afb823ec..7a1136b811 100644 --- a/desktop/src/features/messages/lib/timelineSnapshot.test.mjs +++ b/desktop/src/features/messages/lib/timelineSnapshot.test.mjs @@ -610,7 +610,11 @@ test("deferred-snapshot: fresh when channel ids match", () => { ); }); -test("timeline-body-surface: stale deferred channel snapshot paints skeleton instead of old list", () => { +test("timeline-body-surface: stale deferred channel snapshot paints blank instead of old list", () => { + // Production wiring (MessageTimeline) routes deferred-snapshot divergence + // through isSwitchGap, not isLoading: the gap is a render-pipeline + // artifact, so it paints the blank surface — never the previous channel's + // rows, and not a false skeleton. const isStale = isDeferredTimelineSnapshotStale({ deferredSnapshot: { channelId: "chan-a" }, liveSnapshot: { channelId: "chan-b" }, @@ -619,10 +623,11 @@ test("timeline-body-surface: stale deferred channel snapshot paints skeleton ins assert.equal( selectTimelineBodySurface({ deferredCount: 4, - isLoading: isStale, + isLoading: false, + isSwitchGap: isStale, liveCount: 0, }), - "skeleton", + "blank", ); }); @@ -731,3 +736,29 @@ test("isRenderedTimelineBehindHistoryPrepend: false when rendered oldest already // not behind an older-history prepend. assert.equal(isRenderedTimelineBehindHistoryPrepend([a], [a, b]), false); }); + +test("timeline-body-surface: the channel-switch gap is blank, never a skeleton flash", () => { + // Deferred snapshot still holds the previous channel while the live one + // moved on. Without an authoritative load in flight this is a 1-2 frame + // render-pipeline gap — paint background, not flashing skeleton bars. + assert.equal( + selectTimelineBodySurface({ + deferredCount: 5, + isLoading: false, + isSwitchGap: true, + liveCount: 3, + }), + "blank", + ); + // A genuine authoritative load during the gap keeps the skeleton: the cold + // switch is a real loading state, not a pipeline artifact. + assert.equal( + selectTimelineBodySurface({ + deferredCount: 5, + isLoading: true, + isSwitchGap: true, + liveCount: 0, + }), + "skeleton", + ); +}); diff --git a/desktop/src/features/messages/lib/timelineSnapshot.ts b/desktop/src/features/messages/lib/timelineSnapshot.ts index 4e23fb2245..f7872a0400 100644 --- a/desktop/src/features/messages/lib/timelineSnapshot.ts +++ b/desktop/src/features/messages/lib/timelineSnapshot.ts @@ -181,22 +181,34 @@ export function selectDeferredListRenderState( return "pending"; } -export type TimelineBodySurface = "skeleton" | "empty" | "list"; +export type TimelineBodySurface = "skeleton" | "blank" | "empty" | "list"; export function selectTimelineBodySurface({ deferredCount, preserveSettledEmptyIntro = false, isLoading, + isSwitchGap = false, liveCount, }: { deferredCount: number; preserveSettledEmptyIntro?: boolean; isLoading: boolean; + /** Deferred snapshot still holds another channel (channel-switch gap). */ + isSwitchGap?: boolean; liveCount: number; }): TimelineBodySurface { if (isLoading) { return "skeleton"; } + if (isSwitchGap) { + // The deferred snapshot lags the live one for a frame or two on every + // channel switch. Without an authoritative load in flight that gap is a + // render-pipeline artifact, not a loading state: paint plain background + // instead of flashing skeleton bars on every switch. (Painting the + // previous channel's lagging rows here is also wrong — the header and + // sidebar have already moved on.) + return "blank"; + } const renderState = selectDeferredListRenderState(deferredCount, liveCount); if (renderState === "pending") { diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index fa8bb4e9f6..9d2911fe4f 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -299,10 +299,17 @@ const MessageTimelineBase = React.forwardRef< const timelineBodySurface = selectTimelineBodySurface({ deferredCount: deferredMessages.length, preserveSettledEmptyIntro, - isLoading: timelineIsLoading, + isLoading, + isSwitchGap: isDeferredSnapshotStale, liveCount: messages.length, }); - const showTimelineSkeleton = timelineBodySurface === "skeleton"; + // "blank" (the 1-2 frame channel-switch gap) blocks the same behaviors as + // the skeleton — no autoscroll, jumps, pills, or scrollback while the + // deferred snapshot catches up — but renders plain background instead of + // flashing skeleton bars on every switch. + const showTimelineSkeleton = + timelineBodySurface === "skeleton" || timelineBodySurface === "blank"; + const showSkeletonVisual = timelineBodySurface === "skeleton"; const [isSemanticallyAtBottom, setIsSemanticallyAtBottom] = React.useState(true); // biome-ignore lint/correctness/useExhaustiveDependencies: reset semantic tail state when the active channel changes @@ -601,6 +608,9 @@ const MessageTimelineBase = React.forwardRef< sentinelRef: topSentinelRef, }); + // Blocked (skeleton OR blank switch gap) uses EMPTY_MESSAGES: during the + // gap the deferred rows still belong to the previous channel, and shape + // caching must never write them under this channel's key. const timelineSkeletonRows = useTimelineSkeletonRows({ channelId, isLoading: showTimelineSkeleton, @@ -741,6 +751,7 @@ const MessageTimelineBase = React.forwardRef< data-buzz-conversation-scroll={ useTimelineVirtualizer && showMessageList ? undefined : "true" } + data-render-pending={isRenderPending ? "true" : undefined} data-scroll-restoration-id={scrollRestorationId} data-testid={ useTimelineVirtualizer && showMessageList @@ -791,7 +802,7 @@ const MessageTimelineBase = React.forwardRef< "mt-auto", )} > - {showTimelineSkeleton ? ( + {showSkeletonVisual ? ( ) : null} {activeDirectMessageIntro ? ( diff --git a/desktop/src/features/messages/ui/TimelineSkeleton.tsx b/desktop/src/features/messages/ui/TimelineSkeleton.tsx index 7d290be6e6..5471625a70 100644 --- a/desktop/src/features/messages/ui/TimelineSkeleton.tsx +++ b/desktop/src/features/messages/ui/TimelineSkeleton.tsx @@ -213,6 +213,7 @@ export function TimelineSkeleton({ rows }: TimelineSkeletonProps) { {skeletonRows.map((row) => (
diff --git a/desktop/tests/e2e/channel-revisit-no-skeleton.spec.ts b/desktop/tests/e2e/channel-revisit-no-skeleton.spec.ts new file mode 100644 index 0000000000..9bec3b3da0 --- /dev/null +++ b/desktop/tests/e2e/channel-revisit-no-skeleton.spec.ts @@ -0,0 +1,110 @@ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +/** + * Regression: revisiting a channel that already settled this session must + * paint its cached rows immediately — never a skeleton over known content. + * + * The timeline loading latch used to reset on every channel switch, so a + * re-entry past the messages query's staleTime refetched on mount with + * `isFetching: true` and the unsettled branch held the skeleton for the whole + * relay round-trip — a full flash of skeleton over a fully cached timeline. + * Session-settled channels now render stale-while-revalidate. + * + * Staleness is forced by shifting Date.now() (React Query's staleness clock) + * past the 5-minute window; the window fetch is delayed so the would-be flash + * window is wide enough to observe deterministically. + */ + +const WINDOW_FETCH_DELAY_MS = 600; +const STALENESS_JUMP_MS = 6 * 60_000; + +test("revisiting a settled channel paints cached rows, not a skeleton", async ({ + page, +}) => { + // Date.now offset shim — must install before the app boots. + await page.addInitScript(() => { + const realNow = Date.now.bind(Date); + (window as unknown as { __TIME_OFFSET_MS__: number }).__TIME_OFFSET_MS__ = + 0; + Date.now = () => + realNow() + + (window as unknown as { __TIME_OFFSET_MS__: number }).__TIME_OFFSET_MS__; + }); + await installMockBridge(page, { + // Head (cursorless) fetches are the revisit refetch path; delaying them + // creates the observation window this spec asserts over. + channelHeadDelayMs: WINDOW_FETCH_DELAY_MS, + }); + await page.goto("/"); + await page.waitForFunction( + () => typeof window.__BUZZ_E2E_EMIT_MOCK_MESSAGE__ === "function", + ); + + // First visit: general settles (cold load may show the skeleton — fine). + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + await expect(page.locator("[data-message-id]").first()).toBeVisible({ + timeout: 15_000, + }); + + // Leave for another channel. + await page.getByTestId("channel-deep-history").click(); + await expect(page.getByTestId("chat-title")).toHaveText("deep-history"); + await expect( + page.locator('[data-message-id^="mock-deep-history-"]').first(), + ).toBeVisible({ timeout: 15_000 }); + + // Everything general cached is now stale. + await page.evaluate((jump) => { + (window as unknown as { __TIME_OFFSET_MS__: number }).__TIME_OFFSET_MS__ = + jump; + }, STALENESS_JUMP_MS); + + // Revisit: sample the surface every frame through the refetch window. The + // cached rows must be present the whole time and the skeleton must never + // mount. + const result = await page.evaluate(async (windowDelay) => { + const link = document.querySelector( + '[data-testid="channel-general"]', + ); + if (!link) throw new Error("missing sidebar link"); + const samples: Array<{ skeleton: boolean; rows: number }> = []; + link.click(); + const deadline = performance.now() + windowDelay + 400; + await new Promise((resolve) => { + const tick = () => { + samples.push({ + skeleton: + document.querySelector('[data-testid="timeline-skeleton-row"]') !== + null, + rows: document.querySelectorAll("[data-message-id]").length, + }); + if (performance.now() > deadline) { + resolve(); + return; + } + requestAnimationFrame(tick); + }; + requestAnimationFrame(tick); + }); + return { + sampleCount: samples.length, + skeletonFrames: samples.filter((sample) => sample.skeleton).length, + rowlessFrames: samples.filter((sample) => sample.rows === 0).length, + }; + }, WINDOW_FETCH_DELAY_MS); + + expect(result.sampleCount).toBeGreaterThan(20); + expect( + result.skeletonFrames, + "skeleton mounted over cached rows during the stale-revisit refetch", + ).toBe(0); + // The deferred-snapshot switch gap may render blank background for a couple + // of frames; anything longer means cached rows failed to paint. + expect( + result.rowlessFrames, + "cached rows stayed absent past the switch gap during the stale-revisit refetch", + ).toBeLessThanOrEqual(4); +});