From 02da7232b398266e32885be633fc3053f8b51b45 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 7 Sep 2026 08:24:28 -0300 Subject: [PATCH] refactor(dashboard): split compare-runs/history-tab functions to clear complexity ratchets CompareRunsPanel (92 lines, max-lines-per-function) and HistoryTab (85 lines, same rule) exceeded the 80-line function cap; compareRuns.ts's a2aEventsFrom exceeded the cognitive-complexity cap (16 > 15). Extract pure/presentational helpers (ComparePanelHeaderBar, SideErrorRow, ComparisonMetrics, a2aEventFrom, HistoryStatusRows, refreshNowMsOnActionDone) with no behavior, DOM, i18n or aria change. --- .../orchestration/model/compareRuns.ts | 27 ++- .../orchestration/tabs/CompareRunsPanel.tsx | 158 +++++++++++------- .../orchestration/tabs/HistoryTab.tsx | 70 +++++--- 3 files changed, 168 insertions(+), 87 deletions(-) diff --git a/src/app/(dashboard)/dashboard/orchestration/model/compareRuns.ts b/src/app/(dashboard)/dashboard/orchestration/model/compareRuns.ts index 2cfb3c6072..4caff76d7b 100644 --- a/src/app/(dashboard)/dashboard/orchestration/model/compareRuns.ts +++ b/src/app/(dashboard)/dashboard/orchestration/model/compareRuns.ts @@ -56,21 +56,30 @@ function isNonEmptyString(value: unknown): value is string { return typeof value === "string" && value.length > 0; } +/** One `reconstituteHistoricalTask` event (`{ timestamp: string; state: string; message?: string + * }`) → `RunEvent`, or `null` if the raw shape does not match. Split out of `a2aEventsFrom` only + * to keep that function's cognitive complexity under the ratchet — no behavior change; the same + * field checks run in the same order. */ +function a2aEventFrom(raw: unknown): RunEvent | null { + if (!isRecord(raw)) return null; + const { state, message, timestamp } = raw; + if (!isNonEmptyString(state)) return null; + if (message !== undefined && typeof message !== "string") return null; + if (timestamp !== undefined && timestamp !== null && typeof timestamp !== "string") return null; + return { + label: typeof message === "string" ? message : state, + timestamp: typeof timestamp === "string" ? timestamp : null, + }; +} + /** `{ timestamp: string; state: string; message?: string }` — reconstituteHistoricalTask's shape. */ function a2aEventsFrom(detail: Record): RunEvent[] { const events = detail.events; if (!Array.isArray(events)) return []; const out: RunEvent[] = []; for (const raw of events) { - if (!isRecord(raw)) continue; - const { state, message, timestamp } = raw; - if (!isNonEmptyString(state)) continue; - if (message !== undefined && typeof message !== "string") continue; - if (timestamp !== undefined && timestamp !== null && typeof timestamp !== "string") continue; - out.push({ - label: typeof message === "string" ? message : state, - timestamp: typeof timestamp === "string" ? timestamp : null, - }); + const event = a2aEventFrom(raw); + if (event) out.push(event); } return out; } diff --git a/src/app/(dashboard)/dashboard/orchestration/tabs/CompareRunsPanel.tsx b/src/app/(dashboard)/dashboard/orchestration/tabs/CompareRunsPanel.tsx index 994512bcbd..3c3cfc735f 100644 --- a/src/app/(dashboard)/dashboard/orchestration/tabs/CompareRunsPanel.tsx +++ b/src/app/(dashboard)/dashboard/orchestration/tabs/CompareRunsPanel.tsx @@ -338,6 +338,99 @@ function SideErrorCell({ ); } +/** Title bar (panel label + close button). Split out only to keep `CompareRunsPanel` under the + * max-lines-per-function ratchet — no behavior change. */ +function ComparePanelHeaderBar({ t, onClose }: { t: Translate; onClose: () => void }) { + return ( +
+ {t("compareTitle")} + +
+ ); +} + +/** Per-side fetch-failure row — renders only when at least one side's fetch failed (`SideErrorCell` + * itself still renders an empty, role-less cell for a side that succeeded, so the two columns stay + * grid-aligned). Split out only to keep `CompareRunsPanel` under the max-lines-per-function + * ratchet — no behavior change. */ +function SideErrorRow({ + leftError, + rightError, + t, +}: { + leftError: string | null; + rightError: string | null; + t: Translate; +}) { + if (!leftError && !rightError) return null; + return ( +
+ + +
+ ); +} + +/** Delta-column legend (Task A4 review fix, Minor #4) plus the Duration/Cost/Events metric rows + * — column order is selection order, not chronology, so nothing else on the panel says which side + * a positive delta favors. Split out only to keep `CompareRunsPanel` under the + * max-lines-per-function ratchet — no behavior change. */ +function ComparisonMetrics({ + t, + left, + right, + comparison, + eventsBothOk, +}: { + t: Translate; + left: HistoryItem; + right: HistoryItem; + comparison: RunComparison; + eventsBothOk: boolean; +}) { + return ( +
+
+ + + + + {t("compareDeltaLegend")} + +
+ + + +
+ ); +} + export function CompareRunsPanel({ left, right, @@ -366,34 +459,14 @@ export function CompareRunsPanel({ data-testid="orchestration-history-compare-panel" className="border border-border rounded p-2 overflow-x-auto overflow-y-auto max-h-[45vh] shrink-0" > -
- {t("compareTitle")} - -
+
- {(leftState.error || rightState.error) && ( -
- - -
- )} + {/* Informational only — fires on every mount where the two picks are not the same (source, identity) pair, never in response to an error condition, so `role="status"` @@ -405,40 +478,13 @@ export function CompareRunsPanel({ )} -
- {/* Delta-column legend (Task A4 review fix, Minor #4): column order is selection order, - not chronology, so nothing else on the panel says which side a positive delta favors. - Shares the metric rows' grid template and right-aligns like the delta cells below. */} -
- - - - - {t("compareDeltaLegend")} - -
- - - -
+ diff --git a/src/app/(dashboard)/dashboard/orchestration/tabs/HistoryTab.tsx b/src/app/(dashboard)/dashboard/orchestration/tabs/HistoryTab.tsx index af6e41cf77..600fb75d1c 100644 --- a/src/app/(dashboard)/dashboard/orchestration/tabs/HistoryTab.tsx +++ b/src/app/(dashboard)/dashboard/orchestration/tabs/HistoryTab.tsx @@ -339,6 +339,47 @@ function HistoryGridTable({ ); } +/** Loading indicator + "no rows" empty state. Mutually exclusive by construction (the empty + * message only ever renders once loading has finished), same as the two conditionals this + * replaces. Extracted only to keep `HistoryTab` under the max-lines-per-function ratchet. */ +function HistoryStatusRows({ + isLoading, + hasNoRows, + t, + tCommon, +}: { + isLoading: boolean; + hasNoRows: boolean; + t: ReturnType; + tCommon: ReturnType; +}) { + return ( + <> + {isLoading && ( +
+ {tCommon("loading")} +
+ )} + {hasNoRows && !isLoading &&
{t("historyEmpty")}
} + + ); +} + +/** Re-samples `nowMs` from inside a real event-driven callback (never during render — the + * `nowMs` note on `HistoryTab` explains why) so the drawer's `onActionDone` refetches the grid + * over a new range without closing the drawer: the drawer renders its own success toast right + * after calling `onActionDone`, so unmounting here would throw the confirmation away and the + * operator would see a repeat/cancel silently do nothing. The updater only ever picks the larger + * of the sampled clock and `prev + 1`, so the range always changes (and the refetch always + * happens) even when two samples land in the same millisecond. Extracted only to keep + * `HistoryTab` under the max-lines-per-function ratchet — no behavior change. */ +function refreshNowMsOnActionDone(setNowMs: (updater: (prev: number) => number) => void) { + return () => { + const sampled = Date.now(); + setNowMs((prev) => (sampled > prev ? sampled : prev + 1)); + }; +} + export function HistoryTab() { const t = useTranslations("orchestration"); // `common.loading` is an already-translated global key — the history namespace has no @@ -408,15 +449,12 @@ export function HistoryTab() { - {isLoading && ( -
- {tCommon("loading")} -
- )} - - {grid.rows.length === 0 && !isLoading && ( -
{t("historyEmpty")}
- )} + {grid.rows.length > 0 && ( )} - {/* `onActionDone` must NOT close the drawer: the drawer renders its own success toast - right after calling it, so unmounting here threw the confirmation away and the - operator saw a repeat/cancel silently do nothing. Re-sampling `nowMs` instead - keeps the drawer mounted (the toast lands) and refreshes the grid through the new - range — the same "refetch, don't close" contract `OrchestrationPageClient` uses. - `Date.now()` is sampled inside a real event-driven callback, never during render - (see the `nowMs` note above), and the updater is pure — it only picks the larger of - the sampled clock and `prev + 1`, so the range always changes (and the refetch - always happens) even when two samples land in the same millisecond. */} setSelected(null)} - onActionDone={() => { - const sampled = Date.now(); - setNowMs((prev) => (sampled > prev ? sampled : prev + 1)); - }} + onActionDone={refreshNowMsOnActionDone(setNowMs)} /> );