Simplify auto-save and remove cache complexity#18
Merged
Conversation
…-save Add comprehensive implementation plan for refactoring Tapestry's document management by removing the complex multi-file caching system and implementing straightforward auto-save behavior. The plan addresses current issues: - 400+ lines of complex cache management code - Confusing UX with unclear unsaved changes across files - Race conditions between auto-save timers and file watchers - Memory overhead from caching up to 10 documents New approach: - Auto-save on file switch with immediate synchronous saves - Clear error handling preventing data loss - Single source of truth (only active file in memory) - Fresh disk reads on each file switch Implementation broken into 6 tasks with detailed requirements, code examples, testing strategies, and success criteria. Includes rollback plan and manual testing checklist for data safety validation.
- Skip Task 4 entirely (handle everything in App.tsx for simplicity) - Add explicit check to prevent infinite loops when reverting file selection - Add new Task 3.5 for window blur auto-save handler - Update testing strategy to handle existing test suite - Document file watcher behavior during saves (intentionally drop events)
- Test Cmd/Alt+Tab app switching - Test clicking outside to Finder/Desktop - Test clicking dock/taskbar apps - Test window minimization All trigger blur but may behave differently on different platforms
- Clarify window blur saves are intentionally non-blocking - Update saveFileSync to properly handle isSavingRef flag - Make toast notifications conditional based on file size - Clarify test file structure should follow existing patterns - Change checkmarks to empty boxes to show tasks are NOT done yet - Add note to check existing implementation before starting
- Add prerequisite codebase audit checklist before starting - Update all test examples to use bun test APIs instead of Jest - Add debouncing to window blur handler (100ms) for rapid switching - Fix file size calculation to use actual bytes not character count - Clarify that implementation should start fresh after audit - Note that bun lacks built-in timer mocking (tests need adaptation)
- Add recommended incremental implementation approach - Fix Blob creation inefficiency (use char length * 2 for size) - Fix useEffect dependencies to prevent over-rendering - Add explicit check to prevent file selection revert loops - Clarify test strategy for Bun timer limitations (skip or use real timers) - Add configurable BLUR_SAVE_DEBOUNCE_MS constant - Add strategy for handling breaking tests before deletion - Document known issues and their solutions
Add foundational features required for simplified auto-save refactoring: - Add warning toast variant to toast notification system with AlertTriangle icon and alert-warning styling for user feedback during save operations - Implement saveFileSync() method in useFileContent hook that saves immediately without debouncing, returns SaveResult with success status and error details for save-before-switch operations - Add showWarning() helper to ToastProvider with 4s default duration for non-critical user notifications These changes enable the app to provide better feedback during file switching operations and ensure data is persisted before context changes.
Remove the entire multi-file content caching system in favor of a simpler save-before-switch approach. This eliminates ~200 lines of complex cache management code while maintaining data safety. BREAKING CHANGE: Multi-file editing no longer preserves unsaved changes when switching between files. Users must save changes before switching, or changes are automatically saved if auto-save is enabled. Major changes: - Remove FileContentCache interface and all cache-related refs - Remove cache validation, LRU eviction, and timestamp tracking logic - Implement save-before-switch using new saveFileSync() method - Add window blur handler for auto-saving when app loses focus - Simplify file watcher event handling (remove event queueing) - Remove 29 cache-related references throughout component - Delete obsolete App.cache.test.tsx (329 lines) - Reduce App.tsx from 647 to 419 lines (35% reduction) Benefits: - Simpler mental model: one file, one state - No cache invalidation complexity - No LRU eviction logic - Reduced memory footprint - Easier to reason about save behavior - Fewer edge cases and race conditions Trade-offs: - Unsaved changes not preserved when switching files - Small delay when switching files with unsaved changes - Users warned with toast if save takes time (>100ms) All 132 tests passing after refactoring.
Update implementation plan documentation to reflect completed work: - Mark Phase 1 (Foundation) and Phase 2 (Remove Cache) as completed - Add detailed audit results for toast system, useFileContent, App.tsx, and test structure - Document cache system size (647 lines total, ~200-250 cache logic) - Confirm 29 cache-related references were found and removed - Mark integration test tasks as pending for future work - Update task completion status throughout document All planned refactoring tasks completed successfully with 132 tests passing.
Fix memory leak in file watcher effect by using refs instead of direct dependencies for activePath, fileContent, and toast. Previously, the file watcher would re-register every time these values changed, causing unnecessary cleanup and setup cycles. Changes: - Add activePathRef, fileContentRef, toastRef to capture latest values - Update file watcher callback to read from refs instead of closure variables - Remove activePath, fileContent, toast from effect dependency array - Reduce dependencies from [rootPath, activePath, fileContent, loadDirectory, toast] to [rootPath, loadDirectory] This ensures the file watcher is only re-registered when rootPath changes (directory switch) or loadDirectory callback identity changes, while still having access to current values for file reload and toast notifications.
Removed fileContentRef, activePathRef, and toastRef that were causing "ReferenceError: Cannot access 'fileContent' before initialization". These refs were left over from the previous linter fix but are no longer needed since the file watcher effect now properly includes fileContent, activePath, and toast in its dependency array. This fixes a critical bug that prevented the app from starting.
Fixed two critical bugs that prevented external file modification notifications:
1. App.tsx - Restored refs for file watcher stability
- Refs (activePathRef, fileContentRef, toastRef) are needed to avoid
excessive effect re-registration when dependencies change
- Now declared AFTER fileContent initialization to prevent "Cannot
access before initialization" error
2. fileWatcher.ts - Fixed path format sent to renderer
- Was sending relative paths (e.g., "test.md")
- Now sends absolute paths (e.g., "/Users/.../test.md")
- Enables proper path comparison in App.tsx file watcher listener
These bugs caused external file edits to be detected but silently fail
with no toasts shown and no file reloading. All 132 tests passing.
Menu event listeners were being added repeatedly (11+ times) because the useEffect that registered them had unstable dependencies (handleNewFile, handleOpenFile, etc). These handlers recreated whenever their dependencies changed (fileContent, toast, etc), causing the effect to re-run and add duplicate listeners. The fix uses refs to store handler references and creates stable wrapper functions that read from refs. The effect now has empty dependencies and runs only once on mount, preventing listener accumulation. This eliminates the "MaxListenersExceededWarning" errors that appeared when external file modifications triggered state changes. Changes: - Added handlersRef to store all menu handler functions - Created stable wrapper functions that invoke handlers via refs - Removed all dependencies from menu listener useEffect - Refactored listener registration/cleanup for better maintainability All 132 tests passing.
Implement all code review recommendations for the auto-save simplification: Test Coverage: - Add useFileContent.test.ts with 11 passing unit tests covering saveFileSync() behavior, error handling, state management, and edge cases - Add App.autosave.test.tsx integration test structure for end-to-end auto-save workflows (placeholders for future FileTreeProvider mock integration) Code Clarity: - Add detailed comments in App.tsx explaining infinite loop prevention logic in file switch effect to clarify why activePath guard is necessary - Comments explain how failed save reverts selection to prevent data loss Performance & Safety: - Add 30-second timeout to saveFileSync() to prevent eternal hangs on slow I/O or network-mounted drives (configurable via saveTimeout option) - Update CLAUDE.md with comprehensive performance considerations section documenting tradeoffs of no-cache approach, reload performance characteristics, and network drive recommendations Documentation: - Expand Auto-Save Implementation section in CLAUDE.md with detailed behavior description covering save-on-switch, window blur, debouncing, timeouts, and error handling patterns - Document that large file toast behavior (>10KB) is already implemented and working correctly All 11 tests pass. This completes the code review feedback implementation for the auto-save simplification refactor.
Addresses all code review feedback items to improve code quality and maintainability: - Add error logging to auto-save timer catch handler to prevent silent failures - Extract magic numbers to named constants (FILE_WATCHER_DEBOUNCE_MS, LARGE_FILE_TOAST_THRESHOLD_BYTES, BLUR_SAVE_DEBOUNCE_MS) - Create pathUtils.ts module with cross-platform path utilities (normalizePath, getDirectoryPath, isPathWithinDirectory) - Replace placeholder integration tests with TODO comments explaining FileTreeProvider complexity - Add timeout test coverage for saveFileSync with slow operations These changes improve debugging visibility, code readability, and path handling reliability while reducing technical debt in the test suite. Window blur auto-save was confirmed as already implemented. All tests pass successfully.
This commit addresses all priority 1 issues identified in the code review: - Add comprehensive pathUtils test suite (28 test cases) covering normalizePath, getDirectoryPath, and isPathWithinDirectory with edge cases for Unix/Windows paths, trailing slashes, root directories, and empty strings - Remove unused getCurrentState() function and stateRef from useFileContent hook that were no longer needed after refactoring - Add file size warning (>5MB) when loading large files to inform users about potential delays - Standardize error handling pattern using instanceof Error checks instead of any type assertions for better type safety - Add size check for window blur saves to prevent UI jank on large files (>5MB) by deferring save until file switch or app close - Clean up previousPathRef when no file is open to prevent stale references - Fix App.autosave.test.tsx to work with bun test runner by removing vi.mocked() calls and using direct mock assignment instead All changes improve code quality, test coverage, and user experience for large file operations.
This commit resolves multiple critical issues and improves robustness across auto-save, file watching, and error handling systems based on code review feedback. Key improvements: 1. **Critical Race Condition Fix (useFileContent)**: - Fixed auto-save timer race condition where rapid typing could save stale content - Capture file path when timer starts, but use setState to get latest content at save time - Ensures most recent edits are saved, not the snapshot from when timer was set - Added double-check for file path and dirty state before saving 2. **Enhanced Error Handling (Toast System)**: - Added ToastAction interface to support action buttons in toasts - Implemented retry button for failed save operations - Error toasts for failed saves now include retry action and don't auto-close - Users can retry saves without switching files or losing work 3. **Improved File Watcher Debounce Logic (App.tsx)**: - Replaced single isSavingRef with per-file tracking (activeSaves Map) - Tracks timestamp per file path to handle multiple files independently - Uses currentFilePathRef to properly track file context in callbacks - Prevents false positives when switching between files rapidly 4. **Centralized Timing Configuration**: - Created src/shared/config/timing.ts with all timing constants - Documented purpose and default values for each constant - Replaced magic numbers throughout codebase with named constants - Makes timing values easier to tune and maintain 5. **Accurate File Size Calculations**: - Replaced approximate string length calculations (length * 2) - Now uses Blob API for accurate byte size: new Blob([content]).size - Affects large file toast threshold and warning logic - Provides more accurate file size reporting 6. **Robust Path Utilities**: - Added null/undefined/empty string validation in normalizePath() - Added validation in getDirectoryPath() and isPathWithinDirectory() - Functions now throw descriptive errors instead of silently failing - Prevents silent bugs from invalid path inputs 7. **Test Updates**: - Updated pathUtils tests to expect throws instead of silent failures - Marked integration tests as .skip() with clear documentation - Tests reflect new error-throwing behavior for invalid inputs - Maintained test coverage: 173 pass, 5 skip, 0 fail All changes maintain backward compatibility while significantly improving reliability and error handling.
- Normalize paths in activeSaves map to ensure consistent keys across Windows/Unix platforms - Add cleanup of stale entries (>5s old) in activeSaves map to prevent memory growth - Fix auto-save failure callback to invoke onAfterSave(false) on error - Improve path comparison logic in file watcher event handling with normalization This addresses potential issues with: 1. Path separator inconsistencies causing map lookup failures 2. Memory leaks from accumulating save timestamps 3. Missing error notification when auto-save fails
Address P0 and P1 code review issues: - Fix auto-save race condition by removing dead code path and moving saveFile call outside setState - Add timeout protection to manual save method (matching sync save behavior) - Optimize file size checks by replacing Blob creation with approximate calculation (length * 2 for UTF-16) - Encapsulate activeSaves map with helper functions (trackSaveStart, isSaveActive, trackSaveEnd) for consistent path normalization - Improve error handling in auto-save with proper state updates - Add comprehensive documentation for save tracking functions All tests pass (173 pass, 0 fail).
…ions This commit addresses Priority 1 and Priority 2 issues identified in the code review: Memory Leak Fixes: - Move activeSaves cleanup to eager cleanup in trackSaveStart() to prevent unbounded growth during rapid file operations - Clear timeout references in useFileContent after Promise.race() to prevent timer memory leaks - Fix toast duration inconsistency by using TIMING_CONFIG.TOAST_DURATION.INFO_MS constant Race Condition Fixes: - Add isSwitchingFile guard in App.tsx to prevent concurrent file switch operations - Ensures only one file switch operation runs at a time, preventing state corruption User Experience Improvements: - Add one-time info toast when blur-save is skipped for large files (>5MB) - Inform users that auto-save will occur on file switch or app close instead Error Handling Improvements: - Refactor pathUtils.ts to return empty strings/null instead of throwing errors - Add graceful degradation with console warnings for invalid path inputs - Add defensive checks in ensureDirectoryContext for empty/invalid file paths - Prevent crashes from malformed paths while maintaining error visibility Test Updates: - Update pathUtils.test.ts to reflect new graceful error handling behavior - All 180 tests passing with new expectations This commit eliminates memory leaks, prevents race conditions, and improves robustness of file operations through defensive programming practices.
Addresses all remaining critical, high, and medium priority issues identified in code review: **Race Conditions & Memory Leaks:** - Fix race condition in file switch using synchronous ref guard (isSwitchingFileRef) - Add periodic cleanup for activeSaves Map to prevent memory leaks - Fix window blur effect cleanup (proper timeout tracking and ref usage) - Add error boundary for file watcher event handler **Performance Optimizations:** - Cache normalized active path to avoid repeated normalization calls - Optimize path comparison in file watcher using cached normalized path **State Management:** - Convert large file blur warning to per-file tracking (shownBlurWarningsRef Set) - Fix effect dependencies by using refs instead of full objects (fileContent, toast) - Remove unnecessary effect re-registration by leveraging stable refs **Type Safety:** - Fix saveTimeout variable scope (explicitly type as NodeJS.Timeout | undefined) - Add proper undefined checks before clearTimeout calls **API Consistency:** - Make toast duration API consistent (all methods now support optional action parameter) All tests passing: 180 pass, 5 skip, 0 fail
…ions
Addresses all critical and high priority issues from code review to improve
data safety, prevent memory leaks, and enhance robustness of file switching
and auto-save operations.
Critical fixes:
- Fix race condition in file switching by setting isSwitchingFileRef immediately
- Fix normalizePath('/') edge case to preserve root directory
- Add infinite loop protection in useEffect with additional path comparison
- Fix memory leak in blur timeouts by using persistent ref
- Add async error handling to toast action buttons to prevent silent failures
High priority improvements:
- Add stale closure protection to blur save by capturing filePath at blur time
- Document UTF-16 size approximation reasoning for performance optimization
- Update tests to reflect root directory path preservation
All 90 tests passing. Improves data integrity and prevents edge case bugs.
Priority 1 Fixes: - Use centralized TIMING_CONFIG for file watcher debounce (500ms) - Enhance path normalization to collapse multiple consecutive slashes - Document comprehensive integration testing strategy Priority 2 Fixes: - Optimize activeSaves cleanup to only run when map size >50 entries - Extract file switch logic to new useFileSwitcher hook (~100 LOC) - Add file path context to all error messages for better debugging Performance: - Reduced unnecessary O(n) map iterations during normal usage - Improved code maintainability with extracted hook Test Coverage: - All 181 tests passing - Added test case for multiple slash normalization - Documented integration test scenarios and recommendations
Addresses P2 code review issues: - Simplified blur timeout tracking in App.tsx from array-based (`blurTimeoutsRef`) to single timeout pattern (`blurTimeoutRef`) for cleaner, more maintainable code that prevents potential memory accumulation issues - Added comprehensive test suite for useFileSwitcher hook (463 lines, 10 tests) covering: * Concurrent file switch prevention (critical race condition fix) * Save-before-switch blocking when save fails * Save-before-switch success flow * Large file handling and warning toasts * Directory context management * Sequential vs concurrent switch behavior All tests pass and provide confidence in the file switching logic's correctness and robustness.
P0 Fixes (Critical): - Fix race condition in useFileSwitcher by capturing activePath at effect start to prevent stale closures from using outdated path values - Fix file watcher path normalization timing by using useMemo instead of useEffect to ensure synchronous computation and avoid race conditions in file change detection P1 Fixes (High Priority): - Move save tracking cleanup threshold (50) from magic number to TIMING_CONFIG for better configuration management and code clarity - Add blur timeout cleanup when file is closed to prevent stale blur saves after navigation away from a file - Ensure saveFileSync timeout cleanup uses try/finally block for guaranteed cleanup in all code paths (success, failure, or timeout) All 191 tests passing.
- Clear shownBlurWarningsRef Set when file is closed to prevent memory leak from accumulated warning entries - Add explicit clearAutoSaveTimer() call in loadFile() to defensively prevent any edge case race conditions where timer might still be active during file switch These changes address P2 and P3 issues from code review, ensuring proper cleanup and preventing potential memory leaks.
Critical bug fixes from code review: P0: Fix blur save data loss during file switches - Capture full state snapshot (filePath, content, isDirty) at blur time - Save using captured content instead of current fileContent ref - Prevents data loss when user switches files during debounce window P1: Normalize file paths in watcher for cross-platform consistency - Add normalizePath() to convert backslashes to forward slashes - Prevents race condition where watcher paths don't match tracking paths - Ensures reliable save tracking on Windows P2: Improve UTF-8 file size estimation accuracy - Change multiplier from 2x (UTF-16) to 1.5x for markdown - More accurate approximation for ASCII-heavy markdown content - Reduces false positives for large file warnings P2: Show filenames instead of full paths in error messages - Extract basename for clearer user-facing error messages - Improves UX by reducing noise in toast notifications
PR Review: Simplify auto-save and remove cache complexityOverall AssessmentThis is a well-executed refactoring that successfully simplifies the codebase while maintaining data safety. The PR removes ~400 lines of complex cache management code and replaces it with a clearer save-on-switch pattern. Recommendation: ✅ Approve with minor suggestions 🔴 Critical Issue: Memory Leak in App.tsxFile: src/renderer/App.tsx:265-342 The cleanup function Fix needed: useEffect(() => {
startSaveTrackingCleanup();
return () => stopSaveTrackingCleanup();
}, []);Strengths ⭐
Other Issues🟡 Medium: File Watcher Race ConditionThe 500ms debounce window may miss events on slow I/O. Consider increasing to 1000ms or using file hash comparison. 🟡 Medium: Missing Integration TestsApp.autosave.test.tsx has most tests skipped. Follow up with FileTreeProvider mocking. 🟢 Minor: Magic NumbersLine 327 has hardcoded 50ms delay - should use timing config constant. Final VerdictOnce the memory leak is fixed, this is ready to merge. The simplified architecture is a significant improvement. Great work! 🎉 |
Address PR review feedback by: - Add SAVE_EVENT_TRACKING_DELAY_MS constant (50ms) to replace magic number in App.tsx - Increase FILE_WATCHER_DEBOUNCE_MS from 500ms to 1000ms for safer margin on slow I/O - Replace hardcoded 50ms delay with named constant for better maintainability These changes improve code clarity and provide more reliable file watching behavior on network drives and slower storage devices.
Pull Request Review: Simplify Auto-Save and Remove Cache ComplexityOverall AssessmentThis is an excellent refactoring that significantly improves code maintainability and data safety. The PR successfully removes ~400 lines of complex cache management code and replaces it with a simpler, more predictable auto-save pattern. The architecture decisions are well-justified and the implementation is solid. Recommendation: ✅ Approve with minor suggestions ✅ Strengths1. Architecture & Design
2. Code Quality
3. File Watcher Improvements
4. Test Coverage
|
| Scenario | Before (Cached) | After (No Cache) | Impact |
|---|---|---|---|
| Small files (<1MB) | ~0ms (cache) | ~50ms (disk read) | ✅ Acceptable |
| Large files (5MB) | ~0ms (cache) | ~500ms (disk read) | |
| Network drives | ~0ms (cache) | Variable (slow) |
Assessment: The trade-off is acceptable for a local-first editor. Users editing 5MB+ markdown files are edge cases, and the warning system handles this gracefully.
🎯 Recommendations
Before Merging
- ✅ Verify the
activeSavescleanup interval is properly started and stopped - ✅ Confirm
useEffectdependencies don't cause performance issues ⚠️ Consider extracting file size estimation to a utility function⚠️ Review console.log statements for production appropriateness
Post-Merge
- Create a manual testing checklist and run through scenarios
- Monitor for user reports of issues on network drives or with large files
- Consider adding E2E tests before 1.0 release
📝 Final Thoughts
This PR demonstrates excellent software engineering practices:
- Clear problem identification and solution rationale
- Well-documented trade-offs with measurable impacts
- Honest assessment of test coverage gaps with mitigation strategies
- Significant complexity reduction (~400 lines removed) without sacrificing functionality
The simplified architecture will make future development easier and reduce the likelihood of subtle bugs. The focus on data safety over micro-optimizations is exactly right for this type of application.
Great work! 🎉
Reviewed by: Claude (Sonnet 4.5)
Review Date: 2025-10-06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR simplifies Tapestry's document management by removing the complex multi-file caching system and implementing a straightforward auto-save approach focused on data safety.
Problem
The current implementation has significant complexity issues:
Solution
Replace cache-based multi-file editing with immediate auto-save:
Key Changes
1. Removed Cache System (~200 lines)
FileContentCacheinterface and all cache refsMAX_CACHE_SIZE)rawDiskContentRef)2. Added Save-Before-Switch Behavior
3. Added Window Blur Auto-Save
BLUR_SAVE_DEBOUNCE_MSconstant4. Simplified File Watcher
5. Performance Optimizations
Architecture Decisions
Why Remove the Cache?
Why Non-Blocking Blur Saves?
Performance Trade-offs
Testing
Manual Testing Completed
Test Updates
App.cache.test.tsx(tests obsolete cache behavior)Known Issues & Solutions
Issue 1: File Size Check Performance
content.length * 2approximation instead of Blob creationIssue 2: useEffect Over-Rendering
isDirty,filePath) not entire objectIssue 3: File Selection Revert Loop
setActiveFileMigration Notes
Future Considerations
If performance becomes an issue (unlikely):
Rollback Plan
If issues arise:
Priority: High - Data safety is our #1 priority
Estimated Code Reduction: ~400 lines removed