Skip to content

Fix onboarding skip alert and empty name bypass#6080

Merged
beastoin merged 3 commits into
mainfrom
worktree-fix-onboarding-flaws-v3
Mar 27, 2026
Merged

Fix onboarding skip alert and empty name bypass#6080
beastoin merged 3 commits into
mainfrom
worktree-fix-onboarding-flaws-v3

Conversation

@beastoin
Copy link
Copy Markdown
Collaborator

@beastoin beastoin commented Mar 27, 2026

Summary

Evidence

Flow-walker report: https://flow-walker.beastoin.workers.dev/runs/qZx3xnCZpk.html

Screenshots

1. Onboarding chat step with Skip button and 3D brain graph
Onboarding start

2. Skip alert renders behind brain graph (FLAW — alert buttons at x=845 instead of centered)
Skip alert hidden

3. Empty send guard — canSend=false prevents empty message, new guard adds defense-in-depth
Empty send test

agent-swift accessibility dump (#6069)

e21  AXButton  "Skip anyway"     bounds={x:845, y:522}  -- behind brain graph
e22  AXButton  "Continue setup"  bounds={x:845, y:556}  -- behind brain graph

Test plan

  • Click "Skip" during onboarding — confirmation dialog should appear above all content
  • Press Enter/Cmd+Enter with empty input — nothing should be sent
  • Normal onboarding flow still works end-to-end

by AI for @beastoin

🤖 Generated with Claude Code

…pass

Move .alert() modifier from Skip button to top-level VStack so the
confirmation dialog renders above the 3D brain graph ZStack (#6069).

Add empty text guard in sendMessage() to prevent sending whitespace-only
input which could bypass the name entry step (#6071).

Closes #6069
Closes #6071

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 27, 2026

Greptile Summary

This PR fixes two onboarding UX regressions on the desktop app: an invisible skip-confirmation alert (the .alert() modifier was attached to the Skip Button inside a ZStack, causing SwiftUI to render it behind the brain graph overlay) and a whitespace-only send bypass in sendMessage().\n\n- Alert fix (OnboardingChatView.swift lines 169→495): Moving .alert() from the Button to the top-level VStack in var body is the canonical SwiftUI approach. Alert modifiers attached to views buried inside a ZStack can render beneath sibling content; placing them on the outermost view guarantees correct layering. Fix is correct and well-scoped.\n- Empty-text guard (OnboardingChatView.swift line 779): The added guard !text.isEmpty else { return } is technically redundant — the existing guard canSend (line 776) already prevents whitespace-only sends because canSend trims inputText before the empty check. The new guard is harmless and serves as a defensive layer, but the PR description's framing of it as a separate independently-reachable bug may be overstated.\n\nNo regressions introduced; both changes are safe to merge.

Confidence Score: 5/5

Safe to merge — both fixes are correct SwiftUI patterns with no regressions introduced.

The alert relocation is the right fix and follows standard SwiftUI guidance. The empty-text guard is redundant but harmless. No new logic paths or state mutations are introduced. The change is well-scoped to a single file with a very narrow blast radius.

No files require special attention.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/OnboardingChatView.swift Two targeted fixes: moves .alert() from Skip button to the top-level VStack body (correct SwiftUI pattern to render alerts above ZStack siblings), and adds a redundant-but-harmless empty-text guard after trimming whitespace in sendMessage().

Sequence Diagram

sequenceDiagram
    actor User
    participant SkipButton
    participant TopLevelVStack
    participant Alert
    participant SendMessage
    participant canSend

    Note over SkipButton,Alert: Before fix: .alert() on Button inside ZStack → rendered behind brain graph
    Note over TopLevelVStack,Alert: After fix: .alert() on top-level VStack → renders above all content

    User->>SkipButton: tap "Skip"
    SkipButton->>TopLevelVStack: showSkipConfirmation = true
    TopLevelVStack->>Alert: present (above ZStack)
    Alert-->>User: visible confirmation dialog

    User->>SendMessage: press Enter / click send
    SendMessage->>canSend: guard canSend
    canSend-->>SendMessage: false if isSending or inputText is empty/whitespace
    SendMessage->>SendMessage: let text = inputText.trimmed
    SendMessage->>SendMessage: guard !text.isEmpty (new defensive check)
    SendMessage-->>User: message sent only if non-empty
Loading

Reviews (1): Last reviewed commit: "Fix onboarding skip alert hidden behind ..." | Re-trigger Greptile

guard canSend else { return }

let text = inputText.trimmingCharacters(in: .whitespacesAndNewlines)
guard !text.isEmpty else { return }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Redundant empty-text guard

The new guard !text.isEmpty else { return } is already covered by the guard canSend on line 776. canSend is defined as:

private var canSend: Bool {
    !inputText.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty && !chatProvider.isSending
}

Since canSend requires that inputText is non-empty after trimming, any whitespace-only input will have already caused an early return at line 776 — making this check unreachable in practice.

The guard is harmless (and is a reasonable defensive layer if sendMessage() is ever called from a new code path), but the PR description's framing of it as a separate bug-fix may be overstated. Worth keeping as defensive programming, but it's not fixing an independently reachable bug today.

@beastoin
Copy link
Copy Markdown
Collaborator Author

All omi-pr-workflow checkpoints passed (CP0-CP8):

  • CP0: Preflight ✅
  • CP1: Issues Desktop onboarding: Skip confirmation dialog hidden behind 3D brain graph #6069 and Desktop onboarding: Empty name bypass skips entire chat step #6071 understood ✅
  • CP2: Worktree workspace from clean main ✅
  • CP3: Code explored, approach planned (UI-only, live_test_required=false) ✅
  • CP4: Codex consulted — confirmed both fixes, applied identical patches independently ✅
  • CP5: Implementation committed, build passes (xcrun swift build -c debug) ✅
  • CP6: PR created with summary, evidence, issue links ✅
  • CP7: Codex reviewer approved — PR_APPROVED_LGTM, no blocking findings ✅
  • CP8: Codex tester evaluated — low edge-case risk, canSend already guards empty sends (new guard is defense-in-depth), named test bundle built and ran ✅
  • CP9: Skipped (UI-only, no audio/streaming paths)

Greptile also reviewed: Confidence 5/5, "Safe to merge — both fixes are correct SwiftUI patterns with no regressions introduced."

PR is ready for merge. Waiting for manager approval.

by AI for @beastoin

@beastoin
Copy link
Copy Markdown
Collaborator Author

Bug Evidence (screenshots + flow-walker report)

Flow-walker report: https://flow-walker.beastoin.workers.dev/runs/qZx3xnCZpk.html

Step 1: Onboarding chat step with Skip button and brain graph

onboarding-start
The chat step shows Skip button (top-right) and 3D brain graph (right panel).

Step 2: Skip alert hidden behind brain graph (#6069)

After clicking Skip, the chat dims but the confirmation dialog is NOT visible to the user. Via agent-swift snapshot -i --json, the alert buttons are at x=845 (in the right panel behind the brain graph), not centered:

e21  AXButton  "Skip anyway"     bounds={x:845, y:522}
e22  AXButton  "Continue setup"  bounds={x:845, y:556}

Fix: Moving .alert() to the top-level VStack ensures it renders above all sibling content.

Step 3: Empty input test (#6071)

Send button (Arrow Up Circle) is disabled when input is empty (canSend returns false). The new guard !text.isEmpty is defense-in-depth for any code path that calls sendMessage() without going through canSend.

by AI for @beastoin

@beastoin
Copy link
Copy Markdown
Collaborator Author

beastoin commented Mar 27, 2026

Evidence

Flow-walker report: https://flow-walker.beastoin.workers.dev/runs/qZx3xnCZpk.html

Screenshots

1. Onboarding chat step with Skip button and 3D brain graph
Onboarding start

2. Skip alert renders behind brain graph (FLAW — alert buttons at x=845 instead of centered)
Skip alert hidden behind brain graph

3. Empty send guard — canSend=false prevents empty message, new guard adds defense-in-depth
Empty send test

beastoin and others added 2 commits March 27, 2026 04:46
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Screenshots now hosted at gs://omi-pr-assets/pr-6080/

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@beastoin
Copy link
Copy Markdown
Collaborator Author

lgtm

@beastoin beastoin merged commit 9a53515 into main Mar 27, 2026
2 checks passed
@beastoin beastoin deleted the worktree-fix-onboarding-flaws-v3 branch March 27, 2026 08:11
Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
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.

Desktop onboarding: Empty name bypass skips entire chat step Desktop onboarding: Skip confirmation dialog hidden behind 3D brain graph

1 participant