Track file modification times to ignore metadata-only changes#251
Conversation
Switch known_files from HashSet to HashMap<PathBuf, SystemTime> to track modification times. Only emit file change events when the modification time changes, ignoring metadata-only updates such as access time changes.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a critical bug in the file watcher where read operations on files were incorrectly triggering modified events, causing infinite reload loops. The solution involves tracking file modification times (mtime) to distinguish between actual content changes and metadata-only updates.
- Refactored
known_filesfrom aHashSet<PathBuf>to aHashMap<PathBuf, SystemTime>to store file modification timestamps - Modified event determination logic to compare current
mtimeagainst stored timestamps before emitting events - Updated file and directory watching setup to record initial modification times
Comments suppressed due to low confidence (1)
src-tauri/src/file_watcher.rs:143
- [nitpick] The variable name
known_files_mapis redundant since the parameter is already calledknown_files. Consider using a simpler name likeknown_filesorfilesfor the local variable.
let mut known_files_map = known_files.lock().unwrap();
| let _ = app_handle.emit("file-changed", &change_event); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This closing brace appears to be orphaned and would cause a compilation error. The function structure seems incorrect - there should be a closing brace for the function at line 103, not an additional one here.
| let current_mtime = metadata | ||
| .modified() | ||
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); |
There was a problem hiding this comment.
[nitpick] Using UNIX_EPOCH as a fallback for modification time could be misleading. Consider using a more recent timestamp or handling the error case explicitly to avoid confusion about when a file was actually modified.
| let current_mtime = metadata | |
| .modified() | |
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); | |
| let current_mtime = match metadata.modified() { | |
| Ok(mtime) => mtime, | |
| Err(_) => return Some(FileChangeType::Modified), | |
| }; |
| let mtime = metadata | ||
| .modified() | ||
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); | ||
| self | ||
| .known_files | ||
| .lock() | ||
| .unwrap() | ||
| .insert(path_buf.clone(), mtime); |
There was a problem hiding this comment.
[nitpick] Using UNIX_EPOCH as a fallback for modification time could be misleading. Consider using a more recent timestamp or handling the error case explicitly to avoid confusion about when a file was actually modified.
| let mtime = metadata | |
| .modified() | |
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); | |
| self | |
| .known_files | |
| .lock() | |
| .unwrap() | |
| .insert(path_buf.clone(), mtime); | |
| match metadata.modified() { | |
| Ok(mtime) => { | |
| self | |
| .known_files | |
| .lock() | |
| .unwrap() | |
| .insert(path_buf.clone(), mtime); | |
| } | |
| Err(e) => { | |
| eprintln!("Failed to get modification time for {:?}: {}", path_buf, e); | |
| // Optionally, you could return an error here instead of just logging. | |
| } | |
| } |
| let mtime = metadata | ||
| .modified() | ||
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); | ||
| known_files.insert(path, mtime); |
There was a problem hiding this comment.
[nitpick] Using UNIX_EPOCH as a fallback for modification time could be misleading. Consider using a more recent timestamp or handling the error case explicitly to avoid confusion about when a file was actually modified.
| let mtime = metadata | |
| .modified() | |
| .unwrap_or(std::time::SystemTime::UNIX_EPOCH); | |
| known_files.insert(path, mtime); | |
| if let Ok(mtime) = metadata.modified() { | |
| known_files.insert(path, mtime); | |
| } else { | |
| // Optionally log: could not get modification time for path | |
| } |
- Replace Created/Modified with Opened/Reloaded event types - Emit Opened event when a file is watched - Improve handling of file modification times and error cases - Add logging for metadata access issues
Description
This pull request addresses a critical bug in the file watcher that caused it to incorrectly trigger
modifiedevents when a file was only read, not changed. Read operations on some operating systems update a file's access time (atime), which the watcher was misinterpreting as a content modification. This led to infinite reload loops in the application.The solution is to make the watcher track each file's modification time (
mtime). An event is now only considered a "modification" if the file'smtimehas actually changed, effectively filtering out noise from metadata-only updates.known_filesfromArc<Mutex<HashSet<PathBuf>>>toArc<Mutex<HashMap<PathBuf, SystemTime>>>to store file modification timestamps.determine_event_typelogic to compare the file's currentmtimeagainst the stored timestamp before firing an event.setup_path_watchingandsetup_directory_watchingto record the initialmtimeof a file when it is first added to the watcher.Screenshots/Videos
N/A - This change is purely in the backend file-watching logic and has no visual component. The result is the absence of erroneous logging and reloading.
Related Issues
Fixes #197