mirror of
https://github.com/block/buzz.git
synced 2026-11-13 21:01:53 +00:00
fix(desktop): stop the timeline skeleton flashing on channel switches
Two skeleton flashes made switching feel clunky: - Every switch: the deferred timeline snapshot lags the live one for a frame or two, and that gap painted the full skeleton to mask the previous channel's rows — a visible skeleton flash on every switch. The gap is a render-pipeline artifact, not a loading state: it now renders plain background (new "blank" body surface). Blank blocks the same behaviors as the skeleton (autoscroll, jumps, unread pill, scrollback), only the visual differs. Cold loads keep the skeleton. - Stale revisits: the loading latch reset on every switch, so re-entering a channel past the messages staleTime refetched on mount with the unsettled skeleton branch active — a skeleton over a fully cached timeline for the whole relay round-trip. Channels that settled once this session now render their cached rows stale-while-revalidate (session registry, reset via resetCommunityState like every other community-scoped singleton). The revisit spec forces staleness by shifting Date.now past the 5-minute window and asserts the skeleton never mounts over cached rows; the switch-gap behavior is covered by the timelineSnapshot unit suite. Signed-off-by: Max Lampert <maxwell@squareup.com>
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -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<string | null>(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;
|
||||
}
|
||||
|
||||
@@ -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 =
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
28
desktop/src/features/messages/lib/settledTimelineChannels.ts
Normal file
28
desktop/src/features/messages/lib/settledTimelineChannels.ts
Normal file
@@ -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<string>();
|
||||
|
||||
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();
|
||||
}
|
||||
@@ -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,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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") {
|
||||
|
||||
@@ -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 ? (
|
||||
<TimelineSkeleton rows={timelineSkeletonRows} />
|
||||
) : null}
|
||||
{activeDirectMessageIntro ? (
|
||||
|
||||
@@ -213,6 +213,7 @@ export function TimelineSkeleton({ rows }: TimelineSkeletonProps) {
|
||||
{skeletonRows.map((row) => (
|
||||
<article
|
||||
className="relative mx-1 flex items-start gap-2.5 rounded-2xl px-2 py-2"
|
||||
data-testid="timeline-skeleton-row"
|
||||
key={row.key}
|
||||
>
|
||||
<Skeleton className="h-9 w-9 shrink-0 rounded-full" />
|
||||
|
||||
110
desktop/tests/e2e/channel-revisit-no-skeleton.spec.ts
Normal file
110
desktop/tests/e2e/channel-revisit-no-skeleton.spec.ts
Normal file
@@ -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<HTMLElement>(
|
||||
'[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<void>((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);
|
||||
});
|
||||
Reference in New Issue
Block a user