Skip to content

feat(undo): Wire up undo/redo feature for end users#17

Merged
github-actions[bot] merged 1 commit intomainfrom
feature/wire-undo-ui
Jan 14, 2026
Merged

feat(undo): Wire up undo/redo feature for end users#17
github-actions[bot] merged 1 commit intomainfrom
feature/wire-undo-ui

Conversation

@gilmanb1
Copy link
Copy Markdown
Owner

Summary

Wires up the existing SafeFileOrganizer/UndoStack/MovementLog infrastructure to the UI, making undo available to end users.

Changes

AppState.swift

  • Replace FileOrganizer with SafeFileOrganizer for file organization
  • Initialize UndoStack with 100 operation limit on app startup
  • Add canUndoLastMove/canRedoLastMove/undoableOperationsCount state tracking
  • Add undoLastMove() and redoLastMove() async methods
  • Add updateUndoState() helper to sync UI with undo stack

ContentView.swift

  • Add undo/redo buttons to footer view
  • Add keyboard shortcuts (⌘Z for undo, ⇧⌘Z for redo)
  • Show undoable operations count when available
  • Buttons auto-hide when no undo/redo operations available

Addresses Spec Requirements

From spec.md:

  • Section 1.1: "Trust: Full transparency, preview-before-move, and robust undo"
  • Section 3 UX: "Visible undo/redo affordances near move/organize actions"
  • Section 4.3: "Soft Move Option... Fully undoable"
  • Section 6.3: "Undo System - File-level or batch-level undo"

Testing

  • Build passes (all 265 compilation units)
  • All 257 tests pass
  • Manual testing: Undo buttons appear in footer after file organization
  • Keyboard shortcuts registered

Screenshots

N/A - functional UI addition with minimal visual change (small undo/redo icons in footer)

Migration Notes

None - This is an additive change. The SafeFileOrganizer is now used instead of FileOrganizer, but the fallback to FileOrganizer is preserved if the safe organizer fails to initialize.

- Replace FileOrganizer with SafeFileOrganizer in AppState
- Initialize UndoStack with 100 operation limit
- Add canUndoLastMove/canRedoLastMove state tracking
- Add undoLastMove() and redoLastMove() async methods
- Add undo/redo buttons to footer with keyboard shortcuts (⌘Z, ⇧⌘Z)
- All file operations now logged to movement_log database
- Operations are undoable and tracked for transparency

This wires up the existing SafeFileOrganizer/UndoStack/MovementLog
infrastructure to the UI, making undo available to end users.

Addresses spec.md sections 1.1, 3, 4.3, 6.3 regarding undo system.
@gilmanb1 gilmanb1 self-assigned this Jan 14, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Jan 14, 2026

Claude finished @gilmanb1's task —— View job


PR Review: feat(undo): Wire up undo/redo feature for end users

Todo List

  • Read and analyze AppState.swift changes
  • Read and analyze ContentView.swift changes
  • Review Swift 6 concurrency compliance
  • Check memory safety patterns
  • Verify architecture consistency
  • Assess test coverage implications
  • Provide comprehensive review feedback

Review Summary

This PR successfully wires up the existing undo infrastructure to the UI. The implementation is well-architected and follows Swift 6 concurrency best practices. I found no critical issues that would prevent merging.

Approve

Key Strengths

🟢 Excellent Swift 6 Concurrency Compliance

  • Proper @MainActor isolation for AppState (AppState.swift:42-43)
  • Correct async/await patterns for undo operations (AppState.swift:710-751)
  • Safe actor boundary crossings with proper await calls (AppState.swift:704-706)

🟢 Memory Safety

  • Proper [weak self] capture in closures (AppState.swift:224, 291)
  • No retain cycles detected in the new undo functionality
  • Actor-isolated UndoStack prevents data races

🟢 Architecture Consistency

  • Clean separation: UndoStack (actor) ↔ SafeFileOrganizer (actor) ↔ AppState (@mainactor)
  • Follows existing patterns for async operations and error handling
  • Command pattern implementation for undo operations is solid

🟢 Test Coverage

  • Existing SafeFileOrganizerTests.swift covers the underlying infrastructure
  • All 257 tests pass, build succeeds with 265 compilation units

Minor Suggestions

🔵 Suggestion: Consider adding error recovery in updateUndoState() (AppState.swift:696-707)

func updateUndoState() async {
    guard let undoStack = undoStack else {
        // Reset state safely
        canUndoLastMove = false
        canRedoLastMove = false  
        undoableOperationsCount = 0
        return
    }
    
    // Add do-catch for robustness
    do {
        canUndoLastMove = await undoStack.canUndo
        canRedoLastMove = await undoStack.canRedo
        undoableOperationsCount = await undoStack.undoCount
    } catch {
        NSLog("⚠️ Failed to update undo state: \(error)")
        // Set safe defaults
        canUndoLastMove = false
        canRedoLastMove = false
        undoableOperationsCount = 0
    }
}

🔵 Suggestion: The keyboard shortcuts (ContentView.swift:325, 336) are properly implemented but could benefit from accessibility labels:

.help("Undo last file move (⌘Z)")
.accessibilityLabel("Undo last move")

Technical Assessment

Swift 6 Compliance: ✅ Excellent

  • All actor boundaries properly handled
  • No @unchecked Sendable misuse
  • Proper isolation domains maintained

Memory Management: ✅ Clean

  • No memory leaks or retain cycles
  • Proper weak references in closures
  • Safe actor lifecycle management

Error Handling: ✅ Robust

  • Graceful degradation when undo unavailable
  • Proper error propagation in async operations
  • User-visible error messages via lastError

Integration: ✅ Seamless

  • UI updates reactively to undo state changes
  • Keyboard shortcuts follow macOS conventions
  • Visual feedback (button states, operation counts) works correctly

Specification Compliance

This PR addresses all requirements from spec.md:

  • Section 1.1: "robust undo" - Implemented with UndoStack and command pattern
  • Section 3 UX: "Visible undo/redo affordances" - Buttons in footer with counts
  • Section 4.3: "Fully undoable" - All file operations go through SafeFileOrganizer
  • Section 6.3: "File-level undo" - Individual file move commands are undoable

Final Recommendation: This is a clean, well-implemented feature that integrates smoothly with the existing codebase. The code quality is high and follows all established patterns. Ready to merge.

@github-actions github-actions Bot merged commit 3cbb7dd into main Jan 14, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant