Speed up channel/canvas/task loading on app reload#2964
Conversation
Reloading the Channels (project-bluebird) space was slow: channels, their canvases, and filed tasks all blanked out and refetched from scratch. Three changes: - Persist the canvas query cache across reloads (stale-while-revalidate). A shared PersistQueryClientProvider restores channels/dashboards/channel-tasks from disk instantly, then refetches in the background. Persistence is scoped to canvas queries by key, gcTime is raised to match the persist maxAge, and the on-disk blob is wiped on logout/project-switch (via clearAuthScopedQueries) so project-scoped data can't leak across sessions. - Drop a redundant per-channel round-trip: dashboards.list / channelTasks.list resolved the channel's path via an extra getEntry before listing, even though the client already has it from useChannels. Thread the known channelPath through hook → router → service and skip the resolve. - Tune staleTime: useChannels now matches its 30s poll, and the web QueryClient gets the same defaults as desktop (staleTime/gcTime/refetchOnWindowFocus). Generated-By: PostHog Code Task-Id: dc505d33-c219-41c9-ac6b-192caf7730ea
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { channels } = useChannels(); | ||
| const channelPath = channels.find((c) => c.id === channelId)?.path; | ||
| const { dashboards, isLoading } = useDashboards(channelId, channelPath); |
There was a problem hiding this comment.
channelPath key change
On a cold start (no persisted cache), channels is initially [], so channelPath is undefined and useDashboards is called with key { channelId, channelPath: undefined }. Because useChannels() and useDashboards() both fire simultaneously, channels typically resolve first (no extra path-resolve round-trip). The moment channels land, channelPath changes from undefined to the real path and useDashboards switches to a new cache key { channelId, channelPath: "Channels/…" }. React Query has no data for that new key, so isLoading flips back to true, and since line 63 returns null while loading, the dashboards grid disappears and re-renders after the second fetch. This extra render and flash only occur on cold start, not on the persisted-cache reload the PR optimises for, but it is still a regression versus the pre-PR behaviour (single fetch, one loading phase).
| let activePersister: Persister | null = null; | ||
|
|
||
| /** | ||
| * Wipe the on-disk query cache. Called on logout and project switch (via | ||
| * clearAuthScopedQueries) so persisted, project-scoped canvas data never | ||
| * outlives the session that wrote it. | ||
| */ | ||
| export async function removePersistedCache(): Promise<void> { | ||
| await activePersister?.removeClient(); | ||
| } | ||
|
|
There was a problem hiding this comment.
activePersister is set lazily by buildCanvasPersistOptions, which is called as a module-level side-effect in Providers.tsx. If clearAuthScopedQueries (and therefore removePersistedCache) is ever called before Providers has mounted — for example in a unit test, a server-side render path, or during an early auth error before the provider tree is assembled — activePersister is still null and the void removePersistedCache() call silently no-ops, leaving the on-disk blob intact. A project switch or logout in that window would not wipe the cache it was supposed to wipe.
| describe("shouldPersistCanvasQuery", () => { | ||
| it("persists the channels list", () => { | ||
| expect(shouldPersistCanvasQuery(fakeQuery(["canvas-channels"]))).toBe(true); | ||
| }); | ||
|
|
||
| it("persists tRPC dashboards.list and dashboards.get", () => { | ||
| expect( | ||
| shouldPersistCanvasQuery( | ||
| fakeQuery([["dashboards", "list"], { input: {}, type: "query" }]), | ||
| ), | ||
| ).toBe(true); | ||
| expect( | ||
| shouldPersistCanvasQuery( | ||
| fakeQuery([["dashboards", "get"], { input: {}, type: "query" }]), | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("persists tRPC channelTasks.list", () => { | ||
| expect( | ||
| shouldPersistCanvasQuery( | ||
| fakeQuery([["channelTasks", "list"], { input: {}, type: "query" }]), | ||
| ), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("does not persist non-canvas queries", () => { | ||
| expect( | ||
| shouldPersistCanvasQuery(fakeQuery(["auth", "current-user", "us:2"])), | ||
| ).toBe(false); | ||
| expect( | ||
| shouldPersistCanvasQuery( | ||
| fakeQuery([["sessions", "list"], { input: {}, type: "query" }]), | ||
| ), | ||
| ).toBe(false); | ||
| expect( | ||
| shouldPersistCanvasQuery( | ||
| fakeQuery([["dashboards", "saveFreeform"], { input: {} }]), | ||
| ), | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it("does not persist queries that have not succeeded", () => { | ||
| expect( | ||
| shouldPersistCanvasQuery(fakeQuery(["canvas-channels"], "pending")), | ||
| ).toBe(false); | ||
| expect( | ||
| shouldPersistCanvasQuery(fakeQuery(["canvas-channels"], "error")), | ||
| ).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
it.each)
The team rule is to always prefer parameterised tests. The shouldPersistCanvasQuery suite has a natural table structure (query key → expected boolean) that maps directly to it.each. The current individual it blocks duplicate the call to shouldPersistCanvasQuery and fakeQuery per case. The same applies to the new channelTasksService.test.ts tests that could be parameterised over (channelPath provided? → getEntry called?) scenarios.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
… tests Greptile review follow-ups: - Pin the persist packages to react-query's resolved version (5.90.20) and add a pnpm override for @tanstack/query-core so the whole tree shares a single query-core. Previously the persist packages floated to 5.101.x and pulled a second query-core, which could make the persist/restore machinery operate on mismatched Query internals. - WebsiteDashboardsIndex: hold the dashboards query until channels have loaded so it fires once with the resolved channelPath. Avoids a cold-start double-fetch and loading flash from the query key changing when channelPath resolves. - Parameterise the new tests with it.each (predicate cases; service path-resolution cases) per the team convention. Generated-By: PostHog Code Task-Id: dc505d33-c219-41c9-ac6b-192caf7730ea
Unblocks local build/typecheck on this branch, which inherits the broken `sonner` import from main (FolderPicker imports an undeclared dependency). This is the same one-line fix as hotfix PR #2965; once that merges to main and main is merged back in, this resolves identically (no conflict). Kept here so the branch builds locally in the meantime. Generated-By: PostHog Code Task-Id: dc505d33-c219-41c9-ac6b-192caf7730ea
1 participant
Problem
Reloading the Channels (project-bluebird) space felt slow. On every reload the
channel sidebar, a channel's canvases, and its filed tasks all blanked out and
refetched from scratch — the in-memory query cache is destroyed on reload and
nothing is shown until the network resolves. On top of that, each per-channel
list made a redundant extra round-trip to resolve the channel's folder path.
Why: make the Channels space feel instant on reload instead of waiting on a
cold cache + chatty round-trips. (Backend/Redis wouldn't help here — the cost is
the cold cache and the round-trip waterfall from the desktop app, not backend
compute, so the fix is client-side.)
Changes
shared
PersistQueryClientProvider(desktop: async electron-store persister;web:
localStorage) restores channels/dashboards/channel-tasks from diskinstantly, then refetches in the background. Persistence is scoped to canvas
queries by key,
gcTimeis raised to match the persistmaxAge, and theon-disk blob is wiped on logout/project-switch (via
clearAuthScopedQueries)so project-scoped data never leaks across sessions.
dashboards.list/channelTasks.listresolved the channel's path with an extragetEntrybefore listing, even though the client already has it from
useChannels.Thread the known
channelPaththrough hook → router → service and skip theresolve (also halves
useTaskChannelMap's fan-out from N×2 to N).useChannelsnow matches its 30s poll, and the webQueryClientgets the same defaults as desktop.How did you test this?
list(channelId, channelPath)skipsgetEntrywhen a path is supplied and falls back when it isn't(
dashboardsService.test.ts, newchannelTasksService.test.ts); the persistallowlist predicate (
queryPersistence.test.ts).typecheckclean on the changed packages/apps (the only remaining error is apre-existing
sonnerimport inFolderPicker.tsx, unrelated to this change).biome checkclean on all changed files; host-boundary check reports no newviolations.
Channels space to confirm the instant-from-cache paint.
Automatic notifications
Created with PostHog Code