-
Notifications
You must be signed in to change notification settings - Fork 1
chore: fix all warnings and code cleanup #594
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: master
Are you sure you want to change the base?
Conversation
# Conflicts: # .gitignore # README.md # app/src/main/java/to/bitkit/services/LightningService.kt
app/src/main/java/to/bitkit/ui/screens/transfer/SpendingConfirmScreen.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/to/bitkit/ui/screens/transfer/components/ProgressSteps.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/to/bitkit/ui/screens/transfer/components/ProgressSteps.kt
Fixed
Show fixed
Hide fixed
# Conflicts: # app/src/main/java/to/bitkit/services/RNBackupClient.kt
c8630bb to
f4125ff
Compare
This comment has been minimized.
This comment has been minimized.
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 performs comprehensive code cleanup and quality improvements across the codebase, addressing warnings, localizing hardcoded strings, and implementing new features for boost fee estimation and weather widget caching.
Changes:
- Adds dynamic boost fee time estimates based on fee rates
- Implements API response caching for weather widget
- Localizes remaining hardcoded strings throughout the UI
- Refactors error handling to use
runCatchingpattern consistently - Removes deprecated code and updates dependencies
- Converts NotificationUtils object functions to extension functions
- Simplifies state management by removing combined
MainUiStatein favor of direct repository state usage
Reviewed changes
Copilot reviewed 174 out of 176 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Updates AGP to 8.13.2 and adds compose stability analyzer plugin |
| docs/strings.md | Adds comprehensive documentation of dev-only strings that don't need translation |
| config/detekt/detekt.yml | Expands magic number ignore list for common technical values |
| app/src/main/res/values/strings.xml | Adds numerous localized strings for errors, settings, and UI elements |
| app/src/main/java/to/bitkit/ui/sheets/BoostTransactionViewModel.kt | Implements dynamic fee time estimation based on blocktank fee rates |
| app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt | Removes deprecated combined uiState in favor of individual state flows |
| app/src/main/java/to/bitkit/services/LightningService.kt | Extensive cleanup including better logging, error handling, and removing deprecated fee conversion |
| app/src/main/java/to/bitkit/utils/Errors.kt | Converts sealed class error objects to classes for proper exception cause chains |
| app/src/main/java/to/bitkit/ui/utils/NotificationUtils.kt | Deleted file - functions converted to Context extensions |
| app/src/main/java/to/bitkit/ui/Notifications.kt | Adds notification utility extension functions for Context |
Code ReviewI found 10 high-signal issues that need attention: Critical Bugs (2)1. Variable Shadowing Bug in ActivityListViewModelFile: app/src/main/java/to/bitkit/viewmodels/ActivityListViewModel.kt bitkit-android/app/src/main/java/to/bitkit/viewmodels/ActivityListViewModel.kt Lines 91 to 93 in 9d05e20
Issue: The inner it inside update { } shadows the outer it from collect. This will cause filtered activities to never update. Fix - revert to original: 2. Non-local Returns Bypass Cleanup in TransferViewModelFile: app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt bitkit-android/app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt Lines 229 to 263 in 9d05e20
Issue: Non-local return statements skip the .also { } cleanup block that replaced the original finally block. Fix: Use labeled returns: return@runCatching Result.failure(...) CLAUDE.md Violations (8)Rule: NEVER use file-level @file:Suppress(...) annotations Files with violations:
Recommendation: Move @Suppress annotations to specific declarations (class/function/property level). Summary: 2 critical bugs + 8 CLAUDE.md violations. All issues validated with high confidence. |
This PR:
Description
The PR addresses multiple code quality improvements and new features:
Cleanup
compose-stability-analyzerdependencyPreview
N/A
QA Notes
Tests:
1. Boost Fee Estimates
3. Weather Widget
4. Regression Testing