mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): preserve fresh channel timelines (#5577)
## Summary - skip the channel subscription catch-up request when the authoritative channel window was fetched successfully within the existing five-minute freshness period - keep the responsive deferred skeleton for populated channel switches instead of briefly rendering empty-channel actions - preserve a real empty-channel intro across the first appended message only after React has committed that empty state This is intentionally narrow. It does not claim to solve the separate sidebar startup cost or general main-thread stalls found during the investigation. ### Related issue N/A — no matching open issue or PR found. ### Testing - pre-push desktop gate on `f1be6beea90b9715e04e5fc65cc5cfbe8210e0d9`: - desktop tests: 4,621 passed - desktop check: passed - desktop typecheck: passed - branch-skew: passed - focused cache/surface/lifecycle tests: 62 passed - manual diagnostic trace after rollback: - 16/16 channel revisits skipped catch-up refresh - 0 revisit refresh starts - 0 populated-channel empty/intro flashes - cached switches retained the deferred skeleton-to-list path No screenshot: the regression is a transient channel-switch state and request behavior, covered by lifecycle tests and the diagnostic trace rather than a stable visual diff. Signed-off-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
This commit is contained in:
@@ -17,6 +17,7 @@ import {
|
||||
import {
|
||||
projectChannelWindowMessages,
|
||||
refreshChannelWindowMessages,
|
||||
shouldRefreshChannelWindowAfterSubscribe,
|
||||
} from "@/features/messages/lib/projectChannelWindow";
|
||||
import { reconcileChannelWindowMessages } from "@/features/messages/lib/channelWindowReconciliation";
|
||||
import {
|
||||
@@ -373,6 +374,9 @@ export function useChannelSubscription(channel: Channel | null) {
|
||||
}
|
||||
|
||||
cleanup = dispose;
|
||||
if (!shouldRefreshChannelWindowAfterSubscribe(queryClient, channelId)) {
|
||||
return;
|
||||
}
|
||||
void refreshNewestWindow().catch((error) => {
|
||||
if (!isDisposed) {
|
||||
console.error(
|
||||
@@ -394,7 +398,7 @@ export function useChannelSubscription(channel: Channel | null) {
|
||||
void cleanup();
|
||||
}
|
||||
};
|
||||
}, [channelId, channelType]);
|
||||
}, [channelId, channelType, queryClient]);
|
||||
}
|
||||
|
||||
export function useSendMessageMutation(
|
||||
|
||||
@@ -11,8 +11,10 @@ import {
|
||||
replaceNewestChannelWindow,
|
||||
} from "./channelWindowStore.ts";
|
||||
import {
|
||||
CHANNEL_WINDOW_FRESH_MS,
|
||||
projectChannelWindowMessages,
|
||||
refreshChannelWindowMessages,
|
||||
shouldRefreshChannelWindowAfterSubscribe,
|
||||
} from "./projectChannelWindow.ts";
|
||||
import { reconcileChannelWindowMessages } from "./channelWindowReconciliation.ts";
|
||||
|
||||
@@ -273,3 +275,93 @@ test("test_live_projection_retains_pending_send_and_non_broadcast_thread_reply",
|
||||
"live",
|
||||
]);
|
||||
});
|
||||
|
||||
test("test_subscribe_refresh_skips_fresh_populated_window", () => {
|
||||
const harness = createHarness();
|
||||
const updatedAt = harness.client.getQueryState(
|
||||
harness.windowKey,
|
||||
).dataUpdatedAt;
|
||||
|
||||
assert.equal(
|
||||
shouldRefreshChannelWindowAfterSubscribe(
|
||||
harness.client,
|
||||
harness.channelId,
|
||||
updatedAt + CHANNEL_WINDOW_FRESH_MS - 1,
|
||||
),
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
test("test_subscribe_refresh_runs_for_stale_window", () => {
|
||||
const harness = createHarness();
|
||||
const updatedAt = harness.client.getQueryState(
|
||||
harness.windowKey,
|
||||
).dataUpdatedAt;
|
||||
|
||||
assert.equal(
|
||||
shouldRefreshChannelWindowAfterSubscribe(
|
||||
harness.client,
|
||||
harness.channelId,
|
||||
updatedAt + CHANNEL_WINDOW_FRESH_MS,
|
||||
),
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("test_live_cache_merge_does_not_extend_window_freshness", () => {
|
||||
const harness = createHarness();
|
||||
const windowUpdatedAt = harness.client.getQueryState(
|
||||
harness.windowKey,
|
||||
).dataUpdatedAt;
|
||||
|
||||
harness.client.setQueryData(harness.messagesKey, (messages) => [
|
||||
...messages,
|
||||
event("live-cache-only", 110),
|
||||
]);
|
||||
|
||||
assert.equal(
|
||||
shouldRefreshChannelWindowAfterSubscribe(
|
||||
harness.client,
|
||||
harness.channelId,
|
||||
windowUpdatedAt + CHANNEL_WINDOW_FRESH_MS,
|
||||
),
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("test_subscribe_refresh_runs_without_a_message_query", () => {
|
||||
const client = new QueryClient({
|
||||
defaultOptions: { queries: { retry: false } },
|
||||
});
|
||||
|
||||
assert.equal(
|
||||
shouldRefreshChannelWindowAfterSubscribe(client, "missing-channel"),
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
test("test_subscribe_refresh_does_not_duplicate_inflight_initial_fetch", async () => {
|
||||
const client = new QueryClient({
|
||||
defaultOptions: { queries: { retry: false } },
|
||||
});
|
||||
const channelId = "pending-channel";
|
||||
const queryKey = channelMessagesKey(channelId);
|
||||
let resolveFetch;
|
||||
const observer = new QueryObserver(client, {
|
||||
queryKey,
|
||||
queryFn: () =>
|
||||
new Promise((resolve) => {
|
||||
resolveFetch = resolve;
|
||||
}),
|
||||
});
|
||||
const unsubscribe = observer.subscribe(() => {});
|
||||
|
||||
assert.equal(
|
||||
shouldRefreshChannelWindowAfterSubscribe(client, channelId),
|
||||
false,
|
||||
);
|
||||
|
||||
resolveFetch([]);
|
||||
await client.getQueryCache().find({ queryKey })?.promise;
|
||||
unsubscribe();
|
||||
});
|
||||
|
||||
@@ -8,6 +8,34 @@ import {
|
||||
} from "./channelWindowStore";
|
||||
import { reconcileChannelWindowMessages } from "./channelWindowReconciliation";
|
||||
|
||||
export const CHANNEL_WINDOW_FRESH_MS = 5 * 60_000;
|
||||
|
||||
/**
|
||||
* Subscription setup closes the gap between the initial page and live events,
|
||||
* but revisiting a channel with a fresh page has no gap to close. Reconnects
|
||||
* still refresh unconditionally at their call site.
|
||||
*/
|
||||
export function shouldRefreshChannelWindowAfterSubscribe(
|
||||
queryClient: QueryClient,
|
||||
channelId: string,
|
||||
now = Date.now(),
|
||||
): boolean {
|
||||
const messagesState = queryClient.getQueryState(
|
||||
channelMessagesKey(channelId),
|
||||
);
|
||||
if (!messagesState) return true;
|
||||
if (messagesState.fetchStatus === "fetching") return false;
|
||||
const windowState = queryClient.getQueryState(channelWindowKey(channelId));
|
||||
if (
|
||||
messagesState.status !== "success" ||
|
||||
windowState?.status !== "success" ||
|
||||
windowState.dataUpdatedAt === 0
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
return now - windowState.dataUpdatedAt >= CHANNEL_WINDOW_FRESH_MS;
|
||||
}
|
||||
|
||||
/** Keep the rendered timeline cache aligned with its authoritative window. */
|
||||
export function projectChannelWindowMessages(
|
||||
queryClient: QueryClient,
|
||||
|
||||
@@ -420,11 +420,26 @@ test("timeline-body-surface: loading and deferred-pending both paint the single
|
||||
);
|
||||
});
|
||||
|
||||
test("timeline-body-surface: first deferred message preserves a persistent channel intro", () => {
|
||||
test("timeline-body-surface: first authoritative rows wait for deferred paint", () => {
|
||||
// A newly selected populated channel has already resolved live rows, but the
|
||||
// deferred snapshot is still empty. It has never committed a settled empty
|
||||
// surface, so showing its intro here would flash Create agent / Add people.
|
||||
assert.equal(
|
||||
selectTimelineBodySurface({
|
||||
deferredCount: 0,
|
||||
hasPersistentIntro: true,
|
||||
preserveSettledEmptyIntro: false,
|
||||
isLoading: false,
|
||||
liveCount: 1,
|
||||
}),
|
||||
"skeleton",
|
||||
);
|
||||
});
|
||||
|
||||
test("timeline-body-surface: append preserves a previously settled empty intro", () => {
|
||||
assert.equal(
|
||||
selectTimelineBodySurface({
|
||||
deferredCount: 0,
|
||||
preserveSettledEmptyIntro: true,
|
||||
isLoading: false,
|
||||
liveCount: 1,
|
||||
}),
|
||||
|
||||
@@ -185,12 +185,12 @@ export type TimelineBodySurface = "skeleton" | "empty" | "list";
|
||||
|
||||
export function selectTimelineBodySurface({
|
||||
deferredCount,
|
||||
hasPersistentIntro = false,
|
||||
preserveSettledEmptyIntro = false,
|
||||
isLoading,
|
||||
liveCount,
|
||||
}: {
|
||||
deferredCount: number;
|
||||
hasPersistentIntro?: boolean;
|
||||
preserveSettledEmptyIntro?: boolean;
|
||||
isLoading: boolean;
|
||||
liveCount: number;
|
||||
}): TimelineBodySurface {
|
||||
@@ -200,10 +200,11 @@ export function selectTimelineBodySurface({
|
||||
|
||||
const renderState = selectDeferredListRenderState(deferredCount, liveCount);
|
||||
if (renderState === "pending") {
|
||||
// A channel/DM intro is already meaningful stable content. Preserve it
|
||||
// while React's deferred snapshot catches up to the first live message;
|
||||
// replacing it with a skeleton makes an append look like a page reload.
|
||||
return hasPersistentIntro ? "empty" : "skeleton";
|
||||
// Preserve a channel/DM intro across a new append only when this channel
|
||||
// already committed an authoritative empty timeline. On first load, the
|
||||
// live query can resolve before React's deferred rows commit; painting the
|
||||
// intro in that gap flashes empty-channel actions over incoming messages.
|
||||
return preserveSettledEmptyIntro ? "empty" : "skeleton";
|
||||
}
|
||||
return renderState;
|
||||
}
|
||||
|
||||
@@ -16,6 +16,7 @@ import { cn } from "@/shared/lib/cn";
|
||||
import { channelChrome } from "@/shared/layout/chromeLayout";
|
||||
import { Spinner } from "@/shared/ui/spinner";
|
||||
import { TooltipProvider } from "@/shared/ui/tooltip";
|
||||
import { useCommittedEmptyTimeline } from "./useCommittedEmptyTimeline";
|
||||
import { UnreadPill, unreadCountLabel } from "@/shared/ui/UnreadPill";
|
||||
import { ChannelIntroBlock, type ChannelIntro } from "./ChannelIntroBlock";
|
||||
import { TimelineSkeleton, useTimelineSkeletonRows } from "./TimelineSkeleton";
|
||||
@@ -285,13 +286,20 @@ const MessageTimelineBase = React.forwardRef<
|
||||
setTimelineVirtualizerApi(null);
|
||||
}, [scrollContainerRef, scrollContainerDomKey]);
|
||||
|
||||
const hasPersistentIntro =
|
||||
channelIntro !== null || directMessageIntro !== null || pinnedIntro != null;
|
||||
const timelineIsLoading = isLoading || isDeferredSnapshotStale;
|
||||
const preserveSettledEmptyIntro = useCommittedEmptyTimeline({
|
||||
channelId: channelId ?? null,
|
||||
deferredCount: deferredMessages.length,
|
||||
hasPersistentIntro,
|
||||
isLoading: timelineIsLoading,
|
||||
liveCount: messages.length,
|
||||
});
|
||||
const timelineBodySurface = selectTimelineBodySurface({
|
||||
deferredCount: deferredMessages.length,
|
||||
hasPersistentIntro:
|
||||
channelIntro !== null ||
|
||||
directMessageIntro !== null ||
|
||||
pinnedIntro != null,
|
||||
isLoading: isLoading || isDeferredSnapshotStale,
|
||||
preserveSettledEmptyIntro,
|
||||
isLoading: timelineIsLoading,
|
||||
liveCount: messages.length,
|
||||
});
|
||||
const showTimelineSkeleton = timelineBodySurface === "skeleton";
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
import assert from "node:assert/strict";
|
||||
import { after, afterEach, before, test } from "node:test";
|
||||
|
||||
import { JSDOM } from "jsdom";
|
||||
|
||||
const dom = new JSDOM("<!doctype html><html><body></body></html>", {
|
||||
url: "http://localhost",
|
||||
});
|
||||
|
||||
before(() => {
|
||||
Object.assign(globalThis, {
|
||||
document: dom.window.document,
|
||||
HTMLElement: dom.window.HTMLElement,
|
||||
IS_REACT_ACT_ENVIRONMENT: true,
|
||||
window: dom.window,
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
const { cleanup } = await import("@testing-library/react");
|
||||
cleanup();
|
||||
});
|
||||
|
||||
after(() => dom.window.close());
|
||||
|
||||
async function renderTimelineState(initialProps) {
|
||||
const { renderHook } = await import("@testing-library/react");
|
||||
const { useCommittedEmptyTimeline } = await import(
|
||||
"./useCommittedEmptyTimeline.ts"
|
||||
);
|
||||
return renderHook((props) => useCommittedEmptyTimeline(props), {
|
||||
initialProps,
|
||||
});
|
||||
}
|
||||
|
||||
const empty = {
|
||||
channelId: "channel-a",
|
||||
deferredCount: 0,
|
||||
hasPersistentIntro: true,
|
||||
isLoading: false,
|
||||
liveCount: 0,
|
||||
};
|
||||
|
||||
test("only preserves an intro after an empty timeline commits", async () => {
|
||||
const { result, rerender } = await renderTimelineState(empty);
|
||||
|
||||
assert.equal(result.current, false);
|
||||
rerender({ ...empty, liveCount: 1 });
|
||||
assert.equal(result.current, true);
|
||||
rerender({ ...empty, deferredCount: 1, liveCount: 1 });
|
||||
assert.equal(result.current, false);
|
||||
rerender({ ...empty, liveCount: 1 });
|
||||
assert.equal(result.current, false);
|
||||
});
|
||||
|
||||
test("a committed empty proof never carries across channels", async () => {
|
||||
const { result, rerender } = await renderTimelineState(empty);
|
||||
|
||||
rerender({ ...empty, channelId: "channel-b", liveCount: 1 });
|
||||
assert.equal(result.current, false);
|
||||
});
|
||||
|
||||
test("loading and deferred-stale commits cannot establish empty proof", async () => {
|
||||
const { result, rerender } = await renderTimelineState({
|
||||
...empty,
|
||||
isLoading: true,
|
||||
});
|
||||
|
||||
rerender({ ...empty, liveCount: 1 });
|
||||
assert.equal(result.current, false);
|
||||
});
|
||||
@@ -0,0 +1,35 @@
|
||||
import * as React from "react";
|
||||
|
||||
/** Track only empty timelines that React actually committed for this channel. */
|
||||
export function useCommittedEmptyTimeline({
|
||||
channelId,
|
||||
deferredCount,
|
||||
hasPersistentIntro,
|
||||
isLoading,
|
||||
liveCount,
|
||||
}: {
|
||||
channelId: string | null;
|
||||
deferredCount: number;
|
||||
hasPersistentIntro: boolean;
|
||||
isLoading: boolean;
|
||||
liveCount: number;
|
||||
}) {
|
||||
const committedRef = React.useRef({
|
||||
channelId: null as string | null,
|
||||
hasSettledEmpty: false,
|
||||
});
|
||||
const preserveSettledEmptyIntro =
|
||||
hasPersistentIntro &&
|
||||
committedRef.current.channelId === channelId &&
|
||||
committedRef.current.hasSettledEmpty;
|
||||
|
||||
React.useLayoutEffect(() => {
|
||||
if (isLoading) return;
|
||||
committedRef.current = {
|
||||
channelId,
|
||||
hasSettledEmpty: liveCount === 0 && deferredCount === 0,
|
||||
};
|
||||
}, [channelId, deferredCount, isLoading, liveCount]);
|
||||
|
||||
return preserveSettledEmptyIntro;
|
||||
}
|
||||
Reference in New Issue
Block a user