XS⚠️ ◾ Replace Loader2 icon usages with LoadingState component#986
XS⚠️ ◾ Replace Loader2 icon usages with LoadingState component#986tomek-i wants to merge 1 commit into
Conversation
Adds optional className/inline props to LoadingState so it can replace every direct Loader2 usage while preserving original sizing, margin, and animation. Closes #708. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR standardizes loading indicators across the desktop UI by replacing direct Loader2 (lucide-react) usages with the shared LoadingState component, improving visual/behavioral consistency and reducing duplicated spinner styling.
Changes:
- Extended
LoadingStateto support inline rendering and custom sizing/margin viainline+className. - Replaced inline
Loader2JSX usages in several UI components withLoadingState inlinewhile preserving sizing/color classes. - Updated
WorkflowStepCardstatus icon rendering to special-case thein_progressstatus usingLoadingStateinstead of the previous icon table entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/src/components/common/LoadingState.tsx | Adds className + inline props to support both block and inline spinners. |
| src/ui/src/components/health-status/health-status.tsx | Migrates “checking” spinner to LoadingState inline. |
| src/ui/src/components/settings/llm/LLMProviderForm.tsx | Replaces submit-button loader icon with LoadingState inline. |
| src/ui/src/components/settings/llm/OrchestratorBackendSetting.tsx | Replaces readiness re-check spinner with LoadingState inline. |
| src/ui/src/components/video/UploadResult.tsx | Replaces uploading badge spinner with LoadingState inline. |
| src/ui/src/components/workflow/ShaveOutcomeView.tsx | Replaces “still running” spinner with LoadingState inline. |
| src/ui/src/components/workflow/UndoStagePanel.tsx | Replaces in-progress status spinner with LoadingState inline. |
| src/ui/src/components/workflow/WorkflowStepCard.tsx | Uses LoadingState for in_progress status + retry button spinner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| @@ -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"> | |||
There was a problem hiding this comment.
[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
| } | ||
|
|
||
| return ( | ||
| <div className="flex items-center justify-center py-8"> |
There was a problem hiding this comment.
[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
| in_progress: { | ||
| icon: Loader2, | ||
| iconClass: "animate-spin text-zinc-300", | ||
| icon: null, |
There was a problem hiding this comment.
[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
| return ( | ||
| <div className="flex items-center justify-center py-8"> | ||
| <Spinner /> | ||
| <Spinner className={className} /> |
There was a problem hiding this comment.
[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
|
[major, out-of-diff — flagged by: codex-rescue] Pre-existing non-inline
This PR adds exactly the Not blocking for this PR (the bug predates it and this PR doesn't regress it), but flagged because it directly undercuts the PR's own "no visual regressions / consistent loaders" framing if left as-is. |
|
[major, out-of-diff — flagged by: codex-rescue] Pre-existing non-inline
Same recommendation: apply Not blocking for this PR — the bug predates it and isn't introduced or worsened by this diff. |
muster review — PR #986Two independent lenses reviewed this diff in parallel: code-review (conventions/correctness) and codex-rescue (root-cause second opinion). Findings below are consolidated and deduped. Summary
Bottom line: no blocking findings — review advisory only. The in-diff change itself (7 call sites migrated from bare Where the lenses agreed vs. diverged
Findings
Inline comments posted at the relevant lines for findings 3–6; findings 1–2 posted as top-level comments (no diff lines in this PR to anchor to). Posted by ARMADA muster — two-lens review (code-review + codex-rescue), non-interactive. |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest: reviewed — 6 findings (2 major, 2 minor, 2 nit), zero blocking. Advisory only — awaiting human merge (auto-merge off). |
|
🔭 crows-nest: reconciling stale claim — pipeline had already completed (reviewed, addressed, zero blocking findings) but the |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: picked up for review by ARMADA muster (unified scheduler tick, label |
| @@ -60,8 +60,8 @@ function OrchestratorBadge({ backend }: { backend: OrchestratorBackend }) { | |||
|
|
|||
| const STATUS_CONFIG = { | |||
| in_progress: { | |||
There was a problem hiding this comment.
[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)
| 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" />} |
There was a problem hiding this comment.
[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
| } | ||
|
|
||
| return ( | ||
| <div className="flex items-center justify-center py-8"> |
There was a problem hiding this comment.
[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)
| * previously rendered a bare `Loader2` icon inline can preserve their sizing/margin. | ||
| */ | ||
| className?: string; | ||
| /** |
There was a problem hiding this comment.
[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)
Muster review — PR #986Dual-lens review note (degraded): the Verdict: no blocking findings — review advisory onlySummary: 0 blocking · 1 major · 4 minor · 2 nit The refactor itself is sound: mechanical Findings (worst first)
Lens agreement / divergenceThe two lenses converged strongly on the Bottom line0 blocking, 1 major, 4 minor, 2 nit — not blocking merge, but the tailwind-merge sizing fragility (#1) is worth a look before or shortly after merge since it's the one finding with a plausible (if unproven) path to a visible regression. |
tomek-i
removed
the
armada:reviewing
|
🔭 crows-nest: muster review complete — no blocking findings (7 non-blocking findings posted for follow-up). PR is green and mergeable but |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| @@ -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"> | |||
There was a problem hiding this comment.
[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
| export function LoadingState({ className, inline = false }: LoadingStateProps = {}) { | ||
| if (inline) { | ||
| return <Spinner className={className} />; | ||
| } |
There was a problem hiding this comment.
[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
muster review — PR #986Two independent lenses reviewed this diff in parallel: a conventions/correctness pass and a codex-rescue root-cause second opinion. Verdict: no blocking or major findings — 2 nits. This is a clean, tightly-scoped refactor. What both lenses verified independently
Findings (posted inline)
One additional claim raised by the codex-rescue lens (that Bottom line: 0 blocking, 0 major, 2 nits — no blocking issues, review advisory only. Looks mergeable as-is. Posted by |
tomek-i
removed
the
armada:reviewing
|
🔭 crows-nest: review complete — 0 blocking, 0 major, 2 advisory nits (both lenses agreed, no degradation). ✅ reviewed, no blocking findings — awaiting human merge (auto-merge off). |
Summary
Replaces every direct
Loader2(lucide-react) icon usage in the desktop UI with the existing reusableLoadingStatecomponent, so loading indicators are visually and behaviourally consistent across the app instead of each callsite hand-rolling its own spinner classes.Closes #708
Key decisions
LoadingStatewith optionalclassName/inlineprops instead of introducing a second component.classNameforwards to the underlyingSpinner(which already merges classes viacn/tailwind-merge and appliesanimate-spinby default), so callers migrating a bareLoader2icon (e.g.mr-2 h-4 w-4) keep identical sizing/margin without re-specifyinganimate-spin.inlineskips the defaultflex items-center justify-center py-8block wrapper so the spinner can sit next to a label inside a button/badge instead of forcing block layout.<LoadingState />caller (HomePage,ProjectsPage,ApprovalDialog,FinalResultPanel,PromptSelectionDialog) renders pixel-identical output; those files have zero diff.WorkflowStepCard'sSTATUS_CONFIGicon table mappedin_progressto theLoader2component reference, rendered generically alongsideCheckCircle2/XCirclevia<Icon className={...} />. SinceLoadingStateisn't a drop-in Lucide-icon-shaped component,StatusIconnow special-casesstatus === "in_progress"to render<LoadingState inline className={cn("size-5", config.iconClass, className)} />before falling through to the generic icon render for the other statuses.Files changed
src/ui/src/components/common/LoadingState.tsx— add optionalclassName/inlinepropssrc/ui/src/components/health-status/health-status.tsxsrc/ui/src/components/settings/llm/LLMProviderForm.tsxsrc/ui/src/components/settings/llm/OrchestratorBackendSetting.tsxsrc/ui/src/components/video/UploadResult.tsxsrc/ui/src/components/workflow/ShaveOutcomeView.tsxsrc/ui/src/components/workflow/UndoStagePanel.tsxsrc/ui/src/components/workflow/WorkflowStepCard.tsxNo remaining
Loader2usages in application code —grep -rn "Loader2" src/ui/src --include="*.tsx" --include="*.ts"returns nothing outsideui/spinner.tsx's ownLoader2Iconimport, which is the low-level primitiveLoadingStatealready wraps and was intentionally left untouched.Acceptance criteria
Loader2now useLoadingState.className, so rendered output matches the prior markup (animate-spinnow applied byLoadingState/Spinneritself).LoadingStateis flexible enough to support prior sizing/margin/animation via props (className,inline) with no visual regressions.Loader2by name; the one test coupled to the visual contract (WorkflowStepCard.test.tsx's.animate-spinquerySelector assertion for thein_progressstatus) still passes sinceSpinnerappliesanimate-spinby default.Testing performed
npm run build— passnpm rebuild better-sqlite3 --build-from-source && npx vitest run --exclude 'src/ui/**'— pass (68 files / 696 tests)npm --prefix src/ui install && npm --prefix src/ui test— pass (35 files / 259 tests), includingWorkflowStepCard.test.tsx,UploadResult.test.tsx,ShaveOutcomeView.test.tsxnpm run lint— pass (biome clean; one pre-existing informational note on an unrelated file with no diff in this PR)npm run format— no fixes needed;git diff --exit-codeclean