Skip to content

fix(wizard): progress indicator instead of disabled button while checking (#680)#710

Merged
malpern merged 1 commit into
masterfrom
fix/680-wizard-checking-progress
Jun 2, 2026
Merged

fix(wizard): progress indicator instead of disabled button while checking (#680)#710
malpern merged 1 commit into
masterfrom
fix/680-wizard-checking-progress

Conversation

@malpern
Copy link
Copy Markdown
Owner

@malpern malpern commented Jun 2, 2026

Closes #680.

The wizard communication page rendered a disabled Checking... button during the TCP check (empty action + .disabled(true)), which reads as a frozen/unresponsive control. Replaced with a plain ProgressView spinner + "Checking…" label so the in-progress state is unambiguous.

… checking (#680)

The communication page rendered a disabled "Checking..." button during the TCP
check, which reads as a frozen/unresponsive control. Replace it with a plain
spinner + "Checking…" label so the in-progress state is unambiguous.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

Code Review

PR: fix(wizard): progress indicator instead of disabled button while checking (#680)
Scope: 1 file, +11/-5 lines in WizardCommunicationPage.swift


Overview

A clear, well-scoped UX fix. The old Button("Checking...") {} with .disabled(true) looked like a frozen/broken control. Replacing it with ProgressView + label is the semantically correct macOS pattern for in-progress state. The framing and layout preservation are solid.


Issues

1. Inline comment contains an issue reference — violates CLAUDE.md convention

The comment at line 119-120 ends with (#680). CLAUDE.md says: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." Drop the issue number; the WHY (disabled button reads as frozen) is worth keeping.

Suggested:

// A ProgressView is unambiguous; a disabled button reads as a frozen/unresponsive control.

2. ProgressView has no accessibilityLabel

.accessibilityIdentifier is useful for UI test targeting but VoiceOver announces a bare ProgressView as "Progress indicator" with no context. A screen reader user mid-wizard won't know what is being checked. Suggested addition:

.accessibilityElement(children: .combine)
.accessibilityLabel("Checking communication status")
.accessibilityIdentifier("wizard-comm-checking-indicator")

3. Snapshot test coverage

This is a visually significant change (spinner replaces a disabled button). If Tests/KeyPathSnapshotTests/ has snapshots covering WizardCommunicationPage in the .checking or .authTesting state, they'll fail and need updating. Worth a quick check before merge.


What's Good

  • Correct semantic fix — ProgressView is the right control for in-progress state
  • Layout continuity: minHeight: 44 and padding(.top, WizardDesign.Spacing.itemGap) preserved
  • Uses proper Unicode ellipsis not three ASCII dots
  • Uses WizardDesign.Spacing.labelGap for inline icon+text spacing (consistent with design system)
  • .accessibilityIdentifier added — enables future UI test assertions
  • No scope creep — focused purely on the reported issue

Verdict

Two small things to address before merge: drop the (#680) from the inline comment, and add an accessibilityLabel for VoiceOver. The core fix is correct and the approach is right.

@malpern malpern merged commit c71fdd5 into master Jun 2, 2026
3 checks passed
@malpern malpern deleted the fix/680-wizard-checking-progress branch June 2, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wizard communication page shows a permanently disabled button — looks hung

1 participant