fix(tui): preserve old state on load_file failure (issue #32)
load_file previously cleared file_watcher before attempting to open the new file. If open failed, the watcher was lost while loading_state and file_path still pointed to the old file, breaking change detection. Refactor to build-then-swap pattern: all fallible work runs on local variables first, then self is updated only on success. On failure, the old file's watcher, loading_state and file_path remain intact. Update test to verify all three fields are preserved on failure. Closes #32
This commit is contained in:
+32
-13
@@ -150,13 +150,13 @@ impl App {
|
|||||||
}
|
}
|
||||||
|
|
||||||
pub fn load_file(&mut self, path: &str) -> anyhow::Result<()> {
|
pub fn load_file(&mut self, path: &str) -> anyhow::Result<()> {
|
||||||
// Cancel any existing background indexer by dropping the old state
|
// ── Phase 1: Pure computation, no mutation of self ──────────
|
||||||
self.file_watcher = None;
|
// If any step fails and returns ?, self remains completely untouched,
|
||||||
|
// preserving the old file's watcher, loading_state, and file_path.
|
||||||
let mut pfr = ProgressiveFileReader::open(Path::new(path))
|
let mut pfr = ProgressiveFileReader::open(Path::new(path))
|
||||||
.map_err(|e| anyhow::anyhow!("{e}"))?;
|
.map_err(|e| anyhow::anyhow!("{e}"))?;
|
||||||
|
|
||||||
if pfr.is_sampling() {
|
let new_loading_state = if pfr.is_sampling() {
|
||||||
// Cache miss: spawn background indexer
|
// Cache miss: spawn background indexer
|
||||||
let (cancel_tx, cancel_rx) = crossbeam_channel::bounded(1);
|
let (cancel_tx, cancel_rx) = crossbeam_channel::bounded(1);
|
||||||
let generation = pfr.generation();
|
let generation = pfr.generation();
|
||||||
@@ -175,19 +175,27 @@ impl App {
|
|||||||
).map_err(|e| anyhow::anyhow!("{e}"))?;
|
).map_err(|e| anyhow::anyhow!("{e}"))?;
|
||||||
|
|
||||||
let estimated = pfr.line_count() as u64;
|
let estimated = pfr.line_count() as u64;
|
||||||
self.loading_state = AppLoadingState::Loading {
|
AppLoadingState::Loading {
|
||||||
reader: pfr,
|
reader: pfr,
|
||||||
estimated_lines: estimated,
|
estimated_lines: estimated,
|
||||||
progress_percent: 0.0,
|
progress_percent: 0.0,
|
||||||
};
|
}
|
||||||
} else {
|
} else {
|
||||||
// Cache hit: Ready state
|
// Cache hit: Ready state
|
||||||
self.loading_state = AppLoadingState::Ready { reader: pfr };
|
AppLoadingState::Ready { reader: pfr }
|
||||||
}
|
};
|
||||||
|
|
||||||
self.file_watcher = FileWatcher::watch(Path::new(path)).ok();
|
let new_watcher = FileWatcher::watch(Path::new(path)).ok();
|
||||||
|
|
||||||
|
// ── Phase 2: Commit — swap self to new state ───────────────
|
||||||
|
// SAFETY: Do NOT add any fallible operations (with ?) below this point.
|
||||||
|
// The old file_watcher and loading_state are dropped here, cancelling
|
||||||
|
// any background indexer for the previous file.
|
||||||
|
self.file_watcher = new_watcher;
|
||||||
|
self.loading_state = new_loading_state;
|
||||||
self.file_path = Some(path.to_string());
|
self.file_path = Some(path.to_string());
|
||||||
|
|
||||||
|
// Reset UI state for the new file
|
||||||
self.cursor_line = 0;
|
self.cursor_line = 0;
|
||||||
self.v_offset = 0;
|
self.v_offset = 0;
|
||||||
self.v_sub_offset = 0;
|
self.v_sub_offset = 0;
|
||||||
@@ -2351,17 +2359,28 @@ plain text line
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_file_watcher_stopped_on_nonexistent_file() {
|
fn test_file_watcher_preserved_on_nonexistent_file() {
|
||||||
let path = make_temp_file("data\n");
|
let path = make_temp_file("data\n");
|
||||||
let result = std::panic::catch_unwind(|| {
|
let result = std::panic::catch_unwind(|| {
|
||||||
let mut app = App::new();
|
let mut app = App::new();
|
||||||
app.load_file(path.to_str().unwrap()).unwrap();
|
app.load_file(path.to_str().unwrap()).unwrap();
|
||||||
assert!(app.file_watcher.is_some());
|
assert!(app.file_watcher.is_some());
|
||||||
|
|
||||||
let _ = app.load_file("/tmp/no_such_file_log_viewer_test_xyz_999");
|
let result = app.load_file("/tmp/no_such_file_log_viewer_test_xyz_999");
|
||||||
|
assert!(result.is_err());
|
||||||
|
|
||||||
assert!(
|
assert!(
|
||||||
app.file_watcher.is_none(),
|
app.file_watcher.is_some(),
|
||||||
"file_watcher should be None after failed load_file"
|
"file_watcher should be preserved after failed load_file"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
app.file_path.as_deref(),
|
||||||
|
Some(path.to_str().unwrap()),
|
||||||
|
"file_path should be preserved after failed load_file"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
app.is_loaded(),
|
||||||
|
"loading_state should remain Ready after failed load_file"
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
cleanup(&path);
|
cleanup(&path);
|
||||||
|
|||||||
Reference in New Issue
Block a user