-
Notifications
You must be signed in to change notification settings - Fork 1
fix: unify timed sheets behavior #556
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.
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
base: feat/nav3
Are you sure you want to change the base?
Conversation
|
@piotr-iohk cc |
… into fix/uniffy-timed-sheet-behavior # Conflicts: # app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
# Conflicts: # app/src/main/java/to/bitkit/ui/ContentView.kt # app/src/main/java/to/bitkit/ui/nav/Navigator.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
# Conflicts: # app/src/main/java/to/bitkit/ui/nav/entries/HomeEntries.kt
|
Waiting for #554 |
app/src/main/java/to/bitkit/utils/timedsheets/sheets/AppUpdateTimedSheet.kt
Fixed
Show fixed
Hide fixed
|
found a navigation transition inconsistency not related to this branch Screen_recording_20251226_064504.webm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the timed sheets system by extracting logic into a dedicated TimedSheetManager class and individual sheet implementations. The changes remove the balance-based trigger mechanism (aligning with iOS behavior), introduce proper separation of concerns through individual sheet classes, and add comprehensive unit test coverage.
Key changes:
- Introduced
TimedSheetManagerto centralize sheet display logic and queue management - Created individual sheet implementations (AppUpdateTimedSheet, BackupTimedSheet, etc.) following the
TimedSheetIteminterface - Removed balance change observer trigger, replacing with manual check on home screen entry
- Added comprehensive unit tests for all sheet types and the manager
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/test/java/to/bitkit/utils/timedsheets/sheets/QuickPayTimedSheetTest.kt | Unit tests for QuickPay sheet logic covering display conditions and dismissal behavior |
| app/src/test/java/to/bitkit/utils/timedsheets/sheets/NotificationsTimedSheetTest.kt | Unit tests for Notifications sheet including timeout logic and balance requirements |
| app/src/test/java/to/bitkit/utils/timedsheets/sheets/HighBalanceTimedSheetTest.kt | Unit tests for HighBalance sheet covering USD threshold checks, warning limits, and timeout behavior |
| app/src/test/java/to/bitkit/utils/timedsheets/sheets/BackupTimedSheetTest.kt | Unit tests for Backup sheet verification and timeout logic |
| app/src/test/java/to/bitkit/utils/timedsheets/sheets/AppUpdateTimedSheetTest.kt | Unit tests for AppUpdate sheet covering version checking and critical update filtering |
| app/src/test/java/to/bitkit/utils/timedsheets/TimedSheetManagerTest.kt | Unit tests for TimedSheetManager covering queue management, priority sorting, and lifecycle |
| app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt | Refactored to use TimedSheetManager, removed inline sheet logic, extracted critical update check |
| app/src/main/java/to/bitkit/utils/timedsheets/sheets/QuickPayTimedSheet.kt | Implementation of QuickPay sheet display and dismissal logic |
| app/src/main/java/to/bitkit/utils/timedsheets/sheets/NotificationsTimedSheet.kt | Implementation of Notifications sheet with timeout and balance checks |
| app/src/main/java/to/bitkit/utils/timedsheets/sheets/HighBalanceTimedSheet.kt | Implementation of HighBalance sheet with USD conversion and warning limits |
| app/src/main/java/to/bitkit/utils/timedsheets/sheets/BackupTimedSheet.kt | Implementation of Backup sheet with verification and timeout logic |
| app/src/main/java/to/bitkit/utils/timedsheets/sheets/AppUpdateTimedSheet.kt | Implementation of AppUpdate sheet with version checking and critical filtering |
| app/src/main/java/to/bitkit/utils/timedsheets/TimedSheetUtils.kt | Utility functions for timeout checking and interval constants |
| app/src/main/java/to/bitkit/utils/timedsheets/TimedSheetManager.kt | Manager class for sheet registration, queue management, and display logic |
| app/src/main/java/to/bitkit/utils/timedsheets/TimedSheetItem.kt | Interface defining the contract for timed sheet implementations |
| app/src/main/java/to/bitkit/ui/nav/entries/SheetEntries.kt | Updated sheet entries to use DisposableEffect for dismissal handling |
| app/src/main/java/to/bitkit/ui/nav/SheetSceneStrategy.kt | Added automatic dismissal on swipe-down gesture detection |
| app/src/main/java/to/bitkit/ui/nav/Navigator.kt | Added navigateToCriticalUpdate method for clearing backstack |
| app/src/main/java/to/bitkit/ui/ContentView.kt | Added special routing logic for critical update navigation |
| app/src/main/java/to/bitkit/di/TimedSheetModule.kt | Dependency injection module for TimedSheetManager provider |
| delay(CHECK_DELAY_MILLIS) | ||
| checkAndShowNextSheet() |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delay before showing the next sheet in queue creates a 2-second gap between sheets. Consider making this configurable or using a smaller delay for queued sheets to improve user experience, as the initial delay already occurred when entering the home screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This delay helps the user don't be overwhelmed when there is multiple sheets
| currentTimedSheet = sheet | ||
| _currentSheet.value = sheet.type | ||
| sheet.onShown() | ||
| registeredSheets.remove(sheet) |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing sheets from registeredSheets after showing means they cannot be shown again even if the user re-enters the home screen and conditions change. If sheets should be eligible to show again on subsequent home screen visits, this removal should be reconsidered. Consider tracking shown sheets separately or allowing sheets to reset their eligibility.
| registeredSheets.remove(sheet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the sheets has a logic to be re-triggered on a short time. removing the registered sheet helps not repeating the checks
e57eef3 to
1eb159f
Compare
Closes #448
Closes #497
Description
This PR extract the timed sheets logic to a manager class, removes the trigger on balance change (like iOS) and build a robust Unit test flow
Preview
basic_flow.webm
iOS.mp4
after_two_days.webm
critical.update.webm
QA Notes