From f6c6ffea150187b5a6786f2c1df423cc8f243a89 Mon Sep 17 00:00:00 2001 From: Anso Date: Tue, 2 Jun 2026 09:50:51 -0400 Subject: [PATCH] fix(blueprints): stop the deployment detail sheet flickering and show deploy progress (#1278) * fix(blueprints): stop the blueprint detail sheet flickering while open The Fleet > Deployments blueprint detail sheet reloaded its data on a short timer while open, flickering the body through its loading skeleton every few seconds. The sheet's load effect was keyed on the refresh callback, which was memoized with the parent's onOpenChange prop. The parent passes a fresh onOpenChange closure on every render, so each parent re-render (driven by the Fleet view's polling) recreated refresh, re-ran the load effect, and refetched the blueprint. Every refetch flipped the loading flag, swapping the populated body for skeletons and back. Hold onOpenChange in a ref so refresh depends only on blueprintId. The load effect now runs on open and on blueprint change, not on every parent render. Switching to a different blueprint still refetches. * fix(blueprints): keep the blueprint detail body visible during a refresh After the sheet has loaded, a reload triggered by an action (apply, withdraw, accept, enable/disable, save) flipped the loading flag and swapped the whole body for skeletons until the reload finished, a brief flash on every action. Gate the skeleton on whether the blueprint has loaded yet, not on the loading flag. The skeleton now shows only on the first load; later reloads keep the populated body, deployment table, and compose source on screen while they run. * fix(blueprints): show progress while a fresh deploy runs Confirming a fresh deploy from the blueprint detail sheet starts a remote deploy that can take several seconds. The button only became disabled with no other change, so the action looked frozen. Show a spinner and a "Deploying" label on the button while the deploy runs, and dim it, so the click clearly registers as in progress. --- .../blueprints/BlueprintDetail.test.tsx | 75 ++++++++++++++++++- .../components/blueprints/BlueprintDetail.tsx | 14 +++- .../blueprints/StateReviewDialog.test.tsx | 30 ++++++++ .../blueprints/StateReviewDialog.tsx | 17 ++++- 4 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 frontend/src/components/blueprints/StateReviewDialog.test.tsx diff --git a/frontend/src/components/blueprints/BlueprintDetail.test.tsx b/frontend/src/components/blueprints/BlueprintDetail.test.tsx index 9bf51774..884dcf7c 100644 --- a/frontend/src/components/blueprints/BlueprintDetail.test.tsx +++ b/frontend/src/components/blueprints/BlueprintDetail.test.tsx @@ -7,7 +7,7 @@ * of them, so the sheet can never issue a request the API answers with 403. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen } from '@testing-library/react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; import type { BlueprintSummary } from '@/lib/blueprintsApi'; vi.mock('@/lib/blueprintsApi', async (importOriginal) => { @@ -57,6 +57,79 @@ beforeEach(() => { vi.mocked(getBlueprint).mockResolvedValue(summary()); }); +describe('BlueprintDetail data fetching', () => { + it('does not refetch when the parent re-renders with new callback identities', async () => { + const { rerender } = render( + {}} onChanged={noop} canEdit distinctLabels={[]} />, + ); + + // Let the initial load settle so the body content is on screen. + expect(await screen.findByText('Show compose source')).toBeInTheDocument(); + const callsAfterLoad = vi.mocked(getBlueprint).mock.calls.length; + + // A parent re-render (e.g. the Fleet view's polling) hands the open sheet a + // brand-new onOpenChange closure every render. Before the fix that closure was + // a refresh dependency, so the load effect re-ran on every parent render and + // flickered the body through its loading skeleton. It must now keep showing the + // data it already has instead of refetching. + rerender( + {}} onChanged={noop} canEdit distinctLabels={[]} />, + ); + rerender( + {}} onChanged={noop} canEdit distinctLabels={[]} />, + ); + await Promise.resolve(); + + expect(vi.mocked(getBlueprint)).toHaveBeenCalledTimes(callsAfterLoad); + }); + + it('refetches when blueprintId changes while the sheet stays open', async () => { + const { rerender } = render( + , + ); + expect(await screen.findByText('Show compose source')).toBeInTheDocument(); + const callsAfterLoad = vi.mocked(getBlueprint).mock.calls.length; + + // Opening a different blueprint without closing the sheet must load the new one, + // so blueprintId has to stay a refresh dependency. + rerender( + , + ); + await screen.findByText('Show compose source'); + + expect(vi.mocked(getBlueprint)).toHaveBeenCalledTimes(callsAfterLoad + 1); + expect(vi.mocked(getBlueprint)).toHaveBeenLastCalledWith(2); + }); + + it('keeps the loaded body on screen while a refresh is in flight', async () => { + let settleRefresh = () => {}; + vi.mocked(getBlueprint) + .mockResolvedValueOnce(summary()) + .mockImplementationOnce( + () => new Promise((resolve) => { settleRefresh = () => resolve(summary()); }), + ); + + render( + , + ); + expect(await screen.findByText('Show compose source')).toBeInTheDocument(); + const callsAfterLoad = vi.mocked(getBlueprint).mock.calls.length; + + // Applying reloads the blueprint. The populated body must stay mounted instead + // of collapsing to the loading skeleton while that reload is in flight. + fireEvent.click(screen.getByRole('button', { name: /apply now/i })); + await waitFor(() => expect(vi.mocked(getBlueprint).mock.calls.length).toBe(callsAfterLoad + 1)); + + // The deployment table only renders in the loaded body branch, never in the + // skeleton, so its presence proves the skeleton did not take over the refresh. + expect(screen.getByText('Show compose source')).toBeInTheDocument(); + expect(screen.getByTestId('deployment-table')).toBeInTheDocument(); + + settleRefresh(); + await screen.findByText('Show compose source'); + }); +}); + describe('BlueprintDetail action gating', () => { it('shows the Apply / Edit / Delete actions for an admin (canEdit)', async () => { render( diff --git a/frontend/src/components/blueprints/BlueprintDetail.tsx b/frontend/src/components/blueprints/BlueprintDetail.tsx index 686e6997..a31f0c89 100644 --- a/frontend/src/components/blueprints/BlueprintDetail.tsx +++ b/frontend/src/components/blueprints/BlueprintDetail.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useCallback } from 'react'; +import { useEffect, useRef, useState, useCallback } from 'react'; import { Pencil, Pin, Play, Power, Trash2 } from 'lucide-react'; import { SystemSheet, SheetSection } from '@/components/ui/system-sheet'; import { Modal, ModalDestructiveHeader, ModalBody, ModalFooter } from '@/components/ui/modal'; @@ -48,6 +48,12 @@ export function BlueprintDetail({ blueprintId, open, onOpenChange, onChanged, ca const [deleteConfirmText, setDeleteConfirmText] = useState(''); const { nodes } = useNodes(); + // Hold the latest onOpenChange without making it a refresh dependency. Parents + // pass a fresh closure on every render, so binding refresh to it would re-run the + // load effect on each parent render and flicker the body through its skeleton. + const onOpenChangeRef = useRef(onOpenChange); + useEffect(() => { onOpenChangeRef.current = onOpenChange; }, [onOpenChange]); + const refresh = useCallback(async () => { setLoading(true); try { @@ -55,11 +61,11 @@ export function BlueprintDetail({ blueprintId, open, onOpenChange, onChanged, ca setSummary(result); } catch (err) { toast.error(err instanceof Error ? err.message : 'Failed to load blueprint'); - onOpenChange(false); + onOpenChangeRef.current(false); } finally { setLoading(false); } - }, [blueprintId, onOpenChange]); + }, [blueprintId]); useEffect(() => { if (open) { @@ -245,7 +251,7 @@ export function BlueprintDetail({ blueprintId, open, onOpenChange, onChanged, ca footerContext={footerContext} size="lg" > - {loading || !blueprint || !summary ? ( + {!blueprint || !summary ? (
diff --git a/frontend/src/components/blueprints/StateReviewDialog.test.tsx b/frontend/src/components/blueprints/StateReviewDialog.test.tsx new file mode 100644 index 00000000..34135e0e --- /dev/null +++ b/frontend/src/components/blueprints/StateReviewDialog.test.tsx @@ -0,0 +1,30 @@ +/** + * The fresh-deploy action triggers a remote deploy that can take several seconds. + * The dialog must show an in-progress indicator while it runs, otherwise the click + * looks like it did nothing. + */ +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { StateReviewDialog } from './StateReviewDialog'; + +const baseProps = { + open: true, + onOpenChange: () => {}, + blueprintName: 'heimdall', + nodeName: 'sencho-test-01', + onAccept: () => {}, +}; + +describe('StateReviewDialog', () => { + it('offers the fresh deploy action when idle', () => { + render(); + expect(screen.getByText('Deploy fresh')).toBeInTheDocument(); + expect(screen.queryByText(/deploying/i)).not.toBeInTheDocument(); + }); + + it('shows an in-progress indicator and disables the action while deploying', () => { + render(); + expect(screen.getByText(/deploying/i)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /deploying/i })).toBeDisabled(); + }); +}); diff --git a/frontend/src/components/blueprints/StateReviewDialog.tsx b/frontend/src/components/blueprints/StateReviewDialog.tsx index bb9968c7..bc1a45f3 100644 --- a/frontend/src/components/blueprints/StateReviewDialog.tsx +++ b/frontend/src/components/blueprints/StateReviewDialog.tsx @@ -1,4 +1,4 @@ -import { Database, Play } from 'lucide-react'; +import { Database, Loader2, Play } from 'lucide-react'; import { Modal, ModalHeader, ModalBody, ModalFooter } from '@/components/ui/modal'; import { Button } from '@/components/ui/button'; @@ -31,11 +31,20 @@ export function StateReviewDialog({ type="button" onClick={() => onAccept('fresh')} disabled={busy} - className="w-full text-left rounded-lg border border-card-border border-t-card-border-top bg-card hover:border-t-card-border-hover transition-colors p-3 cursor-pointer" + className="w-full text-left rounded-lg border border-card-border border-t-card-border-top bg-card hover:border-t-card-border-hover transition-colors p-3 cursor-pointer disabled:opacity-60 disabled:cursor-not-allowed" >
- - Deploy fresh + {busy ? ( + <> + + Deploying… + + ) : ( + <> + + Deploy fresh + + )}

Create empty named volumes on this node. The container starts with whatever default state its image carries.