Skip to content

XS⚠️ ◾ Replace Loader2 icon usages with LoadingState component #986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomek-i wants to merge 2 commits into
base: main
Choose a base branch
from
Open

XS⚠️ ◾ Replace Loader2 icon usages with LoadingState component #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79fe851
Replace Loader2 icon usages with LoadingState component
Jul 21, 2026
1f925eb
Merge branch 'main' into 708-loader2-to-loadingstate
tomek-i Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/ui/src/components/common/LoadingState.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,28 @@
import { Spinner } from "../ui/spinner";

export function LoadingState() {
interface LoadingStateProps {
/**
* Extra classes applied to the spinner icon itself (e.g. "mr-2 h-4 w-4") so callers that
* previously rendered a bare `Loader2` icon inline can preserve their sizing/margin.
*/
className?: string;
/**

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Design note: this relocates rather than root-causes the loading-indicator inconsistency

Issue #708's presumed intent is that loading indicators are visually/behaviourally inconsistent across the app. This PR is a real improvement — it centralizes the icon and default animate-spin behaviour into Spinner/LoadingState — but by adding two open-ended escape hatches (className free-forms size/margin/colour per callsite, inline toggles layout), every callsite still hand-picks its own size (h-5 w-5 / h-4 w-4 / h-3 w-3 / w-3 h-3 / size-3.5 / size-5) and colour (text-white/50, text-purple-200, text-zinc-300, unstyled). The visual inconsistency is preserved, just expressed through className props on a shared component instead of raw Loader2 props.

Not blocking — worth naming as a follow-up if #708's real goal was visual consistency rather than code reuse: consider a size/variant prop enum instead of open className forwarding.

flagged by: codex-rescue (fallback lens)

* When set, skips the default centered block wrapper (`flex items-center justify-center
* py-8`) so the spinner sits inline next to a label instead of taking over a full block.
* Callers migrating a small inline `Loader2` (inside a button, next to text, etc.) should
* pass `true`; full-page/section loaders should leave this unset to keep prior behaviour.
*/
inline?: boolean;
}

export function LoadingState({ className, inline = false }: LoadingStateProps = {}) {
if (inline) {
return <Spinner className={className} />;
}

@tomek-i tomek-i Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] className + inline are two independent, uncoupled props — future misuse is possible

Nothing enforces that an inline-sized className (e.g. mr-2 h-4 w-4) is only ever paired with inline. A future caller could pass a small icon-sized className without inline and get an unwanted flex items-center justify-center py-8 block wrapper around a small icon, or pass inline with no className and get a bare unstyled spinner where block centering was actually wanted. All 8 current callsites in this PR pair them correctly, so this isn't an active bug — just a shape that relies on caller discipline rather than being structurally safe. A single prop (e.g. layout: "block" | "inline" combined with a size/variant prop) would make the misuse impossible by construction, if this component grows more callers over time.

flagged by: codex-rescue


return (
<div className="flex items-center justify-center py-8">

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] LoadingState conflates two visual roles behind a silent boolean default

LoadingState previously had one contract: full-block centered loader. It now has two — pass nothing → block wrapper with py-8 padding; pass inline → bare inline spinner. Because inline defaults to false with no compile-time signal distinguishing the two use cases, a caller who forgets inline at a new inline call site silently gets a broken block layout instead of a type error. This is exactly what's already happened at several pre-existing (untouched-by-this-PR) call sites in FinalResultPanel.tsx and ApprovalDialog.tsx — see the top-level summary comment. Not blocking for this diff since it doesn't itself introduce a wrong callsite, but it's the abstraction seam most likely to keep causing bugs as more call sites migrate. Consider two distinctly-named exports (e.g. LoadingState for block, an InlineSpinner/LoadingIcon for inline) instead of one component with a silently-defaulted mode flag.

flagged by: codex-rescue

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] New className/inline props on LoadingState have no direct test coverage

No test file exists for LoadingState.tsx. This PR adds two new, behavior-changing props — className (forwarded into Spinner's cn() merge) and inline (conditionally omits the block wrapper) — with no unit test verifying: (a) inline actually omits the flex items-center justify-center py-8 wrapper, (b) className reaches the rendered Spinner, (c) the no-args/default rendering is unchanged. Coverage relies entirely on incidental assertions in the 8 consuming components, most of which (HealthStatus, LLMProviderForm, OrchestratorBackendSetting, UndoStagePanel) have no test file at all, and none assert on the spinner's classes or wrapper presence.

Suggested fix: a small dedicated test rendering LoadingState with/without inline and asserting wrapper presence/absence + className passthrough would catch regressions (e.g. the tailwind-merge sizing issue flagged separately) before they reach a consumer.

flagged by: codex-rescue (fallback lens)

<Spinner />
<Spinner className={className} />

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Inline vs block branches duplicate the Spinner render

The two branches (if (inline) return <Spinner className={className} /> and the block-wrapped fallback) both just render <Spinner className={className} />, differing only in whether it's wrapped in the flex items-center justify-center py-8 div. Could be simplified to a single return with a conditional wrapper, e.g.:

const spinner = <Spinner className={className} />;
return inline ? spinner : <div className="flex items-center justify-center py-8">{spinner}</div>;

Purely stylistic — current form is also perfectly readable.

flagged by: code-review

</div>
);
}
5 changes: 3 additions & 2 deletions src/ui/src/components/health-status/health-status.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Ban, CheckCircle2, Loader2, TriangleAlert, X } from "lucide-react";
import { Ban, CheckCircle2, Loader2, TriangleAlert, X,XCircle } from "lucide-react";
import * as React from "react";
import { cn } from "@/lib/utils";
import { LoadingState } from "../common/LoadingState";

interface SuccessDetails {
username?: string;
Expand Down Expand Up @@ -68,7 +69,7 @@ export const HealthStatus = React.forwardRef<HTMLDivElement, HealthStatusProps>(
className={cn("group relative flex items-center gap-2", className)}
{...props}
>
<Loader2 className="h-5 w-5 text-white/50 animate-spin" />
<LoadingState inline className="h-5 w-5 text-white/50" />
<span className="invisible group-hover:visible absolute left-0 top-6 z-10 w-max max-w-xs rounded bg-neutral-800 px-2 py-1 text-xs shadow-lg">
Checking...
</span>
Expand Down
4 changes: 2 additions & 2 deletions src/ui/src/components/settings/llm/LLMProviderForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import type { ModelConfig } from "@shared/types/llm";
import { Loader2 } from "lucide-react";
import { useState } from "react";
import type { UseFormReturn } from "react-hook-form";
import { DeleteConfirmDialog } from "@/components/dialogs/DeleteConfirmDialog";
import { LLMProviderFields, type ProviderOption } from "@/components/llm/LLMProviderFields";
import type { HealthStatusInfo } from "../../../types";
import { LoadingState } from "../../common/LoadingState";
import { Button } from "../../ui/button";
import { Form, FormControl, FormField, FormItem, FormLabel, FormMessage } from "../../ui/form";
import { Input } from "../../ui/input";
Expand Down Expand Up @@ -81,7 +81,7 @@ export function LLMProviderForm({
Clear Config
</Button>
<Button type="submit" size="sm" disabled={isLoading}>
{isLoading ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null}
{isLoading ? <LoadingState inline className="mr-2 h-4 w-4" /> : null}
Save
</Button>
</div>
Expand Down
4 changes: 2 additions & 2 deletions src/ui/src/components/settings/llm/OrchestratorBackendSetting.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { LLMConfigV2, OrchestrationBackend, OrchestratorReadiness } from "@shared/types/llm";
import { DEFAULT_ORCHESTRATION_BACKEND } from "@shared/types/llm";
import { Loader2 } from "lucide-react";
import { useCallback, useEffect, useState } from "react";
import { toast } from "sonner";
import { LoadingState } from "@/components/common/LoadingState";
import { Button } from "@/components/ui/button";
import {
Select,
Expand Down Expand Up @@ -207,7 +207,7 @@ export function OrchestratorBackendSetting({ isActive }: OrchestratorBackendSett
onClick={() => void checkReadiness()}
disabled={isCheckingReadiness}
>
{isCheckingReadiness && <Loader2 className="mr-1.5 h-3 w-3 animate-spin" />}
{isCheckingReadiness && <LoadingState inline className="mr-1.5 h-3 w-3" />}

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Nested role="status" live region introduced at this callsite

SettingsWarningBanner (src/ui/src/components/settings/SettingsWarningBanner.tsx) renders its action slot inside an <output aria-live="polite">, which carries an implicit role="status". Previously the "Re-check" button's Loader2 icon carried no ARIA role, so there was no nesting. Now <LoadingState inline className="mr-1.5 h-3 w-3" /> renders Spinner, which sets an explicit role="status" aria-label="Loading" (src/ui/src/components/ui/spinner.tsx) — producing a role="status" element nested inside another role="status"/aria-live region. This is new at this one callsite (none of the other seven converted callsites sit inside an existing live region). Screen readers may announce the region twice or behave inconsistently with duplicated status roles.

Not blocking, but worth a look — consider suppressing Spinner's own role in this nested context, or confirm the double-announcement is an accepted trade-off.

flagged by: code-review lens

Re-check
</Button>
}
Expand Down
5 changes: 3 additions & 2 deletions src/ui/src/components/video/UploadResult.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Copy, ExternalLink, Loader2 } from "lucide-react";
import { Copy, ExternalLink } from "lucide-react";
import { toast } from "sonner";
import { Card, CardContent, CardDescription, CardHeader, CardTitle } from "@/components/ui/card";
import { useClipboard } from "../../hooks/useClipboard";
import { UploadStatus, type VideoUploadResult } from "../../types";
import { LoadingState } from "../common/LoadingState";
import { Badge } from "../ui/badge";
import { Button } from "../ui/button";

Expand All @@ -16,7 +17,7 @@ const openUrl = (url: string | null) => {

const UploadingBadge = () => (
<span className="text-sm font-medium px-3 py-1.5 rounded-full bg-white/10 text-white/80 border border-white/20 flex items-center gap-2">

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Loading spinner now announces redundant "Loading" to screen readers

Spinner (src/ui/src/components/ui/spinner.tsx) unconditionally renders role="status" aria-label="Loading". Previously the bare Loader2 icons at these 7 migrated call sites had no ARIA semantics at all — now every one announces "Loading" via an implicit live region. Here specifically, the spinner sits directly beside the visible text "Uploading", so a screen reader will likely announce something like "Loading Uploading" — redundant rather than a clean improvement. Similar redundancy exists at LLMProviderForm.tsx (+"Save"), OrchestratorBackendSetting.tsx (+"Re-check"), ShaveOutcomeView.tsx (+"This shave is still running"), UndoStagePanel.tsx, and WorkflowStepCard.tsx's retry button. Consider an aria-hidden escape hatch on Spinner/LoadingState (or an optional label override) for inline usages where adjacent visible text already conveys the loading state, rather than every migrated site double-announcing by default.

flagged by: codex-rescue

@tomek-i tomek-i Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Migrated inline spinners now announce role="status"/aria-label="Loading" next to visible text

Both review lenses independently flagged this. Spinner (src/ui/src/components/ui/spinner.tsx) unconditionally renders role="status" aria-label="Loading" on the wrapped Loader2Icon. The prior bare <Loader2 .../> icons at every migrated callsite (this one, health-status.tsx, LLMProviderForm.tsx, OrchestratorBackendSetting.tsx, ShaveOutcomeView.tsx, UndoStagePanel.tsx, and WorkflowStepCard.tsx's retry button) were purely decorative with no ARIA semantics. After this change, each now introduces a new accessible live-region announcing "Loading" immediately beside text that already conveys the same state (e.g. "Uploading"), and if multiple LoadingState instances render at once (e.g. several WorkflowStepCards simultaneously in_progress), screen readers get several indistinguishable "Loading" announcements with no way to tell them apart.

This is arguably a net accessibility improvement over having no ARIA semantics at all, so it's not a regression worth blocking on — but it's a real, untested-for behavioral change the PR description doesn't call out (acceptance criteria only address visual/animate-spin parity). Worth a conscious decision: keep as-is (accept the duplication as an intentional improvement), or let LoadingState/Spinner accept an optional override/suppression for aria-label so inline decorative usages next to redundant text can opt out.

flagged by: code-review + codex-rescue

<Loader2 className="w-3 h-3 animate-spin" />
<LoadingState inline className="w-3 h-3" />
Uploading
</span>
);
Expand Down
4 changes: 2 additions & 2 deletions src/ui/src/components/workflow/ShaveOutcomeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AlertTriangle, ExternalLink, Loader2 } from "lucide-react";
import { AlertTriangle, ExternalLink } from "lucide-react";
import { useCallback, useEffect, useState } from "react";
import { getStatusVariant } from "@/lib/shave-utils";
import { ipcClient } from "../../services/ipc-client";
Expand Down Expand Up @@ -121,7 +121,7 @@ export function ShaveOutcomeView({ shaveId }: ShaveOutcomeViewProps) {
{isProcessing && (
<div className="rounded-md border border-white/15 bg-white/5 p-3">
<div className="flex items-center gap-2 text-white/80 font-medium">
<Loader2 className="h-4 w-4 animate-spin" />
<LoadingState inline className="h-4 w-4" />
This shave is still running
</div>
<p className="text-sm text-white/60 mt-1">
Expand Down
5 changes: 3 additions & 2 deletions src/ui/src/components/workflow/UndoStagePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Check, ChevronRight, Loader2, Undo2, Wrench, X } from "lucide-react";
import { Check, ChevronRight, Undo2, Wrench, X } from "lucide-react";
import type React from "react";
import { type MCPStep, MCPStepType } from "../../types";
import { deepParseJson, formatToolName } from "../../utils";
import { LoadingState } from "../common/LoadingState";
import { ReasoningStep } from "./ReasoningStep";

const handleDetailsToggle = (data: unknown) => (e: React.SyntheticEvent<HTMLDetailsElement>) => {
Expand Down Expand Up @@ -72,7 +73,7 @@ interface UndoStagePanelProps {

const StatusBadge = ({ status }: { status: UndoStagePanelProps["status"] }) => {
if (status === "in_progress") {
return <Loader2 className="w-4 h-4 text-purple-200 animate-spin" />;
return <LoadingState inline className="w-4 h-4 text-purple-200" />;
}
if (status === "completed") {
return <Check className="w-4 h-4 text-green-300" />;
Expand Down
13 changes: 9 additions & 4 deletions src/ui/src/components/workflow/WorkflowStepCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
CheckCircle2,
ChevronDown,
ChevronRight,
Loader2,
RefreshCw,
Sparkles,
XCircle,
Expand All @@ -15,6 +14,7 @@ import { cn } from "@/lib/utils";
import { formatErrorMessage, isErrorStep } from "@/utils";
import { ipcClient } from "../../services/ipc-client";
import type { MCPStep } from "../../types";
import { LoadingState } from "../common/LoadingState";
import { Badge } from "../ui/badge";
import { Button } from "../ui/button";
import { Card, CardContent } from "../ui/card";
Expand Down Expand Up @@ -60,8 +60,8 @@ function OrchestratorBadge({ backend }: { backend: OrchestratorBackend }) {

const STATUS_CONFIG = {
in_progress: {

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] STATUS_CONFIG.in_progress.icon is now dead/misleading data

StatusIcon special-cases status === "in_progress" with an early return (rendering <LoadingState inline .../>) before config.icon is ever read for that branch. STATUS_CONFIG.in_progress.icon was changed from Loader2 to null but the key is still present — unlike not_started.icon: null, which legitimately falls through to the generic !Icon branch, in_progress.icon is now unreachable/unused. A future maintainer changing STATUS_CONFIG.in_progress.icon (e.g. swapping the icon) would see no effect and have to discover the early-return special-case in StatusIcon to understand why.

Suggested fix: drop the icon key from the in_progress entry (or add a short comment noting it's superseded by StatusIcon's early return).

flagged by: both lenses independently (code-review + codex-rescue)

icon: Loader2,
iconClass: "animate-spin text-zinc-300",
icon: null,

@tomek-i tomek-i Jul 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] STATUS_CONFIG.in_progress.icon is now dead/unused

in_progress.icon is set to null and iconClass no longer includes animate-spin, but StatusIcon special-cases status === "in_progress" before ever reading config.icon (line ~92), so this field is now vestigial — it happens to share the same null value as not_started's icon, but for not_started that null is actually read and drives the neutral-circle fallback, while for in_progress it's never read at all (the early return bypasses it). A future maintainer skimming STATUS_CONFIG could reasonably assume icon: null still means "render the neutral circle" for in_progress", and removing the special case would silently turn the spinner into a plain circle. Consider dropping the iconkey from thein_progress` entry (or adding a short comment noting it's unused/kept only for object-shape uniformity).

flagged independently by both lenses: code-review + codex-rescue

iconClass: "text-zinc-300",
containerClass: "border-gray-500/30 bg-gray-500/5",
textClass: "text-white/90",
},
Expand All @@ -87,6 +87,11 @@ const STATUS_CONFIG = {

function StatusIcon({ status, className }: { status: WorkflowStep["status"]; className?: string }) {
const config = STATUS_CONFIG[status as keyof typeof STATUS_CONFIG] || STATUS_CONFIG.not_started;

if (status === "in_progress") {
return <LoadingState inline className={cn("size-5", config.iconClass, className)} />;
}

const Icon = config.icon;

if (!Icon) {
Expand Down Expand Up @@ -271,7 +276,7 @@ export function WorkflowStepCard({ step, label, shaveId }: WorkflowStepCardProps
className="bg-white/[0.08] border border-white/[0.15] hover:bg-white/[0.12] text-white/80 shrink-0"
>
{isRetrying ? (
<Loader2 className="size-3.5 animate-spin" />
<LoadingState inline className="size-3.5" />
) : (
<>
<RefreshCw className="size-3.5 mr-1.5" />
Expand Down
Loading