fix(desktop): prevent artifact version flicker - #80229
Conversation
Keep the current artifact preview mounted while the next version loads. Disable version-specific actions until the replacement preview is ready. Generated-By: PostHog Code Task-Id: 4fa3fef3-16a1-4806-9b8e-0fac3f0057ae
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
puemos
left a comment
There was a problem hiding this comment.
Merge Confidence: 4.0/5 - Confident
"Looks good - minor concerns only"
Assessment
- ✓ Well-scoped single-concern change: replace the loading spinner on artifact version navigation with the previously-loaded document, across exactly 2 source files and 1 test
- ✓ The correctness argument is handled — every write/navigate affordance that could act on stale content is gated (
canEditviaversionsLoading, both version-nav buttons,commentsEnabled) - ✓ Additive API change only:
useArtifactPreviewDatagainsisPlaceholderData; repo search confirmsArtifactPreview.tsxis the sole consumer - ✓ Error path verified safe: TanStack v5 only applies
placeholderDatawhile status ispending, so a failed fetch drops the placeholder, releases the guards, and still rendersArtifactPreviewError(ArtifactPreview.tsx:294) - ⚠
commentsEnabled={commentsEnabled && !isPlaceholderData}feeds document generation, not just UI —AnnotatedArtifactHtml.tsx:75-82memoisespreviewDocumenton it andartifactHtmlFrame.tsx:33-39rebuilds the iframe blob URL, so HTML artifacts now take two full iframe reloads per version switch, partly undoing the flicker fix this PR is for - ⚠
placeholderData: (previousData) => previousDatais unscoped: because each version is a distinctartifactIdin the query key, selecting an unrelated artifact from the sidebar also renders the prior artifact's body under the new header - ⚠ The test's
useQuerymock keepsartifact-0in the placeholder state permanently, so it proves the guards engage but never that they release — a stuck-disabled state would pass - ⚠
commentTarget/commentsQuery(ArtifactPreview.tsx:135-139) and the derivedcomments/commentLoadError(lines 190-193) still key on the newly-selected version and are gated on the ungatedcommentsEnabled; worth confirming nothing renders them against the stale body
Review: Keep previous artifact content visible during version navigation via React Query placeholderData
This PR replaces the loading spinner shown while navigating between artifact versions with the previously-loaded document, by adding placeholderData: (previousData) => previousData to the artifactPreview query and surfacing isPlaceholderData up into ArtifactPreview. Because the pane now renders content that doesn't match the selected version, the change also gates the affordances that would act on it: editing (versionsLoading: runsLoading || isPlaceholderData), both version-nav buttons, and the comment surface.
The core approach is sound and the guard set covers the write paths. Three things are worth attention before merge.
First, commentsEnabled is load-bearing for document generation, not just presentation — AnnotatedArtifactHtml.tsx:75-82 memoises previewDocument on it and artifactHtmlFrame.tsx:33-39 converts that into the iframe's blob src. Deriving it from isPlaceholderData therefore causes two full iframe reloads per version switch for HTML artifacts, which works against the flicker reduction this PR is chasing.
Second, the placeholder isn't scoped to version steps: each version is its own artifactId in the query key, so selecting an unrelated artifact from the sidebar also renders the prior artifact's body under the new header.
Third, the new test's useQuery mock keeps the fixture in the placeholder state permanently, so it verifies the guards engaging but never releasing — the stuck-disabled failure mode would pass unnoticed.
The error path checks out: TanStack v5 applies placeholderData only while status is pending, so a failed fetch drops the placeholder, re-enables the arrows, and falls through to ArtifactPreviewError. No security, breaking-change, or hooks issues found.
Scope preview placeholders to versions of the same artifact name. Keep comments bound to the displayed version, and cover transition completion and guarded actions. Generated-By: PostHog Code Task-Id: 4fa3fef3-16a1-4806-9b8e-0fac3f0057ae
There was a problem hiding this comment.
Contained frontend UX fix to a loading-state flicker in the desktop artifact preview; not risky territory, and all four substantive inline review concerns (stale-placeholder test gap, missing edit/comment guard coverage, extra iframe reloads, cross-artifact cache bleed) were fixed in a follow-up commit and verified present in the diff.
- Author wrote 96% of the modified lines and has 7 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 75L, 2F substantive, 169L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (169L, 3F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ e081a98 · reviewed head 3539c7f |
Problem
People switching artifact versions see the preview pane flash to a spinner the first time each version loads.
Changes
How did you test this code?
ArtifactPreview.test.tsxto cover the loading state, its completion, guarded editing, stable comments, and unrelated artifacts.@posthog/uitype check, desktop Biome, host boundaries, andci:preflight.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
None. This change does not affect a documented workflow.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
An OpenAI coding agent worked through pi. It used
/writing-tests,/writing-pr-descriptions,/writing-simplified-technical-english, and/working-with-task-comments. The session link is unavailable.