Skip to content

Commit 024d75b

Browse files
bburdabburda
committed
fix: address review round 3 - poll guard, progress clamping, busy state, runtime validation, mutation signals
1 parent c23b649 commit 024d75b

4 files changed

Lines changed: 74 additions & 30 deletions

File tree

‎src/components/UpdateCard.tsx‎

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export type UpdateAction = 'prepare' | 'execute' | 'automated' | 'delete';
2626
interface UpdateCardProps {
2727
entry: UpdateEntry;
2828
baseUrl?: string | null;
29+
busy?: boolean;
2930
onAction?: (id: string, action: UpdateAction) => void;
3031
}
3132

@@ -70,6 +71,10 @@ function actionButtonsForStatus(status: UpdateStatusValue): UpdateAction[] {
7071
}
7172
}
7273

74+
function clampProgress(value: number): number {
75+
return Math.min(100, Math.max(0, value));
76+
}
77+
7378
function actionLabel(action: UpdateAction): string {
7479
switch (action) {
7580
case 'prepare':
@@ -83,7 +88,7 @@ function actionLabel(action: UpdateAction): string {
8388
}
8489
}
8590

86-
export function UpdateCard({ entry, baseUrl, onAction }: UpdateCardProps) {
91+
export function UpdateCard({ entry, baseUrl, busy, onAction }: UpdateCardProps) {
8792
const { id, status } = entry;
8893
const [detailOpen, setDetailOpen] = useState(false);
8994
const [detail, setDetail] = useState<Record<string, unknown> | null>(null);
@@ -151,21 +156,25 @@ export function UpdateCard({ entry, baseUrl, onAction }: UpdateCardProps) {
151156

152157
{status !== null && status !== undefined && (
153158
<>
154-
{status.progress !== undefined && (
155-
<div
156-
role="progressbar"
157-
aria-label={`Progress for update ${id}`}
158-
aria-valuenow={status.progress}
159-
aria-valuemin={0}
160-
aria-valuemax={100}
161-
className="w-full h-2 rounded-full bg-muted overflow-hidden"
162-
>
163-
<div
164-
className={`h-full ${progressBarColor(status.status)} transition-all`}
165-
style={{ width: `${status.progress}%` }}
166-
/>
167-
</div>
168-
)}
159+
{status.progress !== undefined &&
160+
(() => {
161+
const clamped = clampProgress(status.progress);
162+
return (
163+
<div
164+
role="progressbar"
165+
aria-label={`Progress for update ${id}`}
166+
aria-valuenow={clamped}
167+
aria-valuemin={0}
168+
aria-valuemax={100}
169+
className="w-full h-2 rounded-full bg-muted overflow-hidden"
170+
>
171+
<div
172+
className={`h-full ${progressBarColor(status.status)} transition-all`}
173+
style={{ width: `${clamped}%` }}
174+
/>
175+
</div>
176+
);
177+
})()}
169178

170179
{status.sub_progress && status.sub_progress.length > 0 && (
171180
<ul className="space-y-1">
@@ -175,10 +184,12 @@ export function UpdateCard({ entry, baseUrl, onAction }: UpdateCardProps) {
175184
<div className="flex-1 h-1.5 rounded-full bg-muted overflow-hidden">
176185
<div
177186
className={`h-full ${progressBarColor(status.status)}`}
178-
style={{ width: `${sub.progress}%` }}
187+
style={{ width: `${clampProgress(sub.progress)}%` }}
179188
/>
180189
</div>
181-
<span className="w-9 text-right tabular-nums">{sub.progress}%</span>
190+
<span className="w-9 text-right tabular-nums">
191+
{clampProgress(sub.progress)}%
192+
</span>
182193
</li>
183194
))}
184195
</ul>
@@ -198,6 +209,7 @@ export function UpdateCard({ entry, baseUrl, onAction }: UpdateCardProps) {
198209
key={action}
199210
size="sm"
200211
variant={action === 'delete' ? 'destructive' : 'outline'}
212+
disabled={busy}
201213
onClick={() => onAction(id, action)}
202214
>
203215
{actionLabel(action)}

‎src/components/UpdatesDashboard.tsx‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
import { useCallback, useMemo } from 'react';
15+
import { useCallback, useMemo, useState } from 'react';
1616
import { useShallow } from 'zustand/shallow';
1717
import { Package, RefreshCw, AlertTriangle, Server } from 'lucide-react';
1818
import { toast } from 'react-toastify';
@@ -37,6 +37,7 @@ export function UpdatesDashboard() {
3737
const baseUrl = isConnected && serverUrl ? normalizeBaseUrl(serverUrl) : null;
3838

3939
const { updates, isLoading, error, notAvailable, refresh } = useUpdatesPolling(baseUrl);
40+
const [busyIds, setBusyIds] = useState<Set<string>>(new Set());
4041

4142
const summary = useMemo(() => {
4243
let active = 0;
@@ -58,6 +59,7 @@ export function UpdatesDashboard() {
5859
const confirmed = window.confirm(`Delete update "${id}"? This cannot be undone.`);
5960
if (!confirmed) return;
6061
}
62+
setBusyIds((prev) => new Set(prev).add(id));
6163
try {
6264
if (action === 'prepare') await triggerPrepare(baseUrl, id);
6365
else if (action === 'execute') await triggerExecute(baseUrl, id);
@@ -67,6 +69,12 @@ export function UpdatesDashboard() {
6769
refresh();
6870
} catch (err) {
6971
toast.error(err instanceof Error ? err.message : String(err));
72+
} finally {
73+
setBusyIds((prev) => {
74+
const next = new Set(prev);
75+
next.delete(id);
76+
return next;
77+
});
7078
}
7179
},
7280
[baseUrl, refresh]
@@ -167,7 +175,13 @@ export function UpdatesDashboard() {
167175
{header}
168176
<div className="grid gap-4 md:grid-cols-2">
169177
{updates.map((entry) => (
170-
<UpdateCard key={entry.id} entry={entry} baseUrl={baseUrl} onAction={handleAction} />
178+
<UpdateCard
179+
key={entry.id}
180+
entry={entry}
181+
baseUrl={baseUrl}
182+
busy={busyIds.has(entry.id)}
183+
onAction={handleAction}
184+
/>
171185
))}
172186
</div>
173187
</div>

‎src/hooks/useUpdatesPolling.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,20 @@ export function useUpdatesPolling(baseUrl: string | null, intervalMs?: number):
4141
const hasLoadedRef = useRef(false);
4242
// AbortController for in-flight fetches
4343
const abortRef = useRef<AbortController | null>(null);
44+
const fetchingRef = useRef(false);
4445
const [refreshTick, setRefreshTick] = useState(0);
4546

4647
const doFetch = useCallback(
47-
async (isInitial: boolean) => {
48+
async (isInitial: boolean, force: boolean = false) => {
4849
if (!baseUrl) return;
4950

51+
// Skip if a fetch is already in flight (prevents starvation on slow networks)
52+
if (fetchingRef.current && !force) return;
53+
5054
abortRef.current?.abort();
5155
const controller = new AbortController();
5256
abortRef.current = controller;
57+
fetchingRef.current = true;
5358

5459
if (isInitial && !hasLoadedRef.current) {
5560
setIsLoading(true);
@@ -90,6 +95,7 @@ export function useUpdatesPolling(baseUrl: string | null, intervalMs?: number):
9095
setError(err instanceof Error ? err.message : String(err));
9196
}
9297
} finally {
98+
fetchingRef.current = false;
9399
if (!controller.signal.aborted) {
94100
setIsLoading(false);
95101
}
@@ -113,7 +119,7 @@ export function useUpdatesPolling(baseUrl: string | null, intervalMs?: number):
113119
if (!isVisible) return;
114120

115121
const isInitial = !hasLoadedRef.current;
116-
void doFetch(isInitial);
122+
void doFetch(isInitial, true);
117123

118124
return () => {
119125
abortRef.current?.abort();

‎src/lib/updates-api.ts‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,19 @@ function updatePath(baseUrl: string, id: string, suffix?: string): string {
5151
export async function fetchUpdateIds(baseUrl: string, signal?: AbortSignal): Promise<string[]> {
5252
const res = await fetch(`${baseUrl}/updates`, { signal });
5353
await ensureOk(res);
54-
const data: { items: string[] } = await res.json();
55-
return data.items;
54+
const data = await res.json();
55+
return Array.isArray(data?.items) ? data.items : [];
5656
}
5757

5858
/** GET /updates/{id}/status - returns update status with progress */
5959
export async function fetchUpdateStatus(baseUrl: string, id: string, signal?: AbortSignal): Promise<UpdateStatus> {
6060
const res = await fetch(updatePath(baseUrl, id, 'status'), { signal });
6161
await ensureOk(res);
62-
return res.json();
62+
const data = await res.json();
63+
if (typeof data?.status !== 'string') {
64+
throw new UpdatesApiError('Invalid status response', 0);
65+
}
66+
return data as UpdateStatus;
6367
}
6468

6569
/** GET /updates/{id} - returns plugin-defined detail object */
@@ -74,37 +78,45 @@ export async function fetchUpdateDetail(
7478
}
7579

7680
/** PUT /updates/{id}/prepare - start preparation (202) */
77-
export async function triggerPrepare(baseUrl: string, id: string, data?: unknown): Promise<void> {
81+
export async function triggerPrepare(baseUrl: string, id: string, data?: unknown, signal?: AbortSignal): Promise<void> {
7882
const res = await fetch(updatePath(baseUrl, id, 'prepare'), {
7983
method: 'PUT',
8084
headers: { 'Content-Type': 'application/json' },
8185
body: JSON.stringify(data ?? {}),
86+
signal,
8287
});
8388
await ensureOk(res);
8489
}
8590

8691
/** PUT /updates/{id}/execute - start execution (202) */
87-
export async function triggerExecute(baseUrl: string, id: string, data?: unknown): Promise<void> {
92+
export async function triggerExecute(baseUrl: string, id: string, data?: unknown, signal?: AbortSignal): Promise<void> {
8893
const res = await fetch(updatePath(baseUrl, id, 'execute'), {
8994
method: 'PUT',
9095
headers: { 'Content-Type': 'application/json' },
9196
body: JSON.stringify(data ?? {}),
97+
signal,
9298
});
9399
await ensureOk(res);
94100
}
95101

96102
/** PUT /updates/{id}/automated - start automated update (202) */
97-
export async function triggerAutomated(baseUrl: string, id: string, data?: unknown): Promise<void> {
103+
export async function triggerAutomated(
104+
baseUrl: string,
105+
id: string,
106+
data?: unknown,
107+
signal?: AbortSignal
108+
): Promise<void> {
98109
const res = await fetch(updatePath(baseUrl, id, 'automated'), {
99110
method: 'PUT',
100111
headers: { 'Content-Type': 'application/json' },
101112
body: JSON.stringify(data ?? {}),
113+
signal,
102114
});
103115
await ensureOk(res);
104116
}
105117

106118
/** DELETE /updates/{id} - remove update (204) */
107-
export async function deleteUpdate(baseUrl: string, id: string): Promise<void> {
108-
const res = await fetch(updatePath(baseUrl, id), { method: 'DELETE' });
119+
export async function deleteUpdate(baseUrl: string, id: string, signal?: AbortSignal): Promise<void> {
120+
const res = await fetch(updatePath(baseUrl, id), { method: 'DELETE', signal });
109121
await ensureOk(res);
110122
}

0 commit comments

Comments
 (0)