[#698] Add a foundation for Rating Prompt feature [PART 2]#700
Open
thinh2k1310 wants to merge 1 commit intofeature/#698-foundation-for-rating-promptfrom
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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. Comment |
thinh2k1310
requested review from
Shayokh144,
Thieurom,
donmartial,
dquangit,
ducbm051291,
kevinhoangpq,
markgravity,
minhnimble,
nhgia,
nkhanh44,
nmint8m,
patcharapon-j,
phongvhd93,
roman-nimble,
suho and
vnntsu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift (1)
29-53:⚠️ Potential issue | 🟡 Minor
recordAppLaunch()is coupled torestoreSessionIfNeeded— may miss or double-count launches.Placing
recordAppLaunch()insiderestoreSessionIfNeededties "app launch" semantics to the landing screen's lifecycle. Two concerns:
- If the landing view is re-entered after sign-out (a new
LandingViewModelwithhasRestoredSession == false), this will increment the launch counter again within the same app process — inflating the rating-prompt eligibility signal.- Conversely, if the app is launched directly into a deep link or another flow that bypasses Landing, the launch will never be recorded.
Consider calling
recordAppLaunch()from a single app-level entry point (e.g.,App.init/ scenedidBecomeActive) instead, and keepingLandingViewModelfocused on session restoration + prompt triggering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift` around lines 29 - 53, The call to ratingPromptStorage.recordAppLaunch() inside LandingViewModel.restoreSessionIfNeeded() couples app-launch counting to the landing screen and can double-count or miss launches; remove that call from restoreSessionIfNeeded() (leave restoreSessionIfNeeded(), checkForceUpdateUseCase(), sessionRepository.hasActiveSession(), and tryShowRatingPrompt() intact) and instead invoke ratingPromptStorage.recordAppLaunch() exactly once from a single app-level entry point (e.g., App.init or scene didBecomeActive) so launches are recorded once per process start; ensure the app-level caller has access to ratingPromptStorage (inject or pass it) and update any related tests to reflect the new single responsibility.
🧹 Nitpick comments (3)
template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift (1)
50-60: Rating prompt never reached for returning signed-in users who don't pass through demo flow, and fires on every demo tap.Two observations on the trigger points:
- Line 58:
recordSignificantEvent()runs every time the user taps "Continue with Demo". If a user repeatedly taps it (or re-enters the flow), significant-event count inflates artificially — it's not really a "significant" signal.- Line 60 unconditionally fires the prompt right after saving the demo session, on the same interaction. That's the same tick as the event being recorded; the prompt is driven almost entirely by launch count + days rather than this new event.
Consider whether a demo-session entry truly qualifies as a significant event, and whether prompting immediately on the same action is the desired UX vs. deferring to the next meaningful milestone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift` around lines 50 - 60, The current continueWithDemoSession() calls ratingPromptStorage.recordSignificantEvent() and then immediately awaits tryShowRatingPrompt(), causing demo taps to inflate event counts and trigger the prompt on the same interaction. Change continueWithDemoSession() so it only saves the demo session (sessionRepository.save(tokenSet: DemoTokenSet())) and sets state = .signedIn, but do NOT call ratingPromptStorage.recordSignificantEvent() on every demo tap and do NOT call tryShowRatingPrompt() immediately; instead record the significant event only once (e.g., when a demo session is first created or on a later meaningful milestone) and let the existing state observer that runs if state == .signedIn invoke tryShowRatingPrompt() on subsequent app flow (or defer recording/prompting to a delayed/next-session check).template/Modules/Domain/Tests/Sources/Entities/RatingPromptDataTests.swift (1)
35-43: Potential flakiness around day-boundary / DST.
daysSinceFirstLaunchis likely computed viaCalendar.dateComponents(.day, ...), which can yield6or8instead of7around DST transitions or when the arithmetic happens near midnight. Consider using a fixed reference date (inject anowprovider) or asserting a small tolerance range to avoid rare CI flakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Domain/Tests/Sources/Entities/RatingPromptDataTests.swift` around lines 35 - 43, The test is flaky around day boundaries/DST; update the code and test to use an injectable reference "now" so calculations are deterministic: add an optional now: Date parameter (defaulting to Date()) to RatingPromptData's initializer and have daysSinceFirstLaunch compute days using Calendar and startOfDay on both dates (or otherwise use the injected now) to avoid DST/midnight issues; then change the test calculatesDaysSinceFirstLaunchCorrectly to create a fixed "now" Date, compute sevenDaysAgo from that fixed now, construct RatingPromptData(firstLaunchDate:sevenDaysAgo, now:fixedNow) and assert data.daysSinceFirstLaunch == 7.template/Modules/Data/Tests/Sources/Mocks/UserDefaultsManagerMock.swift (1)
21-27: Align mock'ssetObject/getObjectwith production encoding semantics.The mock stores raw
Treferences and retrieves them viaas? T, whereasUserDefaultsManagerencodesCodablevalues toDatausingJSONEncoderand decodes viaJSONDecoder. This mismatch won't catch bugs where encoding/decoding fails in production (e.g.,Codable-incompatible types, key mismatches, or mixedData/typed reads). Current tests pass becauseRatingPromptStorageusesgetDataValuewith manual encoding rather thansetObject/getObject, but the mock contract is misleading for future callers.Proposed fix
func setObject<T: Codable>(_ value: T?, key: String) { - if let value = value { - storage[key] = value + if let value = value, let data = try? JSONEncoder().encode(value) { + storage[key] = data } else { storage.removeValue(forKey: key) } } func getObject<T: Codable>(ofType: T.Type, key: String) -> T? { - return storage[key] as? T + guard let data = storage[key] as? Data else { return nil } + return try? JSONDecoder().decode(T.self, from: data) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/Modules/Data/Tests/Sources/Mocks/UserDefaultsManagerMock.swift` around lines 21 - 27, The mock's setObject<T: Codable>(_ value:key:) and getObject<T: Codable>(_ key:) currently store raw T references and cast back with as? T, which diverges from production UserDefaultsManager that encodes Codable values to Data with JSONEncoder and decodes with JSONDecoder; update UserDefaultsManagerMock so setObject encodes value with JSONEncoder (storing Data or removing key on nil) and getObject decodes stored Data via JSONDecoder to T? (returning nil on encode/decode failure) to mirror production semantics; refer to the methods setObject<T: Codable>(_ value: T?, key: String) and getObject<T: Codable>(_ key: String) in the mock and ensure the mock's internal storage holds Data, not raw T, and handles encoding/decoding errors gracefully (return nil) to match real behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/Modules/Data/Tests/Sources/Mocks/UserDefaultsManagerMock.swift`:
- Around line 65-67: The mock's clearDataForCommonKeys() currently removes all
entries via storage.removeAll(), but should mirror the real implementation by
only removing keys defined in UserDefaultsKey.allCases; change
clearDataForCommonKeys() to iterate over UserDefaultsKey.allCases and call
storage.removeValue(forKey:) (or equivalent) for each key so only those keys are
cleared and other stored values remain intact.
In
`@template/Modules/Data/Tests/Sources/Services/DefaultRatingPromptPresenterTests.swift`:
- Around line 51-64: Remove the class-level `@MainActor` from
SpyStoreReviewController so its init is not main-actor-isolated (leave the
protocol-required `@MainActor` on requestReview()), and drop the redundant
`@unchecked` Sendable if not needed; alternatively, mark the test scope as
`@MainActor` (or serialize the suite) so constructing SpyStoreReviewController()
from plain async tests doesn't violate Swift 6 concurrency rules; apply the same
change pattern to DefaultRatingPromptPresenter where the class-level `@MainActor`
is redundantly applied.
---
Outside diff comments:
In
`@template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift`:
- Around line 29-53: The call to ratingPromptStorage.recordAppLaunch() inside
LandingViewModel.restoreSessionIfNeeded() couples app-launch counting to the
landing screen and can double-count or miss launches; remove that call from
restoreSessionIfNeeded() (leave restoreSessionIfNeeded(),
checkForceUpdateUseCase(), sessionRepository.hasActiveSession(), and
tryShowRatingPrompt() intact) and instead invoke
ratingPromptStorage.recordAppLaunch() exactly once from a single app-level entry
point (e.g., App.init or scene didBecomeActive) so launches are recorded once
per process start; ensure the app-level caller has access to ratingPromptStorage
(inject or pass it) and update any related tests to reflect the new single
responsibility.
---
Nitpick comments:
In `@template/Modules/Data/Tests/Sources/Mocks/UserDefaultsManagerMock.swift`:
- Around line 21-27: The mock's setObject<T: Codable>(_ value:key:) and
getObject<T: Codable>(_ key:) currently store raw T references and cast back
with as? T, which diverges from production UserDefaultsManager that encodes
Codable values to Data with JSONEncoder and decodes with JSONDecoder; update
UserDefaultsManagerMock so setObject encodes value with JSONEncoder (storing
Data or removing key on nil) and getObject decodes stored Data via JSONDecoder
to T? (returning nil on encode/decode failure) to mirror production semantics;
refer to the methods setObject<T: Codable>(_ value: T?, key: String) and
getObject<T: Codable>(_ key: String) in the mock and ensure the mock's internal
storage holds Data, not raw T, and handles encoding/decoding errors gracefully
(return nil) to match real behavior.
In `@template/Modules/Domain/Tests/Sources/Entities/RatingPromptDataTests.swift`:
- Around line 35-43: The test is flaky around day boundaries/DST; update the
code and test to use an injectable reference "now" so calculations are
deterministic: add an optional now: Date parameter (defaulting to Date()) to
RatingPromptData's initializer and have daysSinceFirstLaunch compute days using
Calendar and startOfDay on both dates (or otherwise use the injected now) to
avoid DST/midnight issues; then change the test
calculatesDaysSinceFirstLaunchCorrectly to create a fixed "now" Date, compute
sevenDaysAgo from that fixed now, construct
RatingPromptData(firstLaunchDate:sevenDaysAgo, now:fixedNow) and assert
data.daysSinceFirstLaunch == 7.
In
`@template/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift`:
- Around line 50-60: The current continueWithDemoSession() calls
ratingPromptStorage.recordSignificantEvent() and then immediately awaits
tryShowRatingPrompt(), causing demo taps to inflate event counts and trigger the
prompt on the same interaction. Change continueWithDemoSession() so it only
saves the demo session (sessionRepository.save(tokenSet: DemoTokenSet())) and
sets state = .signedIn, but do NOT call
ratingPromptStorage.recordSignificantEvent() on every demo tap and do NOT call
tryShowRatingPrompt() immediately; instead record the significant event only
once (e.g., when a demo session is first created or on a later meaningful
milestone) and let the existing state observer that runs if state == .signedIn
invoke tryShowRatingPrompt() on subsequent app flow (or defer
recording/prompting to a delayed/next-session check).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eab6ec1-0c00-442d-8212-01b765997a46
📒 Files selected for processing (7)
template/Modules/Data/Tests/Sources/Mocks/UserDefaultsManagerMock.swifttemplate/Modules/Data/Tests/Sources/Repositories/RatingPromptStorageTests.swifttemplate/Modules/Data/Tests/Sources/Services/DefaultRatingPromptPresenterTests.swifttemplate/Modules/Domain/Tests/Sources/Entities/RatingPromptDataTests.swifttemplate/Modules/Domain/Tests/Sources/UseCases/RequestRatingPromptUseCaseTests.swifttemplate/Modules/Domain/Tests/Sources/UseCases/ShouldShowRatingPromptUseCaseTests.swifttemplate/Tuist/Interfaces/SwiftUI/Sources/Presentation/Modules/Landing/LandingViewModel.swift
thinh2k1310
force-pushed
the
feature/#698-foundation-for-rating-prompt-part-2
branch
from
9a8f7b0 to
e77a10b
Compare
ducbm051291
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened 👀
LandingViewModel.RatingPromptStorageTestsDefaultRatingPromptPresenterTestsRatingPromptDataTestsRequestRatingPromptUseCaseTestsShouldShowRatingPromptUseCaseTestsInsight 📝
N/A
Proof Of Work 📹
Summary by CodeRabbit
Release Notes
New Features
Tests