|
| 1 | +# Code Cleanup Opportunities |
| 2 | + |
| 3 | +**Date**: 2025-11-14 |
| 4 | +**Purpose**: Document technical debt and cleanup opportunities for post-migration |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +## Overview |
| 9 | + |
| 10 | +This document catalogues identified cleanup opportunities that can be addressed after major migration milestones. These are low-priority improvements that don't block core development but enhance code quality. |
| 11 | + |
| 12 | +--- |
| 13 | + |
| 14 | +## Category 1: Dead Code Removal |
| 15 | + |
| 16 | +### 1.1: Commands.swift Dead Code |
| 17 | + |
| 18 | +**File**: `BirchEditor/BirchEditor.swift/BirchEditor/Commands.swift` |
| 19 | +**Lines**: 17-70 |
| 20 | +**Status**: Confirmed dead code (never called) |
| 21 | +**Priority**: Low |
| 22 | +**Effort**: 5 minutes |
| 23 | + |
| 24 | +**Dead Properties**: |
| 25 | +```swift |
| 26 | +@MainActor |
| 27 | +static var scriptCommandsDisposables: [DisposableType]? // Line 20 |
| 28 | +@MainActor |
| 29 | +static var scriptsFolderMonitor: PathMonitor? // Line 22 |
| 30 | +``` |
| 31 | + |
| 32 | +**Dead Functions**: |
| 33 | +```swift |
| 34 | +static func initScriptCommands() { ... } // Lines 43-51 |
| 35 | +static func reloadScriptCommands() { ... } // Lines 53-70 |
| 36 | +``` |
| 37 | + |
| 38 | +**Reason**: |
| 39 | +- `ScriptCommands.swift` provides the active implementation |
| 40 | +- `OutlineEditorAppDelegate.swift:53` calls `ScriptCommands.initScriptCommands()` (not `Commands.initScriptCommands()`) |
| 41 | +- No references to `Commands.scriptCommandsDisposables` or `Commands.scriptsFolderMonitor` |
| 42 | + |
| 43 | +**Removal Plan**: |
| 44 | +1. Delete lines 17-70 from `Commands.swift` |
| 45 | +2. Keep the active code (jsCommands, add(), findCommands(), dispatch()) |
| 46 | +3. Build and test to verify no breakage |
| 47 | +4. Update TODO comment or remove entirely |
| 48 | + |
| 49 | +**When to Remove**: After Phase 2 Swift 6 migration completes |
| 50 | + |
| 51 | +--- |
| 52 | + |
| 53 | +## Category 2: Deprecated API Migration |
| 54 | + |
| 55 | +### 2.1: Legacy Debouncer Removal |
| 56 | + |
| 57 | +**File**: `BirchEditor/BirchEditor.swift/BirchEditor/Debouncer.swift` |
| 58 | +**Lines**: 53-80 |
| 59 | +**Status**: Deprecated (Phase 2 work) |
| 60 | +**Priority**: Medium |
| 61 | +**Effort**: 15 minutes + testing |
| 62 | + |
| 63 | +**Code**: |
| 64 | +```swift |
| 65 | +@available(*, deprecated, message: "Use Actor-based Debouncer(delay: TimeInterval, callback:) instead") |
| 66 | +class LegacyDebouncer: NSObject { ... } |
| 67 | +``` |
| 68 | + |
| 69 | +**Active Usage Sites**: |
| 70 | +- Search for `LegacyDebouncer` usage: |
| 71 | + ```bash |
| 72 | + grep -r "LegacyDebouncer" BirchEditor/ BirchOutline/ TaskPaper/ |
| 73 | + ``` |
| 74 | + |
| 75 | +**Migration Steps**: |
| 76 | +1. Find all `LegacyDebouncer` instantiations |
| 77 | +2. Replace with actor-based `Debouncer` |
| 78 | +3. Update call sites from synchronous to async/await |
| 79 | +4. Remove `LegacyDebouncer` class |
| 80 | + |
| 81 | +**When to Migrate**: After Phase 2 Swift 6 migration (async/await fully adopted) |
| 82 | + |
| 83 | +--- |
| 84 | + |
| 85 | +### 2.2: Legacy delay() Function Removal |
| 86 | + |
| 87 | +**File**: `BirchEditor/BirchEditor.swift/BirchEditor/delay.swift` |
| 88 | +**Lines**: 26-35 |
| 89 | +**Status**: Deprecated (Phase 2 work) |
| 90 | +**Priority**: Medium |
| 91 | +**Effort**: 10 minutes + testing |
| 92 | + |
| 93 | +**Code**: |
| 94 | +```swift |
| 95 | +@available(*, deprecated, message: "Use async delay(_ interval: TimeInterval) instead") |
| 96 | +func delay(_ delay: Double, closure: @escaping () -> Void) { ... } |
| 97 | +``` |
| 98 | + |
| 99 | +**Active Usage Sites**: |
| 100 | +- Search for closure-based delay(): |
| 101 | + ```bash |
| 102 | + grep -r "delay(" BirchEditor/ BirchOutline/ TaskPaper/ | grep "closure:" |
| 103 | + ``` |
| 104 | + |
| 105 | +**Migration Steps**: |
| 106 | +1. Find all closure-based `delay()` calls |
| 107 | +2. Replace with async version: `await delay(interval)` |
| 108 | +3. Add `async` to containing functions |
| 109 | +4. Remove legacy function |
| 110 | + |
| 111 | +**When to Migrate**: After Phase 2 Swift 6 migration |
| 112 | + |
| 113 | +--- |
| 114 | + |
| 115 | +### 2.3: Legacy RemindersStore Methods Removal |
| 116 | + |
| 117 | +**File**: `BirchEditor/BirchEditor.swift/BirchEditor/RemindersStore.swift` |
| 118 | +**Lines**: 51-56, 72-94, 158-196 |
| 119 | +**Status**: Deprecated (Phase 2 work) |
| 120 | +**Priority**: Medium |
| 121 | +**Effort**: 20 minutes + testing |
| 122 | + |
| 123 | +**Deprecated Methods**: |
| 124 | +```swift |
| 125 | +@available(*, deprecated) |
| 126 | +static func requestAccess(to: ..., completion: @escaping (Bool, Error?) -> Void) { ... } |
| 127 | + |
| 128 | +@available(*, deprecated) |
| 129 | +static func fetchReminderCalendars(completion: @escaping ([EKCalendar]) -> Void) { ... } |
| 130 | + |
| 131 | +@available(*, deprecated) |
| 132 | +static func fetchReminders(..., completion: @escaping ([Reminder]) -> Void) { ... } |
| 133 | +``` |
| 134 | + |
| 135 | +**Active Usage Sites**: |
| 136 | +- Search for completion handler versions: |
| 137 | + ```bash |
| 138 | + grep -r "RemindersStore.requestAccess" BirchEditor/ TaskPaper/ |
| 139 | + grep -r "RemindersStore.fetchReminderCalendars" BirchEditor/ TaskPaper/ |
| 140 | + grep -r "RemindersStore.fetchReminders" BirchEditor/ TaskPaper/ |
| 141 | + ``` |
| 142 | + |
| 143 | +**Migration Steps**: |
| 144 | +1. Find all completion handler-based calls |
| 145 | +2. Replace with async/await versions |
| 146 | +3. Remove deprecated methods |
| 147 | + |
| 148 | +**When to Migrate**: After Phase 2 Swift 6 migration |
| 149 | + |
| 150 | +--- |
| 151 | + |
| 152 | +## Category 3: Code Quality Improvements |
| 153 | + |
| 154 | +### 3.1: windowForSheetHack Removal |
| 155 | + |
| 156 | +**File**: `BirchEditor/BirchEditor.swift/BirchEditor/OutlineDocument.swift` |
| 157 | +**Line**: 18 |
| 158 | +**Status**: Hack (workaround for sheet presentation) |
| 159 | +**Priority**: Low |
| 160 | +**Effort**: Unknown (needs investigation) |
| 161 | + |
| 162 | +**Code**: |
| 163 | +```swift |
| 164 | +var windowForSheetHack: NSWindow? |
| 165 | +``` |
| 166 | + |
| 167 | +**Investigation Needed**: |
| 168 | +1. Search for usage of `windowForSheetHack` |
| 169 | +2. Determine if still needed on macOS 11+ |
| 170 | +3. Research modern sheet presentation APIs |
| 171 | +4. Test without hack |
| 172 | + |
| 173 | +**When to Investigate**: After Phase 2, before Phase 3 UI modernization |
| 174 | + |
| 175 | +--- |
| 176 | + |
| 177 | +## Category 4: Optimization Opportunities |
| 178 | + |
| 179 | +### 4.1: Method Swizzling Elimination |
| 180 | + |
| 181 | +**Files**: See `method-swizzling-audit.md` |
| 182 | +**Status**: Documented in Phase 2 |
| 183 | +**Priority**: Medium-High |
| 184 | +**Effort**: 3-7 hours (varies by swizzle) |
| 185 | + |
| 186 | +**Summary**: |
| 187 | +- P2-T07: NSWindowTabbedBase (1-2 hours, low risk) |
| 188 | +- P2-T08/T09: NSTextStorage performance (2-4 hours, medium risk) |
| 189 | +- P2-T10: NSTextView accessibility (2-4 hours, medium risk) |
| 190 | + |
| 191 | +See `MANUAL-XCODE-TASKS.md` sections P2-T07 through P2-T10 for details. |
| 192 | + |
| 193 | +**When to Remove**: Phase 2 method swizzling tasks |
| 194 | + |
| 195 | +--- |
| 196 | + |
| 197 | +## Category 5: Build System Modernization |
| 198 | + |
| 199 | +### 5.1: JavaScript Build System Upgrade |
| 200 | + |
| 201 | +**Files**: `BirchOutline/birch-outline.js/`, `BirchEditor/birch-editor.js/` |
| 202 | +**Status**: Blocked by gulp 3.x incompatibility |
| 203 | +**Priority**: Low (deferred to Phase 4) |
| 204 | +**Effort**: 4-8 hours |
| 205 | + |
| 206 | +See `P1-T10-JavaScript-Build-Test-Results.md` for full analysis. |
| 207 | + |
| 208 | +**Options**: |
| 209 | +1. Upgrade gulp 3→4 (4-8 hours, medium risk) |
| 210 | +2. Defer until Phase 4 Pure Swift migration (eliminate JavaScript entirely) |
| 211 | +3. Use Node.js v11 as workaround if JS changes needed |
| 212 | + |
| 213 | +**Current Decision**: Option 2 (defer until Phase 4) |
| 214 | + |
| 215 | +--- |
| 216 | + |
| 217 | +## Category 6: Security Updates |
| 218 | + |
| 219 | +### 6.1: npm Audit Vulnerabilities |
| 220 | + |
| 221 | +**Status**: 54 vulnerabilities in JavaScript dependencies |
| 222 | +**Priority**: Low (dev dependencies only, not runtime) |
| 223 | +**Effort**: Depends on build system upgrade |
| 224 | + |
| 225 | +**Details**: See `P1-T10-JavaScript-Build-Test-Results.md` |
| 226 | + |
| 227 | +**Breakdown**: |
| 228 | +- 18 critical |
| 229 | +- 27 high |
| 230 | +- 8 moderate |
| 231 | +- 1 low |
| 232 | + |
| 233 | +**When to Fix**: During Category 5.1 build system upgrade, or Phase 4 JavaScript elimination |
| 234 | + |
| 235 | +--- |
| 236 | + |
| 237 | +## Category 7: Documentation Cleanup |
| 238 | + |
| 239 | +### 7.1: Update Inline Comments Post-Migration |
| 240 | + |
| 241 | +**Priority**: Low |
| 242 | +**Effort**: 1-2 hours |
| 243 | + |
| 244 | +**Tasks**: |
| 245 | +1. Remove outdated TODO comments after completing tasks |
| 246 | +2. Update Swift 6 migration comments |
| 247 | +3. Document nonisolated(unsafe) justifications |
| 248 | +4. Add inline explanations for non-obvious concurrency patterns |
| 249 | + |
| 250 | +**When to Do**: After Phase 2 Swift 6 migration |
| 251 | + |
| 252 | +--- |
| 253 | + |
| 254 | +## Execution Priorities |
| 255 | + |
| 256 | +### High Priority (Do in Phase 2) |
| 257 | +1. Method swizzling elimination (P2-T07 through P2-T10) |
| 258 | + |
| 259 | +### Medium Priority (Do in Phase 2/3) |
| 260 | +1. Legacy async API removal (Debouncer, delay, RemindersStore) |
| 261 | +2. windowForSheetHack investigation |
| 262 | + |
| 263 | +### Low Priority (Do in Phase 3/4) |
| 264 | +1. Commands.swift dead code removal |
| 265 | +2. Documentation cleanup |
| 266 | +3. JavaScript build system (or eliminate in Phase 4) |
| 267 | +4. npm audit fixes (or eliminate in Phase 4) |
| 268 | + |
| 269 | +--- |
| 270 | + |
| 271 | +## Testing Strategy |
| 272 | + |
| 273 | +For all cleanup tasks: |
| 274 | +1. Run full test suite before changes (baseline) |
| 275 | +2. Make targeted removal |
| 276 | +3. Build project (⌘B) - verify zero errors |
| 277 | +4. Run full test suite (⌘U) - verify 100% pass rate |
| 278 | +5. Manual smoke test of affected features |
| 279 | +6. Commit with clear description |
| 280 | + |
| 281 | +--- |
| 282 | + |
| 283 | +## Metrics |
| 284 | + |
| 285 | +**Total Cleanup Effort**: ~10-20 hours across all categories |
| 286 | +**Lines of Code Removable**: ~200-300 lines |
| 287 | +**Risk Level**: Low-Medium (most items are isolated) |
| 288 | +**Impact**: Medium code quality improvement |
| 289 | + |
| 290 | +--- |
| 291 | + |
| 292 | +## Conclusion |
| 293 | + |
| 294 | +These cleanup opportunities represent **technical debt** that can be safely deferred. None block core modernization work. Address them incrementally between major milestones for continuous code quality improvement. |
| 295 | + |
| 296 | +**Next Review**: After Phase 2 Swift 6 migration completes |
0 commit comments