fix: address PR review comments for mnemonic crash fix#684
Conversation
- Remove unused assertTrue import from VssBackupClientTest - Remove doc comments and inline comments from VssBackupClient - Refactor setup() to return Result<Boolean> - Add DSL-style setupWithRetry() with SetupRetryLogger builder - Move retry logic from BackupRepo to VssBackupClient - Update tests for Result<Boolean> return type Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code ReviewI found 2 issues that need to be addressed: 1. Logger.error() parameter order violation (CLAUDE.md)File: The CLAUDE.md rule (line 178):
Current code: Logger.error("VSS client setup error", it, TAG)Should be: Logger.error("VSS client setup error", e = it, context = TAG)This is the only occurrence in the codebase using positional parameters - all other Logger.error calls use named parameters. Reference: Repository pattern example in CLAUDE.md 2. Race condition: Retry loop continues after stopObservingBackups()File: The original Problem:
Original code (BackupRepo.kt:166): while (attempt < maxAttempts && isObserving) { // Had isObserving check
val success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}New code (VssBackupClient.kt:87): while (attempt < maxAttempts) { // No isObserving check
val result = setup()
// ...
}Suggested fix:
|
|
Claude comments are redundant. |
Addresses review comments from PR #678
Description
This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):
import kotlin.test.assertTruefrom VssBackupClientTest.ktsetup()return type fromBooleantoResult<Boolean>for cleaner error handlingSetupRetryLoggerbuilder class with trailing lambda DSL for customizable logging insetupWithRetry()BackupRepo.setupVssClientWithRetry()toVssBackupClient.setupWithRetry()Preview
N/A - refactoring
QA Notes
./gradlew testDevDebugUnitTest./gradlew detekt