mirror of
https://github.com/awizemann/scarf.git
synced 2026-05-10 10:36:35 +00:00
fix(ios-onboarding): hide Cancel on first-run onboarding (#55)
App Store Connect feedback: "Cancel button not working" on the
"Connect to Hermes" onboarding screen.
Confirmed root cause in RootModel.cancelOnboarding:
state = servers.isEmpty
? .onboarding(forNewServer: ServerID())
: .serverList
When the user has zero configured servers (the first-run case),
the conditional re-presented a fresh onboarding view. The button
fired, the state mutated, but the visible result was "tap Cancel,
get an identical screen" — indistinguishable from a dead button.
The defensive intent ("don't strand the user on an empty server
list") was reasonable, but the UX-as-shipped is worse than the
strand it tried to prevent — first-run TestFlight users see a
seemingly broken app.
Fix at the right layer: don't show Cancel when there's nowhere
to go.
- New `canCancel: Bool` parameter on OnboardingRootView (default
true). When false, the leading toolbar slot omits the Cancel
button entirely.
- RootView passes `canCancel: !model.servers.isEmpty`.
- RootModel.cancelOnboarding simplified — drops the defensive
`.isEmpty` re-loop branch, asserts the invariant in debug, and
in release still routes to `.serverList` (which renders an
empty-state with the "+ Add server" toolbar button) rather than
re-presenting onboarding.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -185,8 +185,20 @@ final class RootModel {
|
|||||||
|
|
||||||
/// Cancel an in-progress onboarding and return to the list.
|
/// Cancel an in-progress onboarding and return to the list.
|
||||||
/// Called by the sheet's Cancel affordance.
|
/// Called by the sheet's Cancel affordance.
|
||||||
|
///
|
||||||
|
/// Issue #55: prior versions had a defensive `servers.isEmpty`
|
||||||
|
/// fallback that re-presented onboarding when there was nothing
|
||||||
|
/// to fall back to. That made Cancel look broken on first-run.
|
||||||
|
/// `OnboardingRootView` now hides the Cancel button when
|
||||||
|
/// `canCancel == false`, so this path is only ever reached when
|
||||||
|
/// at least one server already exists. In debug we assert that
|
||||||
|
/// invariant; in release we still route to `.serverList` (which
|
||||||
|
/// renders an empty-state with the "+ Add server" button) rather
|
||||||
|
/// than re-presenting onboarding, so the worst case is "user
|
||||||
|
/// sees the empty server list" rather than "Cancel does nothing."
|
||||||
func cancelOnboarding() {
|
func cancelOnboarding() {
|
||||||
state = servers.isEmpty ? .onboarding(forNewServer: ServerID()) : .serverList
|
assert(!servers.isEmpty, "cancelOnboarding called with no servers — Cancel button should be hidden via OnboardingRootView.canCancel")
|
||||||
|
state = .serverList
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Called from OnboardingView when the flow finishes. Reload the
|
/// Called from OnboardingView when the flow finishes. Reload the
|
||||||
@@ -320,7 +332,14 @@ struct RootView: View {
|
|||||||
case .serverList:
|
case .serverList:
|
||||||
ServerListView(model: model)
|
ServerListView(model: model)
|
||||||
case .onboarding(let forNewServer):
|
case .onboarding(let forNewServer):
|
||||||
OnboardingRootView(targetServerID: forNewServer) {
|
// canCancel is gated on whether there's a server list to
|
||||||
|
// return to (issue #55). On first-run the user MUST add
|
||||||
|
// their first server to use the app — the toolbar omits
|
||||||
|
// the Cancel button in that case.
|
||||||
|
OnboardingRootView(
|
||||||
|
targetServerID: forNewServer,
|
||||||
|
canCancel: !model.servers.isEmpty
|
||||||
|
) {
|
||||||
await model.onboardingFinished(serverID: forNewServer)
|
await model.onboardingFinished(serverID: forNewServer)
|
||||||
} onCancel: {
|
} onCancel: {
|
||||||
model.cancelOnboarding()
|
model.cancelOnboarding()
|
||||||
|
|||||||
@@ -18,15 +18,24 @@ struct OnboardingRootView: View {
|
|||||||
/// step 1 with nowhere to go. Optional for callers that don't
|
/// step 1 with nowhere to go. Optional for callers that don't
|
||||||
/// need cancel (shouldn't be any, but keeps the API forgiving).
|
/// need cancel (shouldn't be any, but keeps the API forgiving).
|
||||||
let onCancel: @MainActor () -> Void
|
let onCancel: @MainActor () -> Void
|
||||||
|
/// Whether the Cancel button should appear in the nav bar
|
||||||
|
/// (issue #55). False on the first-run onboarding where there
|
||||||
|
/// is no `.serverList` to fall back to — showing Cancel there
|
||||||
|
/// fired the action but the state machine routed straight back
|
||||||
|
/// into onboarding, so the button looked broken to TestFlight
|
||||||
|
/// users.
|
||||||
|
let canCancel: Bool
|
||||||
|
|
||||||
@State private var vm: OnboardingViewModel
|
@State private var vm: OnboardingViewModel
|
||||||
|
|
||||||
init(
|
init(
|
||||||
targetServerID: ServerID,
|
targetServerID: ServerID,
|
||||||
|
canCancel: Bool = true,
|
||||||
onFinished: @escaping @MainActor () async -> Void,
|
onFinished: @escaping @MainActor () async -> Void,
|
||||||
onCancel: @escaping @MainActor () -> Void = {}
|
onCancel: @escaping @MainActor () -> Void = {}
|
||||||
) {
|
) {
|
||||||
self.targetServerID = targetServerID
|
self.targetServerID = targetServerID
|
||||||
|
self.canCancel = canCancel
|
||||||
self.onFinished = onFinished
|
self.onFinished = onFinished
|
||||||
self.onCancel = onCancel
|
self.onCancel = onCancel
|
||||||
let service = CitadelSSHService()
|
let service = CitadelSSHService()
|
||||||
@@ -63,9 +72,16 @@ struct OnboardingRootView: View {
|
|||||||
// to cancel. Hiding the button then also keeps
|
// to cancel. Hiding the button then also keeps
|
||||||
// users from accidentally wiping a just-saved
|
// users from accidentally wiping a just-saved
|
||||||
// server mid-race.
|
// server mid-race.
|
||||||
|
//
|
||||||
|
// Also hidden on first-run onboarding (issue #55):
|
||||||
|
// there is no server list to return to, so Cancel
|
||||||
|
// would either be inert (state machine looping
|
||||||
|
// back into onboarding) or confusing (an empty
|
||||||
|
// server list with no path forward). Better to
|
||||||
|
// not show the affordance at all.
|
||||||
if case .connected = vm.step {
|
if case .connected = vm.step {
|
||||||
EmptyView()
|
EmptyView()
|
||||||
} else {
|
} else if canCancel {
|
||||||
Button("Cancel") {
|
Button("Cancel") {
|
||||||
onCancel()
|
onCancel()
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user