Skip to content

Revert "Fix onboarding skip alert and empty name bypass"#6076

Merged
beastoin merged 1 commit into
mainfrom
revert-6074-worktree-fix-onboarding-flaws-v2
Mar 27, 2026
Merged

Revert "Fix onboarding skip alert and empty name bypass"#6076
beastoin merged 1 commit into
mainfrom
revert-6074-worktree-fix-onboarding-flaws-v2

Conversation

@beastoin
Copy link
Copy Markdown
Collaborator

Reverts #6074

@beastoin beastoin merged commit f2f6730 into main Mar 27, 2026
2 checks passed
@beastoin beastoin deleted the revert-6074-worktree-fix-onboarding-flaws-v2 branch March 27, 2026 02:05
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 27, 2026

Greptile Summary

This PR reverts #6074, which had fixed two issues in OnboardingChatView.swift: (1) the skip-confirmation alert not presenting reliably when attached to a Button, and (2) an empty-name/empty-message bypass in sendMessage().\n\nKey concerns:\n- Alert placement regression: The .alert modifier is moved back from the outer container onto the Button view. PR #6074 fixed a SwiftUI macOS presentation issue where the alert wasn't reliably shown from a nested Button. This revert likely re-introduces that behavior.\n- Empty-text guard removed: guard !text.isEmpty else { return } is deleted from sendMessage(). While canSend provides a first line of defense, the removed guard was a defense-in-depth measure preventing empty strings from reaching chatProvider.sendMessage and ChatToolExecutor.resumeFollowup in edge cases.\n- The PR description contains no explanation of why these safety fixes needed to be reverted, making it difficult to evaluate whether the trade-off is intentional.

Confidence Score: 2/5

This PR reverts targeted safety fixes without explanation, re-introducing a potentially silent skip (no confirmation dialog) and removing an empty-message guard.

Two regressions are being deliberately reintroduced: unreliable alert presentation on the skip button and removal of an empty-text guard. Neither change is accompanied by a rationale. The alert issue in particular could silently bypass the confirmation step for users, which is the core UX protection during onboarding.

desktop/Desktop/Sources/OnboardingChatView.swift — both changed hunks introduce regressions from #6074

Important Files Changed

Filename Overview
desktop/Desktop/Sources/OnboardingChatView.swift Reverts two safety fixes from #6074: moves the skip-confirmation alert back onto the Button (potentially re-introducing unreliable presentation on macOS) and removes the guard !text.isEmpty defensive check in sendMessage().

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User clicks Skip button] --> B{showSkipConfirmation = true}
    B --> C[.alert on Button fires]
    C -->|Alert presents OK| D{User choice}
    C -->|Alert fails to present on macOS| E[onSkip called silently — bug]
    D -->|Skip anyway| F[onSkip]
    D -->|Continue setup| G[Dismiss alert]

    H[User presses Enter / clicks Send] --> I{canSend check}
    I -->|false| J[Return early]
    I -->|true| K[Trim inputText to text]
    K --> L[inputText = empty string]
    L -->|guard removed| M[chatProvider.sendMessage with text]
    M -->|text could be empty in edge case| N[Empty message sent to AI]
Loading

Reviews (1): Last reviewed commit: "Revert "Fix onboarding skip alert and em..." | Re-trigger Greptile

Comment on lines +172 to +177
.alert("Are you sure?", isPresented: $showSkipConfirmation) {
Button("Skip anyway", role: .destructive) { onSkip() }
Button("Continue setup", role: .cancel) {}
} message: {
Text("Omi won't be useful for you if it doesn't know enough about you.")
}
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.

P1 Alert re-attached to Button may not present reliably

The .alert modifier was deliberately moved off the Button and onto the outer container in PR #6074 to fix a presentation issue. In SwiftUI on macOS, .alert attached to a narrow, nested view like a Button can fail to present when the view hierarchy has competing modifiers or the button is inside a complex HStack/VStack layout. Re-attaching it here may re-introduce the exact bug that #6074 fixed — users clicking "Skip" might see no confirmation dialog at all, which would silently call onSkip() without the expected warning.

If the revert is intentional for a specific reason, please document it in a code comment so future reviewers understand why this placement was chosen over the outer-container approach.

Comment on lines 778 to 779
let text = inputText.trimmingCharacters(in: .whitespacesAndNewlines)
guard !text.isEmpty else { return }
inputText = ""
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.

P1 Empty-text guard removed — defense-in-depth lost

The removed guard !text.isEmpty else { return } (introduced in #6074 for the "empty name bypass" fix) provided a second safety layer after canSend. While canSend does check !inputText.trimmingCharacters(…).isEmpty, the guard on text ensured that even if sendMessage() is called through a code path where canSend is transiently stale or via .onSubmit under unusual timing, an empty string is never forwarded to chatProvider.sendMessage(text) or ChatToolExecutor.resumeFollowup(with: text).

Without this guard, an edge case where canSend passes but text is empty (e.g., rapid keyboard submission while inputText is being cleared) could result in an empty message being sent to the AI and resumeFollowup being invoked with an empty reply.

Suggested change
let text = inputText.trimmingCharacters(in: .whitespacesAndNewlines)
guard !text.isEmpty else { return }
inputText = ""
let text = inputText.trimmingCharacters(in: .whitespacesAndNewlines)
guard !text.isEmpty else { return }
inputText = ""

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.

1 participant