Dashboard: Address review feedback on backup components

yashwin · claude · yashwin · commit add48e7ac38c · 2026-07-07T13:05:53.000+05:30
* Derive the backup selection and viewport once in useBackupsBrowserState,
  and key the mobile-detail chrome off the resolved backup so the page header
  and the browser body can't disagree.
* Move the download-id query-param cleanup into the route-aware shell and drop
  the callback from the shared BackupDownloadFlow.
* Normalize gmt_offset to a number in api-core's fetchSiteSettings and read the
  site settings directly, removing the per-call-site coercion.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
diff --git a/client/dashboard/sites/backup-download/flow.tsx b/client/dashboard/sites/backup-download/flow.tsx
@@ -4,7 +4,7 @@ import { __experimentalVStack as VStack } from '@wordpress/components';
 import { useDispatch } from '@wordpress/data';
 import { __ } from '@wordpress/i18n';
 import { store as noticesStore } from '@wordpress/notices';
-import { useState, useEffect } from 'react';
+import { useState } from 'react';
 import { useAnalytics } from '../../app/analytics';
 import { Card, CardBody } from '../../components/card';
 import { useFormattedTime } from '../../components/formatted-time';
@@ -20,21 +20,14 @@ export function BackupDownloadFlow( {
 	site,
 	rewindId,
 	initialDownloadId,
-	onDownloadIdConsumed,
 }: {
 	site: Site;
 	rewindId: string;
 	initialDownloadId?: number;
-	onDownloadIdConsumed?: () => void;
 } ) {
-	const { data: siteSettings } = useSuspenseQuery( {
-		...siteSettingsQuery( site.ID ),
-		select: ( s ) => ( {
-			gmtOffset: Number( s?.gmt_offset ) || 0,
-			timezoneString: s?.timezone_string || undefined,
-		} ),
-	} );
-	const { gmtOffset, timezoneString } = siteSettings;
+	const { data: siteSettings } = useSuspenseQuery( siteSettingsQuery( site.ID ) );
+	const gmtOffset = siteSettings.gmt_offset;
+	const timezoneString = siteSettings.timezone_string;
 
 	const initialStep: DownloadStep = initialDownloadId ? 'progress' : 'form';
 
@@ -45,13 +38,6 @@ export function BackupDownloadFlow( {
 	const { createSuccessNotice } = useDispatch( noticesStore );
 	const { recordTracksEvent } = useAnalytics();
 
-	// Clean up the downloadId query param once we've captured it.
-	useEffect( () => {
-		if ( initialDownloadId ) {
-			onDownloadIdConsumed?.();
-		}
-	}, [ initialDownloadId, onDownloadIdConsumed ] );
-
 	const handleDownloadInitiate = ( newDownloadId: number ) => {
 		recordTracksEvent( 'calypso_dashboard_backups_download_started' );
 		setCurrentStep( 'progress' );
diff --git a/client/dashboard/sites/backup-download/index.tsx b/client/dashboard/sites/backup-download/index.tsx
@@ -2,7 +2,7 @@ import { siteBySlugQuery } from '@automattic/api-queries';
 import { useSuspenseQuery } from '@tanstack/react-query';
 import { useRouter } from '@tanstack/react-router';
 import { __ } from '@wordpress/i18n';
-import { useCallback } from 'react';
+import { useEffect } from 'react';
 import Breadcrumbs from '../../app/breadcrumbs';
 import { siteBackupDownloadRoute } from '../../app/router/sites';
 import { PageHeader } from '../../components/page-header';
@@ -15,14 +15,16 @@ function SiteBackupDownload() {
 	const { data: site } = useSuspenseQuery( siteBySlugQuery( siteSlug ) );
 	const router = useRouter();
 
-	const handleDownloadIdConsumed = useCallback( () => {
-		router.navigate( {
-			to: siteBackupDownloadRoute.fullPath,
-			params: { siteSlug, rewindId },
-			search: {},
-			replace: true,
-		} );
-	}, [ router, siteSlug, rewindId ] );
+	useEffect( () => {
+		if ( searchDownloadId ) {
+			router.navigate( {
+				to: siteBackupDownloadRoute.fullPath,
+				params: { siteSlug, rewindId },
+				search: {},
+				replace: true,
+			} );
+		}
+	}, [ router, siteSlug, rewindId, searchDownloadId ] );
 
 	return (
 		<PageLayout
@@ -35,7 +37,6 @@ function SiteBackupDownload() {
 				site={ site }
 				rewindId={ rewindId }
 				initialDownloadId={ searchDownloadId }
-				onDownloadIdConsumed={ handleDownloadIdConsumed }
 			/>
 		</PageLayout>
 	);
diff --git a/client/dashboard/sites/backup-restore/flow.tsx b/client/dashboard/sites/backup-restore/flow.tsx
@@ -19,14 +19,9 @@ import type { Site } from '@automattic/api-core';
 type RestoreStep = 'form' | 'progress' | 'success' | 'error';
 
 export function BackupRestoreFlow( { site, rewindId }: { site: Site; rewindId: string } ) {
-	const { data: siteSettings } = useSuspenseQuery( {
-		...siteSettingsQuery( site.ID ),
-		select: ( s ) => ( {
-			gmtOffset: Number( s?.gmt_offset ) || 0,
-			timezoneString: s?.timezone_string || undefined,
-		} ),
-	} );
-	const { gmtOffset, timezoneString } = siteSettings;
+	const { data: siteSettings } = useSuspenseQuery( siteSettingsQuery( site.ID ) );
+	const gmtOffset = siteSettings.gmt_offset;
+	const timezoneString = siteSettings.timezone_string;
 
 	const [ currentStep, setCurrentStep ] = useState< RestoreStep >( 'form' );
 	const [ restoreId, setRestoreId ] = useState< number | null >( null );
diff --git a/client/dashboard/sites/backups/backups-browser.tsx b/client/dashboard/sites/backups/backups-browser.tsx
@@ -1,6 +1,6 @@
 import { __experimentalGrid as Grid } from '@wordpress/components';
 import { useViewportMatch } from '@wordpress/compose';
-import { useState, useEffect } from 'react';
+import { useMemo } from 'react';
 import { PerformanceTrackerStop } from '../../app/performance-tracking';
 import { Card, CardBody } from '../../components/card';
 import { BackupDetails } from './backup-details';
@@ -13,12 +13,54 @@ export function useIsBackupsSmallViewport() {
 	return useViewportMatch( 'xlarge', '<' );
 }
 
-interface BackupsBrowserProps {
+// Single owner of the resolved selection so the page header and browser body can't disagree.
+export function useBackupsBrowserState( {
+	site,
+	rewindId,
+	dateRange,
+	timezoneString,
+	gmtOffset,
+	enabled,
+}: {
 	site: Site;
 	rewindId?: string;
 	dateRange?: { start: Date; end: Date };
 	timezoneString?: string;
 	gmtOffset?: number;
+	enabled?: boolean;
+} ) {
+	const isSmallViewport = useIsBackupsSmallViewport();
+
+	const { activityLog, isLoadingActivityLog } = useActivityLog( {
+		siteId: site.ID,
+		dateRange,
+		gmtOffset,
+		timezoneString,
+		enabled,
+	} );
+
+	const selectedBackup = useMemo< ActivityLogEntry | null >( () => {
+		if ( rewindId ) {
+			return activityLog?.find( ( item ) => item.rewind_id === rewindId ) ?? null;
+		}
+		if ( ! isSmallViewport ) {
+			return activityLog?.[ 0 ] ?? null;
+		}
+		return null;
+	}, [ rewindId, activityLog, isSmallViewport ] );
+
+	return { activityLog, isLoadingActivityLog, selectedBackup, isSmallViewport };
+}
+
+interface BackupsBrowserProps {
+	site: Site;
+	activityLog: ActivityLogEntry[];
+	isLoadingActivityLog: boolean;
+	selectedBackup: ActivityLogEntry | null;
+	isSmallViewport: boolean;
+	dateRange?: { start: Date; end: Date };
+	timezoneString?: string;
+	gmtOffset?: number;
 	searchParams?: Record< string, unknown >;
 	onSelectBackup: ( backup: ActivityLogEntry | null ) => void;
 	onRequestRestore?: ( backup: ActivityLogEntry ) => void;
@@ -28,7 +70,10 @@ interface BackupsBrowserProps {
 
 export function BackupsBrowser( {
 	site,
-	rewindId,
+	activityLog,
+	isLoadingActivityLog,
+	selectedBackup,
+	isSmallViewport,
 	dateRange,
 	timezoneString,
 	gmtOffset,
@@ -38,37 +83,6 @@ export function BackupsBrowser( {
 	onRequestDownload,
 	onGranularDownloadReady,
 }: BackupsBrowserProps ) {
-	const isSmallViewport = useIsBackupsSmallViewport();
-	const [ selectedBackup, setSelectedBackupInState ] = useState< ActivityLogEntry | null >( null );
-
-	const { activityLog, isLoadingActivityLog } = useActivityLog( {
-		siteId: site.ID,
-		dateRange,
-		gmtOffset,
-		timezoneString,
-	} );
-
-	// Auto-select backup based on rewindId parameter.
-	useEffect( () => {
-		if ( rewindId && activityLog ) {
-			const targetBackup = activityLog.find( ( item ) => item.rewind_id === rewindId );
-			if ( targetBackup ) {
-				setSelectedBackupInState( targetBackup );
-			}
-			return;
-		}
-
-		// Select the first backup on the index route (desktop only) to keep the panel populated.
-		const backup = activityLog?.[ 0 ];
-		if ( ! rewindId && backup && ! isSmallViewport ) {
-			setSelectedBackupInState( backup );
-		}
-
-		if ( ! rewindId && ! backup ) {
-			setSelectedBackupInState( null );
-		}
-	}, [ rewindId, activityLog, isSmallViewport ] );
-
 	const renderDetails = ( backup: ActivityLogEntry ) => (
 		<BackupDetails
 			backup={ backup }
diff --git a/client/dashboard/sites/backups/index.tsx b/client/dashboard/sites/backups/index.tsx
@@ -28,7 +28,7 @@ import HostingFeatureGatedWithCallout from '../hosting-feature-gated-with-callou
 import { SitesNoticeArbiter } from '../notice-arbiter';
 import { BackupNotices } from './backup-notices';
 import { BackupNowButton } from './backup-now-button';
-import { BackupsBrowser, useIsBackupsSmallViewport } from './backups-browser';
+import { BackupsBrowser, useBackupsBrowserState } from './backups-browser';
 import illustrationUrl from './backups-callout-illustration.svg';
 import { useBackupState } from './use-backup-state';
 import './style.scss';
@@ -48,14 +48,9 @@ export function BackupsListPage() {
 	const searchParams = siteBackupsRoute.useSearch();
 	const backupState = useBackupState( site.ID );
 
-	const { data: siteSettings } = useSuspenseQuery( {
-		...siteSettingsQuery( site.ID ),
-		select: ( s ) => ( {
-			gmtOffset: Number( s?.gmt_offset ) || 0,
-			timezoneString: s?.timezone_string || undefined,
-		} ),
-	} );
-	const { gmtOffset, timezoneString } = siteSettings;
+	const { data: siteSettings } = useSuspenseQuery( siteSettingsQuery( site.ID ) );
+	const gmtOffset = siteSettings.gmt_offset;
+	const timezoneString = siteSettings.timezone_string;
 
 	const { dateRange, handleDateRangeChange } = useDateRange( {
 		timezoneString,
@@ -109,9 +104,17 @@ export function BackupsListPage() {
 		navigateToBackup( null );
 	};
 
-	const isSmallViewport = useIsBackupsSmallViewport();
 	const hasBackups = hasHostingFeature( site, HostingFeatures.BACKUPS_SELF_SERVE );
-	const isMobileDetailsView = isSmallViewport && !! rewindId;
+	const { activityLog, isLoadingActivityLog, selectedBackup, isSmallViewport } =
+		useBackupsBrowserState( {
+			site,
+			rewindId,
+			dateRange,
+			timezoneString,
+			gmtOffset,
+			enabled: hasBackups,
+		} );
+	const isMobileDetailsView = isSmallViewport && !! selectedBackup;
 	const shouldShowActions = hasBackups && ! isMobileDetailsView;
 
 	const actions = (
@@ -162,7 +165,10 @@ export function BackupsListPage() {
 			{ hasBackups ? (
 				<BackupsBrowser
 					site={ site }
-					rewindId={ rewindId }
+					activityLog={ activityLog }
+					isLoadingActivityLog={ isLoadingActivityLog }
+					selectedBackup={ selectedBackup }
+					isSmallViewport={ isSmallViewport }
 					dateRange={ dateRange }
 					timezoneString={ timezoneString }
 					gmtOffset={ gmtOffset }
diff --git a/client/dashboard/sites/backups/use-activity-log.ts b/client/dashboard/sites/backups/use-activity-log.ts
@@ -10,6 +10,7 @@ export interface UseActivityLogOptions {
 	dateRange?: { start: Date; end: Date };
 	timezoneString?: string;
 	gmtOffset?: number;
+	enabled?: boolean;
 }
 
 export interface UseActivityLogResult {
@@ -25,6 +26,7 @@ export function useActivityLog( {
 	timezoneString,
 	gmtOffset,
 	dateRange,
+	enabled = true,
 }: UseActivityLogOptions ): UseActivityLogResult {
 	const { backup, hasRecentlyCompleted } = useBackupState( siteId );
 
@@ -43,6 +45,7 @@ export function useActivityLog( {
 
 	const queryResult = useQuery( {
 		...siteBackupActivityLogEntriesQuery( siteId, undefined, true, after, before ),
+		enabled,
 		refetchInterval: ( query ) => {
 			if ( ! backup || ! hasRecentlyCompleted ) {
 				return false;
diff --git a/packages/api-core/src/site-settings/fetchers.ts b/packages/api-core/src/site-settings/fetchers.ts
@@ -6,5 +6,8 @@ export async function fetchSiteSettings( siteId: number ): Promise< SiteSettings
 		path: `/sites/${ siteId }/settings`,
 		apiVersion: '1.4',
 	} );
-	return settings;
+	return {
+		...settings,
+		gmt_offset: Number( settings.gmt_offset ) || 0,
+	};
 }
diff --git a/packages/api-core/src/site-settings/types.ts b/packages/api-core/src/site-settings/types.ts
@@ -1,7 +1,7 @@
 export interface SiteSettings {
 	blog_public?: number;
 	is_fully_managed_agency_site?: boolean;
-	gmt_offset?: number | string;
+	gmt_offset?: number;
 	timezone_string?: string;
 	/**
 	 * @deprecated Use `wpcom_public_coming_soon` instead.
