Skip to content

Fix ToS checkbox state desync using @AppStorage#23419

Merged
ashleeradka merged 4 commits into
mainfrom
devin/1775245446-fix-tos-checkbox-state
Apr 3, 2026
Merged

Fix ToS checkbox state desync using @AppStorage#23419
ashleeradka merged 4 commits into
mainfrom
devin/1775245446-fix-tos-checkbox-state

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

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

Summary

Fixes a bug where returning users see an unchecked ToS checkbox despite having previously accepted, causing the "Accept and Start" button to appear enabled with a visually-unchecked box (or vice versa after clicking).

Root cause: @State initialized from UserDefaults.standard.bool(forKey:) captures a snapshot at view init time. When OnboardingFlowView uses .id() to drive step transitions, SwiftUI recreates the view's identity but @State can lose sync with the underlying UserDefaults value — the checkbox visual and the actual persisted state diverge.

Changes (single file — ImproveExperienceStepView.swift):

  1. @State@AppStorage for collectUsageData, sendDiagnostics, and tosAccepted — uses Apple's KVO-backed wrapper that stays in sync with UserDefaults across view identity changes (Apple docs: @AppStorage)

  2. Inlined checkbox as tosCheckbox computed property — replaced the separate private struct VCheckbox: View with a tosCheckbox computed property on the main view. Uses a Button with .buttonStyle(.plain) wrapping only the checkbox indicator, keeping the legal-link text as a separate sibling so link taps open URLs without toggling consent. Same visual design (primary-filled square with checkmark / outlined square), with proper accessibility traits (.isToggle, label, value).

  3. Simplified saveAndContinue() — removed redundant UserDefaults.standard.set(...) calls since @AppStorage persists automatically. Kept an explicit tosAccepted = true as a safeguard.

Why this is safe:

  • Changes are scoped to ImproveExperienceStepView.swift only.
  • Other code that reads/writes these UserDefaults keys (OnboardingState.resetForRetry(), ManagedAssistantConnectionCoordinator, SettingsStore, MetricKitManager) all access UserDefaults directly, which @AppStorage observes correctly via KVO.

References:

Note: CI skips macOS builds — this must be verified locally in Xcode.

Review & Testing Checklist for Human

  • Build in Xcode: CI cannot verify macOS compilation. Confirm the project builds without errors.
  • Checkbox interaction: Click the ToS checkbox and verify it toggles between checked/unchecked, and that "Accept and Start" enables/disables accordingly.
  • Returning user flow: Set tosAccepted = true in UserDefaults, then re-launch onboarding. Confirm the checkbox renders as checked and the "Accept and Start" button is enabled.
  • Fresh user flow: Clean UserDefaults and verify the checkbox starts unchecked.
  • Link taps vs. checkbox toggle: Verify that tapping "Terms of Service" and "Privacy Policy" links opens URLs in a browser without toggling the checkbox.

Notes

  • @AppStorage writes to UserDefaults on every toggle change (not just on save). This is a minor behavioral difference from the old code but is the expected/correct pattern — the toggle reflects the user's intent immediately, and these settings are available in Settings anyway.
  • The strokeBorder used in the new code is slightly different from the old stroke — it insets the stroke within the shape bounds rather than centering it on the edge. Visual difference is negligible at 1.5pt width on a 20×20 box.

Link to Devin session: https://app.devin.ai/sessions/8cc392bb9c9c46b5a8655fff25f7ce24
Requested by: @ashleeradka


Open with Devin

Replace @State initialized from UserDefaults with @AppStorage for
collectUsageData, sendDiagnostics, and tosAccepted. This fixes a bug
where returning users saw an unchecked checkbox despite tosAccepted
being true in UserDefaults, because @State only snapshots the value
at init and doesn't stay in sync with UserDefaults across view
identity changes.

Convert the Button-based VCheckbox to a ToggleStyle (CheckboxToggleStyle)
following Apple's recommended pattern for custom toggle appearances.
This provides proper accessibility traits and standard binding lifecycle.

Simplify saveAndContinue() by removing redundant UserDefaults.set()
calls that @AppStorage handles automatically.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

chatgpt-codex-connector[bot]

This comment was marked as resolved.

devin-ai-integration Bot and others added 2 commits April 3, 2026 19:50
Move the tap interaction from the HStack (which covered the label
with clickable legal links) to a Button wrapping only the checkbox
indicator. This prevents link taps from toggling the consent state.

Also restores keyboard focusability (Button is natively focusable)
and adds .accessibilityAddTraits(.isToggle) and
.accessibilityElement(children: .combine) for proper VoiceOver.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Toggle with a custom ToggleStyle intercepts clicks on macOS before
they reach the inner Button, making the checkbox unresponsive.

Replace with a direct Button-based checkbox (tosCheckbox computed
property) that reliably handles taps on macOS. The Button is natively
keyboard-focusable and includes proper accessibility traits.

The core fix (@AppStorage for UserDefaults sync) remains unchanged.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration devin-ai-integration Bot changed the title Fix ToS checkbox state desync using @AppStorage and ToggleStyle Fix ToS checkbox state desync using @AppStorage Apr 3, 2026
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 new potential issues.

View 3 additional findings in Devin Review.

Open in Devin Review

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

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

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.

Comment on lines +16 to +17
@AppStorage("collectUsageData") private var collectUsageData: Bool = true
@AppStorage("sendDiagnostics") private var sendDiagnostics: Bool = true
Copy link
Copy Markdown
Contributor Author

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

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.

The withAnimation in the Button action already animates the state
change; the .animation modifier on the view was redundant.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@ashleeradka ashleeradka merged commit aaed536 into main Apr 3, 2026
6 checks passed
@ashleeradka ashleeradka deleted the devin/1775245446-fix-tos-checkbox-state branch April 3, 2026 20:04
Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

@@ -143,44 +173,3 @@ struct ImproveExperienceStepView: View {
}
Copy link
Copy Markdown
Contributor Author

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.

noanflaherty pushed a commit that referenced this pull request Apr 3, 2026
* fix: use @AppStorage and ToggleStyle for ToS checkbox state

Replace @State initialized from UserDefaults with @AppStorage for
collectUsageData, sendDiagnostics, and tosAccepted. This fixes a bug
where returning users saw an unchecked checkbox despite tosAccepted
being true in UserDefaults, because @State only snapshots the value
at init and doesn't stay in sync with UserDefaults across view
identity changes.

Convert the Button-based VCheckbox to a ToggleStyle (CheckboxToggleStyle)
following Apple's recommended pattern for custom toggle appearances.
This provides proper accessibility traits and standard binding lifecycle.

Simplify saveAndContinue() by removing redundant UserDefaults.set()
calls that @AppStorage handles automatically.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>

* fix: scope toggle gesture to checkbox indicator only

Move the tap interaction from the HStack (which covered the label
with clickable legal links) to a Button wrapping only the checkbox
indicator. This prevents link taps from toggling the consent state.

Also restores keyboard focusability (Button is natively focusable)
and adds .accessibilityAddTraits(.isToggle) and
.accessibilityElement(children: .combine) for proper VoiceOver.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>

* fix: replace Toggle+ToggleStyle with direct Button for checkbox

Toggle with a custom ToggleStyle intercepts clicks on macOS before
they reach the inner Button, making the checkbox unresponsive.

Replace with a direct Button-based checkbox (tosCheckbox computed
property) that reliably handles taps on macOS. The Button is natively
keyboard-focusable and includes proper accessibility traits.

The core fix (@AppStorage for UserDefaults sync) remains unchanged.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>

* chore: remove redundant .animation modifier on checkbox

The withAnimation in the Button action already animates the state
change; the .animation modifier on the view was redundant.

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: ashlee@vellum.ai <ashlee@vellum.ai>
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