refactor(oobe): expose TOS acceptance as UI state and ban event flows in ViewModels - #263
Conversation
…vent OOBEViewModel now holds a TosAcceptance state (Idle, Accepting, Accepted, Navigated). The activity renders it with collectState and opens the main screen from a RESUMED collector on Accepted, then reports it handled. A StateFlow keeps the result across configuration changes without a buffered Channel, and the state cannot hold a navigation request without the progress view. collectEvents loses its last caller and goes away.
ViewModel files may not import, declare or construct Channel or SharedFlow streams, and ui files may not import collectEvents, so one-off results stay in UI state that survives configuration changes. A probe test pins the detection of qualified, typed and inferred uses against false positives from comments, strings and unrelated channels APIs.
An Accepted state reached while the activity is only STARTED must not open MainActivity until it resumes. A recreated activity must not open it a second time once the state is Navigated. Without this test, dropping minActiveState or the onTosAcceptedHandled call would pass the suite.
The event stream rule selects files through a direct ViewModel or AndroidViewModel parent. A ViewModel built on an intermediate base class would escape that rule, so the name rule forces every class named ViewModel onto a direct base the event stream rule recognizes.
The ui rule had no probe, so a broken predicate would pass on the current clean codebase without notice. The probe feeds plain, aliased and lookalike imports through the same predicate the rule uses.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (8)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughOOBE acceptance now uses a ChangesOOBE acceptance flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant OOBEActivity
participant OOBEViewModel
participant MainActivity
OOBEActivity->>OOBEViewModel: onAcceptTos()
OOBEViewModel->>OOBEViewModel: Set Accepting and complete onboarding
OOBEViewModel->>OOBEViewModel: Set Accepted after 500 ms
OOBEViewModel-->>OOBEActivity: Emit Accepted through tosAcceptance
Note over OOBEActivity: Handle Accepted only while RESUMED
OOBEActivity->>MainActivity: Navigate
OOBEActivity->>OOBEViewModel: onTosAcceptedHandled()
OOBEViewModel->>OOBEViewModel: Set Navigated
Merge Risk: ⚪ Minimal · up to The acceptance and navigation changes are ready to merge after normal checks. A narrow architecture-test gap does not block this change. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is contained within onboarding and preserves the existing consent gate. Acceptance is guarded before asynchronous work starts, and navigation waits until the activity is resumed. No newly exposed acceptance path was found, but interruption recovery and the deployment of special builds remain incompletely established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. I’m a rabbit watching states take flight, Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
OOBE state
OOBEViewModelexposes ToS acceptance as aTosAcceptanceStateFlow instead of a Channel event.OOBEActivityrenders the state withcollectStateand opens the main screen from a RESUMED collector.collectEventsloses its last caller and goes away.Architecture rules
collectEvents.ViewModelmust extendViewModelorAndroidViewModeldirectly, so the event stream rule cannot be bypassed.Tests
Behavior changes
TosAcceptancestates:Idle,Accepting,Accepted,Navigated.MainActivityhappens exactly once across recreation.collectEventshelper is deleted.Coverage
Kover floors 100% instruction / 100% branch hold.
Verification
spotlessCheck, detekt, lintDebug, testDebugUnitTest, koverVerifyDebug, verifyRoborazziDebug, pixel9Api35DebugAndroidTest and assembleRelease PASS. The last three review-fix commits touch tests only.
Parity: the only drift is the renovate.json EXACT drift. It clears when the renovate files align.
Review: an Opus review found 1 should-fix and 3 nits. All are fixed.
Notes
OOBEViewModelTestkeeps a MockK forCompleteOnboardingUseCase, because the use case needs aContext.tosAcceptanceis not saved across process death, same as the Channel before.🤖 Generated with Claude Code
https://claude.ai/code/session_01VvNnYRB6Qeb51U6jGybaRF
TosAcceptancestate, and gated navigation on the activity reachingRESUMED.collectEventsand added architecture checks against event streams in ViewModels andcollectEventsimports in UI files.