Fix backup tags on activity edit

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉

[ben-kaufman]

Fixes backup of tags when editing activity details.

Test: Go to an actitivy, edit its tags (add and remove), then back up the wallet seed, wipe the wallet, and restore it.

[claude]

Code Review: Fix backup tags on activity edit

Thanks for this fix! This PR addresses an important issue with tag backup when editing activity details. Overall, the changes follow good architectural patterns and improve the codebase. Here's my detailed review:

---

✅ Strengths

1. Excellent Architectural Improvement

   • Properly moves business logic from ViewModels to the Repository layer
   • Follows the Repository pattern correctly as outlined in the project conventions
   • ActivityRepo now properly encapsulates tag-related operations with methods like getActivityTags(), addTagsToActivity(), and removeTagsFromActivity()

2. Improved Error Handling

   • Consistent use of Result<T> type throughout
   • Proper use of .onSuccess {} and .onFailure {} patterns
   • Better error logging with context

3. Consistent Patterns

   • All async operations properly use viewModelScope.launch(bgDispatcher) (not GlobalScope)
   • StateFlow patterns maintained correctly
   • Error messages are informative and include context

4. Repository Encapsulation

   • ViewModels no longer directly access coreService.activity - they go through ActivityRepo instead
   • This ensures that tag changes trigger notifyActivitiesChanged() which is critical for backup sync

---

🔍 Code Quality Observations

ActivityDetailViewModel.kt (app/src/main/java/to/bitkit/viewmodels/ActivityDetailViewModel.kt:48-87)

• ✅ Good: Simplified error handling with Result types
• ✅ Good: Removed manual try-catch in favor of Result's onSuccess/onFailure
• ✅ Good: Consistent pattern across all three methods (loadTags, removeTag, addTag)
• 💡 Note: The addTag method calls settingsStore.addLastUsedTag(tag) on success - this is good for UX but consider if this should also be tracked in case of failures

SendCoinSelectionViewModel.kt (app/src/main/java/to/bitkit/ui/screens/wallets/send/SendCoinSelectionViewModel.kt:74-92)

• ✅ Good: Changed from direct coreService access to activityRepo.getActivityTags()
• ✅ Good: Proper error logging added
• ✅ Good: Removed unnecessary outer runCatching block - the repository method already returns a Result
• 💡 Minor: The empty check if (tags.isNotEmpty()) is appropriate for this use case

ActivityRepo.kt - New methods are well-designed:

• ✅ getActivityTags(): Simple, focused method with proper error handling
• ✅ addTagsToActivity(): Includes deduplication logic and calls notifyActivitiesChanged()
• ✅ removeTagsFromActivity(): Validates activity exists and calls notifyActivitiesChanged()

---

🐛 Potential Issues & Concerns

Critical: Backup Sync Issue (The fix this PR addresses)

Root cause identified correctly: The ViewModels were calling coreService.activity methods directly, bypassing the repository layer. This meant notifyActivitiesChanged() wasn't called, so the backup system didn't know tags had changed.

Fix verification: ✅ All tag operations now go through ActivityRepo methods that properly call notifyActivitiesChanged() after modifications.

Minor: Error Handling Edge Case

In SendCoinSelectionViewModel.kt:74-92, the code handles the case where tags fail to load by logging an error, but the map entry is never added. This means subsequent calls to loadTagsByTxId will retry loading the same tags. This is probably acceptable behavior, but consider if you want to add an empty list entry to prevent repeated failures.

.onFailure { e ->Logger.error("Failed to load tags for utxo $txId", e)
// Consider: _tagsByTxId.update { currentMap -> currentMap + (txId to emptyList()) }
}

---

🔒 Security Considerations

✅ No security concerns identified:

• Tag operations validate activity existence before modification
• Proper use of coroutine scopes prevents memory leaks
• No SQL injection risks (using type-safe repository methods)
• Input validation exists in repository layer (e.g., filter { it.isNotBlank() })

---

⚡ Performance Considerations

✅ Performance looks good:

• Repository methods properly use bgDispatcher for background work
• Tag loading is done lazily in SendCoinSelectionViewModel (only when needed)
• No unnecessary database operations
• Proper use of StateFlow for reactive updates

---

🧪 Test Coverage

Current state: Test file updated minimally (just mock change from CoreService to ActivityRepo)

Recommendation: Consider adding tests for the tag-related functionality:

@Test
fun`loadTags updates tags state on success`() = test {
val activityId ="test-activity-id"val expectedTags =listOf("tag1", "tag2")
val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.getActivityTags(activityId))
.thenReturn(Result.success(expectedTags))
sut.setActivity(mockActivity)
sut.loadTags()
assertEquals(expectedTags, sut.tags.value)
}
@Test
fun`addTag calls settingsStore and reloads tags on success`() = test {
val activityId ="test-activity-id"val tag ="new-tag"val mockActivity = mock<Activity.Onchain>()
whenever(activityRepo.addTagsToActivity(activityId, listOf(tag)))
.thenReturn(Result.success(Unit))
sut.setActivity(mockActivity)
sut.addTag(tag)
verify(settingsStore).addLastUsedTag(tag)
verify(activityRepo).getActivityTags(activityId) // loadTags called
}

However, these additions are not blocking - the current fix is solid and the existing test infrastructure proves the refactoring maintains compatibility.

---

📝 Summary & Recommendation

Verdict: ✅ Approve with minor suggestions

This PR successfully fixes the tag backup issue by properly routing tag operations through the repository layer. The architectural improvements are significant and align perfectly with the project's conventions.

Key improvements:

1. Tags are now properly backed up because notifyActivitiesChanged() is called
2. Better separation of concerns (ViewModels → Repository → CoreService)
3. Consistent error handling throughout
4. More maintainable code structure

Minor suggestions (non-blocking):

1. Consider retry behavior for tag loading failures in SendCoinSelectionViewModel
2. Consider adding unit tests for tag operations (can be done in a follow-up PR)

Test plan verification: The described test (edit tags → backup → wipe → restore) should now work correctly since all tag modifications trigger the activitiesChanged notification that the backup system monitors.

Great work! 🎉