fix: address PR review comments for mnemonic crash fix

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.

[ovitrif]

Addresses review comments from PR #678

Description

This PR addresses all review comments on PR #678 (fix/MnemonicNotFound-crash):

• Remove unused import - Removed import kotlin.test.assertTrue from VssBackupClientTest.kt
• Remove code comments - Removed doc comment block and inline comments from VssBackupClient.kt
• Refactor to Result - Changed setup() return type from Boolean to Result<Boolean> for cleaner error handling
• DSL-style logging - Added SetupRetryLogger builder class with trailing lambda DSL for customizable logging in setupWithRetry()
• Move retry logic - Moved retry logic from BackupRepo.setupVssClientWithRetry() to VssBackupClient.setupWithRetry()

Preview

N/A - refactoring

QA Notes

• Unit tests: ./gradlew testDevDebugUnitTest
• Lint: ./gradlew detekt

[claude]

Code Review

I found 2 issues that need to be addressed:

1. Logger.error() parameter order violation (CLAUDE.md)

File:app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt:70

The Logger.error() call uses positional parameters instead of named parameters, violating the CLAUDE.md guideline.

CLAUDE.md rule (line 178):

ALWAYS pass the TAG as context to Logger calls, e.g. Logger.debug("message", context = TAG)

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:app/src/main/java/to/bitkit/repositories/BackupRepo.kt:123

The original setupVssClientWithRetry() method had && isObserving in its while loop condition. When this was refactored to VssBackupClient.setupWithRetry(), this check was removed, creating a race condition.

Problem:

1. The new setupWithRetry() while loop doesn't check if backups are still being observed
2. The job calling setupWithRetry() is launched but not tracked in any cancellable job collection (backupJobs, statusObserverJobs, etc.)
3. When stopObservingBackups() is called, the retry loop continues running until either setup succeeds, fails, or max attempts are exhausted

Original code (BackupRepo.kt:166):

while (attempt < maxAttempts && isObserving) { // Had isObserving checkval success = runCatching { vssBackupClient.setup() }.getOrDefault(false)
// ...
}

New code (VssBackupClient.kt:87):

while (attempt < maxAttempts) { // No isObserving checkval result = setup()
// ...
}

Suggested fix:
Either:

1. Store the job reference and cancel it in stopObservingBackups():

   val setupJob = scope.launch {
   vssBackupClient.setupWithRetry { ... }
   }
   statusObserverJobs.add(setupJob) // Add to tracked jobs

2. Or pass a cancellation check to setupWithRetry():

   suspendfunsetupWithRetry(
   shouldContinue: () ->Boolean = { true },
   // ...
   ): Result<Boolean> = withContext(ioDispatcher) {
   while (attempt < maxAttempts && shouldContinue()) { ... }
   }

[ovitrif]

Claude comments are redundant.