Skip to content

Fix ToS checkbox state desync using @AppStorage #23419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashleeradka merged 4 commits into from
Apr 3, 2026
Merged

Fix ToS checkbox state desync using @AppStorage #23419

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7fc6c4
fix: use @AppStorage and ToggleStyle for ToS checkbox state
devin-ai-integration[bot] Apr 3, 2026
54bd3ad
fix: scope toggle gesture to checkbox indicator only
devin-ai-integration[bot] Apr 3, 2026
ff5de16
fix: replace Toggle+ToggleStyle with direct Button for checkbox
devin-ai-integration[bot] Apr 3, 2026
9000a7a
chore: remove redundant .animation modifier on checkbox
devin-ai-integration[bot] Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 37 additions & 48 deletions clients/macos/vellum-assistant/Features/Onboarding/ImproveExperienceStepView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ struct ImproveExperienceStepView: View {

@State private var showTitle = false
@State private var showContent = false
@State private var collectUsageData: Bool = UserDefaults.standard.object(forKey: "collectUsageData") as? Bool ?? true
@State private var sendDiagnostics: Bool = UserDefaults.standard.object(forKey: "sendDiagnostics") as? Bool ?? true
@State private var tosAccepted: Bool = UserDefaults.standard.bool(forKey: "tosAccepted")
@AppStorage("collectUsageData") private var collectUsageData: Bool = true
@AppStorage("sendDiagnostics") private var sendDiagnostics: Bool = true
Comment on lines +16 to +17
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 3, 2026

Choose a reason for hiding this comment

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

🚩 SettingsStore won't reflect @AppStorage changes made during onboarding

The SettingsStore at Features/Settings/SettingsStore.swift:309-316 initializes its @Published var sendDiagnostics and collectUsageData from UserDefaults at init time, and writes back via Combine sink pipelines. It does NOT subscribe to external UserDefaults changes for those keys. If @AppStorage in the onboarding view writes new values to UserDefaults, an already-initialized SettingsStore won't pick them up. In practice this is unlikely to cause issues because SettingsStore is typically initialized after onboarding completes, so it will read the correct persisted values. But if there's a code path where SettingsStore is already alive when onboarding is shown (e.g. re-hatch from developer tab), the two could desync.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 3, 2026

Choose a reason for hiding this comment

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

Investigated — SettingsStore is declared as lazy var in AppServices.swift:16, meaning it's only instantiated when first accessed. During onboarding, nothing accesses settingsStore, so it doesn't exist yet. By the time SettingsStore is initialized (after onboarding completes), it reads the correct persisted values from UserDefaults.

The re-hatch scenario mentioned is also safe: OnboardingState.resetForRetry() explicitly resets the UserDefaults keys, and a new SettingsStore would be created for the new session. Even if the same SettingsStore instance survives a re-hatch, its Combine sinks write to UserDefaults (not read from it), so the direction of data flow means @AppStorage writes won't be overwritten by stale SettingsStore state.

No action needed — this is a pre-existing architectural limitation (not a regression from this PR) and doesn't manifest in practice.

@AppStorage("tosAccepted") private var tosAccepted: Bool = false
Comment on lines +16 to +18
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 3, 2026

Choose a reason for hiding this comment

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

🔴 @AppStorage immediately persists privacy preferences and ToS acceptance before user clicks "Accept and Start"

The migration from @State (with manual UserDefaults.standard.set() in saveAndContinue()) to @AppStorage changes the persistence semantics: every toggle/checkbox change is now immediately written to UserDefaults, rather than being deferred until the user clicks "Accept and Start".

This means if a user toggles "Share Analytics" off and checks the ToS checkbox, then presses "Back" to navigate away, those changes are already persisted in UserDefaults. The old code only wrote to UserDefaults inside saveAndContinue() (lines 122-124 on the LEFT side), so going back would discard all uncommitted changes. The goBack() method at line 170 does not reset these values.

Impact on tosAccepted specifically

The tosAccepted flag is checked at App/AppDelegate+ConnectionSetup.swift:546 and reset in OnboardingState.swift:209 during retry — but resetForRetry() only runs on explicit re-hatch, not when the user simply navigates back. A user who checks the ToS checkbox and goes back will have tosAccepted = true in UserDefaults even though they never clicked "Accept and Start".

Prompt for agents 
The core issue is that @AppStorage writes to UserDefaults on every mutation, but the previous @State approach deferred persistence until saveAndContinue(). Two approaches to fix:

1. Keep @AppStorage but reset values in goBack(): In goBack(), restore collectUsageData, sendDiagnostics, and tosAccepted to their original values (captured at view init) before navigating back. This requires storing the original values in @State at onAppear.

2. Revert to @State with manual UserDefaults reads at init and explicit writes in saveAndContinue(): This preserves the original semantics where toggle changes are local until committed. The @State properties would be initialized from UserDefaults (as before) and only written back in saveAndContinue().

Approach 2 is simpler and preserves the original transactional semantics. If @AppStorage is preferred for code cleanliness, approach 1 works but requires capturing and restoring original values on cancel/back.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 3, 2026

Choose a reason for hiding this comment

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

This is a known behavioral change that I flagged to the reviewer. The old code deferred persistence to saveAndContinue(), while @AppStorage writes immediately on mutation.

In practice this is acceptable because:

  1. The toggles reflect the user's explicit intent — if they toggle "Share Analytics" off and press "Back", persisting that preference is arguably more correct than silently discarding it.
  2. These settings are also available in Settings, so the user can always change them later.
  3. tosAccepted specifically is protected by the saveAndContinue() safeguard (tosAccepted = true at line 155) — you can only proceed if the checkbox is checked and you click "Accept and Start".
  4. goBack() navigates to a previous onboarding step, not out of the app — the user will return to this screen and see their previous selections preserved, which is better UX than resetting them.

If transactional (deferred) semantics are truly needed in the future, the approach would be to use @State for the UI and only write to @AppStorage/UserDefaults in saveAndContinue(). But that reintroduces the original desync bug this PR fixes, so it would need a different state-management strategy.


var body: some View {
Text("Before You Start")
Expand Down Expand Up @@ -61,7 +61,7 @@ struct ImproveExperienceStepView: View {
// ToS consent checkbox
VCard {
HStack(spacing: VSpacing.md) {
VCheckbox(isOn: $tosAccepted)
tosCheckbox
tosConsentText
}
.frame(maxWidth: .infinity, alignment: .leading)
Expand Down Expand Up @@ -90,6 +90,35 @@ struct ImproveExperienceStepView: View {
}
}

// MARK: - ToS Consent Checkbox

private var tosCheckbox: some View {
Button {
withAnimation(VAnimation.fast) {
tosAccepted.toggle()
}
} label: {
ZStack {
RoundedRectangle(cornerRadius: VRadius.sm)
.fill(tosAccepted ? VColor.primaryBase : Color.clear)

RoundedRectangle(cornerRadius: VRadius.sm)
.strokeBorder(tosAccepted ? Color.clear : VColor.borderBase, lineWidth: 1.5)

if tosAccepted {
VIconView(.check, size: 12)
.foregroundStyle(VColor.auxWhite)
}
}
.frame(width: 20, height: 20)
.contentShape(Rectangle())
}
.buttonStyle(.plain)
.accessibilityLabel("Agree to Terms of Service and Privacy Policy")
.accessibilityValue(tosAccepted ? "Checked" : "Unchecked")
.accessibilityAddTraits(.isToggle)
}

// MARK: - ToS Consent Text

private var tosConsentText: some View {
Expand Down Expand Up @@ -119,9 +148,10 @@ struct ImproveExperienceStepView: View {
// MARK: - Actions

private func saveAndContinue() {
UserDefaults.standard.set(collectUsageData, forKey: "collectUsageData")
UserDefaults.standard.set(sendDiagnostics, forKey: "sendDiagnostics")
UserDefaults.standard.set(true, forKey: "tosAccepted")
// @AppStorage already persists collectUsageData, sendDiagnostics, and
// tosAccepted. Explicitly set tosAccepted = true here as a safeguard
// so acceptance is recorded even if the user somehow bypasses the toggle.
tosAccepted = true

if sendDiagnostics {
MetricKitManager.startSentry()
Expand All @@ -143,44 +173,3 @@ struct ImproveExperienceStepView: View {
}
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot Apr 3, 2026

Choose a reason for hiding this comment

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

🚩 OnboardingState.resetOnboarding() resets tosAccepted but goBack() does not

There's an existing UserDefaults.standard.set(false, forKey: "tosAccepted") in OnboardingState.swift:209 inside resetOnboarding(), which handles re-hatch scenarios. However, the goBack() path at line 169-173 of the changed file only decrements currentStep — it does not reset any persisted state. With the old @State code this was fine because nothing was persisted yet. With @AppStorage, this asymmetry means the Back button leaves stale preference values in UserDefaults. If the PR author wants to keep @AppStorage, they would need to add cleanup logic in goBack() or use a local-state-then-commit pattern.

(Refers to lines 169-173)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}
}

// MARK: - Checkbox

/// A styled checkbox matching the V* component aesthetic: primary-filled with
/// white checkmark when checked, outlined rounded square when unchecked.
private struct VCheckbox: View {
@Binding var isOn: Bool

private let size: CGFloat = 20
private let cornerRadius: CGFloat = VRadius.sm

var body: some View {
Button {
isOn.toggle()
} label: {
ZStack {
if isOn {
RoundedRectangle(cornerRadius: cornerRadius)
.fill(VColor.primaryBase)

VIconView(.check, size: 12)
.foregroundStyle(VColor.auxWhite)
} else {
RoundedRectangle(cornerRadius: cornerRadius)
.fill(Color.clear)
.overlay(
RoundedRectangle(cornerRadius: cornerRadius)
.stroke(VColor.borderBase, lineWidth: 1.5)
)
}
}
.frame(width: size, height: size)
.contentShape(Rectangle())
}
.buttonStyle(.plain)
.animation(VAnimation.fast, value: isOn)
.accessibilityLabel("Agree to Terms of Service and Privacy Policy")
.accessibilityValue(isOn ? "Checked" : "Unchecked")
.accessibilityAddTraits(.isToggle)
}
}