fix(ui): stop stale PR status leaking across cloud task switches - #4066
fix(ui): stop stale PR status leaking across cloud task switches#4066posthog[bot] wants to merge 1 commit into
Conversation
`useTaskPrStatus` disables its query for a cloud task with no `cloudPrUrl` yet, but `placeholderData: (prev) => prev` keeps serving the previously-selected task's resolved data — a disabled query never fetches to replace it, so the stale PR state and CI status persist indefinitely. Guard the disabled case explicitly so a freshly-selected task with no PR never surfaces another task's leftover PR/CI status. Generated-By: PostHog Code Task-Id: 4ee8babf-75b1-4574-a1e7-2cddf1c96134
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 1 issue
Files (1)
packages/ui/src/features/sidebar/useTaskPrStatus.ts
Other findings (outside the changed lines)
Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.
Fix only covers the disabled-query case; placeholderData can still leak a prior task's PR status while an enabled query is in flight
Priority: should_fix | File: packages/ui/src/features/sidebar/useTaskPrStatus.ts:27-38 | Category: bug
Why we think it's a valid issue
- Checked:
useTaskPrStatus.tsfull body, theplaceholderData/enabled/guard interaction, the query-key shape inworkspace.router.ts(getTaskPrStatusinput{ taskId, cloudPrUrl }), and the call sites (TaskListView,ReviewShell,TaskTabIcon,CommandMenu/TaskCommandIcon,CommandCenterPanel/CellStatusBadge,ChannelFeedView). - Found:
placeholderData: (prev) => prev(line 32) is TanStack v5 keepPreviousData — on a reused observer whose queryKey changes it fillsdatafrom the prior key's resolved value during the new fetch. Since the key is{ taskId, cloudPrUrl }, every task switch is a key change. The new guard at line 38 only tests emptiness, andskipQuery(line 24) isfalsefor an enabled→enabled transition, soif (!data || ...)returns the previous task's populateddatawhile the newly-selected (enabled, not-yet-fetched) task's query is in flight. The disabled-query fix does not touch this path. - Found: Reachability holds where the same hook fiber's
taskprop changes in place rather than remounting — the review surface (ReviewShellwhosetaskfollows the selected task) and other single-instance consumers, distinct from the per-row-keyedTaskListView.tsx:98. At least one genuine in-place reuse path exists, so it is not purely speculative. - Impact: During the new task's first fetch the wrong task's PR/CI badge (e.g. a stale
open/merged/closed) renders on it, then self-corrects. Transient and milder than the indefinite disabled-query leak the PR fixes, but the same bug class, in the exact hook under change, and it undercuts the PR's stated goal of stopping stale PR status leaking across task switches. Concrete trigger and concrete consequence are both nameable, and the suggested(prev, prevQuery)scoping is a correct, cheap fix — clears the bar.
Issue description
The new guard (if (skipQuery || !data || ...) return EMPTY;) only suppresses stale data for the permanently-disabled-query case. The root cause it's patching around — placeholderData: (prev) => prev (line 32) — carries over the last successfully resolved data for this hook instance across any query-key change, not just across disabled queries. getTaskPrStatus.queryOptions keys the query by { taskId, cloudPrUrl } (workspace.router.ts), so every task switch is a query-key change. For hook call sites that reuse the same component/hook instance across different tasks (e.g. ChannelFeedView.tsx, ReviewShell.tsx, CommandMenu.tsx, CommandCenterPanel.tsx, TaskTabIcon.tsx — all of which pass a task whose identity changes over the component's lifetime, unlike the per-row instances in TaskListView.tsx), switching from a task whose query already resolved (prState/hasDiff populated) to a different, enabled task whose query key has not been fetched yet will still render the old task's PR/CI status as data (via placeholderData) for the duration of the new fetch, because skipQuery is false and data is non-empty. This is the same class of bug described in the PR intent (stale PR/CI status attributed to the wrong task), just transient (until the new fetch resolves) instead of indefinite. Under network latency or backend slowness this window is not negligible and can visibly show incorrect CI/PR state (e.g. a red 'closed' or stale 'open' badge) for a task that hasn't even loaded its own status yet.
Suggested fix
Scope the carried-over placeholder to the same task instead of blindly reusing the previous observer's data. placeholderData in TanStack Query v5 receives (previousData, previousQuery) as its second argument, where previousQuery exposes the prior query's key/input. Use that to only keep prev when it belongs to the same task.id (and cloudPrUrl), e.g.:
placeholderData: (prev, prevQuery) => {
const prevInput = prevQuery?.queryKey?.[1]?.input as { taskId?: string } | undefined;
return prevInput?.taskId === task.id ? prev : undefined;
},Alternatively, track the previous task.id in a ref and reset to EMPTY/undefined whenever it changes, before the new query settles. This closes the same leak path this PR is trying to fix, for the enabled-query transition rather than only the disabled-query one.
Problem
useTaskPrStatusdisables its query for a cloud task with nocloudPrUrlyet (enabled: !skipQuery), but itsplaceholderData: (prev) => prevkeeps serving whatever the previously-selected task's query resolved to.Changes
useTaskPrStatus(packages/ui/src/features/sidebar/useTaskPrStatus.ts) so it always returns empty status when the query is disabled, instead of trusting whateverdataTanStack Query left behind.How did you test this?
cloudPrUrl: null— asserts the leaked data is not returned.useTaskPrStatus.test.tssuite (9/9 passing).pnpm --filter @posthog/ui typecheck— no new errors introduced by this change (pre-existing unrelated errors from unbuilt@posthog/git/@posthog/platformpackages in the environment).Automatic notifications
Created with PostHog Desktop from this inbox report.