• Improve delete all flow #237 👈 (View in Graphite)
• dev

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Claude finished @yewreeka's task —— View job

Code Review: Improve delete all flow

I've completed a thorough review of PR #237. Overall, this is a well-structured implementation that adds a nice UX improvement for the delete-all flow. However, there are several critical bugs and improvements needed before merge.

🔴 Critical Issues

1. Missing isDeleting = false after successful deletion (AppSettingsViewModel.swift:37)

The isDeleting flag is never reset after successful deletion, which will prevent any future deletion attempts and leave the UI in a loading state indefinitely.

// Lines 33-37
for try await progress in session.deleteAllInboxesWithProgress() {
    deletionProgress = progress
}

// Deletion complete
onComplete()  // ⚠️ Missing: isDeleting = false

Fix: Add isDeleting = false before calling onComplete().

Reference: Macroscope correctly flagged this at AppSettingsViewModel.swift:37.

2. Race condition: No reentrancy guard (AppSettingsViewModel.swift:25)

Multiple concurrent calls to deleteAllData can spawn overlapping deletion tasks. The guard at line 25 doesn't prevent this because it checks isDeleting before setting it, creating a window for race conditions.

func deleteAllData(onComplete: @escaping () -> Void) {
    guard !isDeleting else { return }  // ✅ Good
    isDeleting = true
    // ... rest of deletion
}

While the guard exists, the concern is valid if this method could be called from different contexts rapidly.

Fix: The existing guard is actually sufficient for @MainActor isolation, but ensure UI prevents double-taps.

Reference: Macroscope flagged this at AppSettingsViewModel.swift:28.

3. Non-atomic deletion flow with premature side effects (AppSettingsViewModel.swift:30)

QuicknameSettingsViewModel.shared.delete() is called before the async deletion completes, creating an inconsistent state if deletion fails. The quickname is deleted but inboxes remain.

func deleteAllData(onComplete: @escaping () -> Void) {
    guard !isDeleting else { return }
    isDeleting = true
    deletionError = nil
    deletionProgress = nil

    QuicknameSettingsViewModel.shared.delete()  // ⚠️ Called too early!

    Task {
        do {
            for try await progress in session.deleteAllInboxesWithProgress() {
                deletionProgress = progress
            }
            onComplete()
        } catch {
            deletionError = error
            isDeleting = false
        }
    }
}

Fix: Move QuicknameSettingsViewModel.shared.delete() inside the do block after the async stream completes.

Reference: Macroscope flagged this at AppSettingsViewModel.swift:42.

4. Missing cancellation handler in progress stream (SessionManager.swift:253)

The background Task in deleteAllInboxesWithProgress() is not cancelled when the stream consumer terminates early. This could leave deletion operations running in the background.

AsyncThrowingStream { continuation in
    let task = Task {
        // deletion work
    }
    // ⚠️ Missing: continuation.onTermination = { _ in task.cancel() }
}

Fix: Add continuation.onTermination = { _ in task.cancel() } after creating the task.

Reference: Macroscope flagged this review comment on SessionManager.swift.

5. waitForDeletionComplete() may return immediately (InboxStateMachine.swift:298-312)

The implementation returns immediately if the state is already .idle, .stopping, or .error. But InboxStateManager.delete() calls this right after stopAndDelete(), potentially before the deletion actually starts.

public func waitForDeletionComplete() async {
    for await state in stateSequence {
        switch state {
        case .idle, .stopping:
            return  // ⚠️ May return before deletion even starts
        case .error:
            return
        default:
            continue
        }
    }
}

Fix: Wait for .deleting state first, then wait for terminal state.

Reference: Macroscope flagged this at InboxStateManager.swift:126.

🟡 Medium Priority Issues

6. ShatteringText doesn't regenerate on text change (ShatteringText.swift:61)

The .task(id: text) modifier regenerates values when text changes, which is correct. However, the arrays (letterOffsets, letterRotations, letterScales) are not updated immediately when text length changes, which could cause index-out-of-bounds issues.

.task(id: text) { generateRandomValues() }  // ✅ Already present

The current implementation is actually fine - .task(id:) will call generateRandomValues() when text changes. Macroscope's suggestion to also add .onChange(of: text) is redundant.

Status: No fix needed - current implementation is correct.

Reference: Macroscope comment on ShatteringText.swift.

🟢 Code Quality & Best Practices

✅ Strengths:

1. Good separation of concerns: AppSettingsViewModel properly abstracts business logic from UI
2. Modern Swift concurrency: Excellent use of AsyncThrowingStream for progress reporting
3. Proper @mainactor isolation: UI state updates are correctly isolated
4. Reusable components: HoldToConfirmButton and ShatteringText are well-extracted
5. Comprehensive deletion: SessionManager.deleteAllInboxesWithProgress() properly cleans up device registration, services, and database
6. Progress tracking: Users get clear feedback during the deletion process

📝 Minor Improvements:

1. Add documentation: AppSettingsViewModel.deleteAllData should document the deletion flow
2. Consider adding analytics: Track deletion events for user behavior analysis
3. Error messages: DeleteAllDataView shows raw error.localizedDescription - consider user-friendly messages

🔒 Security Concerns

✅ No security issues found. The deletion flow properly:

• Cleans up device registration
• Removes all local data (keychain, database, XMTP files)
• Unsubscribes from push topics
• Unregisters installations from backend

⚡ Performance Considerations

✅ Good performance design:

1. Parallel deletion with withTaskGroup for multiple services
2. Deferred device registration cleanup ensures it happens even on error
3. Cancellation-aware tasks with proper Task.checkCancellation()

Minor concern: deleteDatabaseFiles() uses synchronous FileManager operations on the main queue. Consider moving to background queue if this becomes slow.

🧪 Test Coverage

⚠️ No tests found for new functionality. Recommend adding:

1. Unit tests for AppSettingsViewModel.deleteAllData()
2. Integration tests for SessionManager.deleteAllInboxesWithProgress()
3. UI tests for DeleteAllDataView hold-to-delete interaction
4. Tests for error scenarios (network failure, database errors)

📋 Summary

Blockers before merge:

1. Fix missing isDeleting = false after successful deletion ✋
2. Move QuicknameSettingsViewModel.shared.delete() to success path ✋
3. Add cancellation handler to progress stream ✋
4. Fix waitForDeletionComplete() race condition ✋

Nice to have:

• Add tests for deletion flow
• Improve error messaging for users
• Add documentation

The core architecture is solid, but the state management issues need to be addressed to prevent bugs in production. The UX improvements (hold-to-delete, progress tracking, shattering animation) are excellent additions.