diff --git a/docs/reviews/ai-review-result_bug-file-vanish_20260304.md b/docs/reviews/ai-review-result_bug-file-vanish_20260304.md new file mode 100644 index 0000000..76603cd --- /dev/null +++ b/docs/reviews/ai-review-result_bug-file-vanish_20260304.md @@ -0,0 +1,36 @@ +# The result of review +Fix bug that the file content is cleared when updating status (PR #8) + +## ❌CRITICAL +Nothing. + +## 🔴HIGH +Nothing. + +## 🟡MEDIUM +Nothing. + +## 🔵LOW + +### Comment1: Silent error suppression in `update_status` +`src/task.rs` - `update_status` method + +`let _ = self.update_frontmatter_preserving_body();` silently discards the `io::Result`. If writing fails (e.g. disk full), the file will have been moved to the new directory but its frontmatter (including `updated_at`) will remain stale with no indication to the caller. This is consistent with the existing pattern in the same method, but worth noting as a future improvement. + +### Comment2: Frontmatter delimiter ambiguity in body extraction +`src/task.rs` - `update_frontmatter_preserving_body` method + +The body is extracted by searching for the first occurrence of `"\n---\n"` in the content after the opening delimiter. If a user writes `\n---\n` inside the markdown body (e.g. a YAML code block or a horizontal rule sequence), the extraction will silently truncate the body at that point. This is an inherent limitation of simple string-based frontmatter parsing, and unlikely to matter in practice given how the app is used. + +### Comment3: Verbose test setup line +`src/task.rs` - `update_status_moves_file_between_directories` test +`tests/integration_test.rs` - `forward_status_moves_md_file_to_next_directory` test + +```rust +// Current: hard to read at a glance +fs::write(task.file_path(), format!("{}{}", fs::read_to_string(task.file_path()).unwrap(), body)).unwrap(); + +// Clearer: +let existing = fs::read_to_string(task.file_path()).unwrap(); +fs::write(task.file_path(), format!("{}{}", existing, body)).unwrap(); +``` diff --git a/src/task.rs b/src/task.rs index 37660e4..bc2d91a 100644 --- a/src/task.rs +++ b/src/task.rs @@ -160,7 +160,19 @@ impl Task { self.updated_at = Utc::now(); let _ = fs::create_dir_all(Self::status_dir(&self.status)); let _ = fs::rename(old_path, self.file_path()); - let _ = self.save(); + let _ = self.update_frontmatter_preserving_body(); + } + + /// Updates only the YAML frontmatter of the task file while preserving the markdown body. + fn update_frontmatter_preserving_body(&self) -> io::Result<()> { + let path = self.file_path(); + let existing = fs::read_to_string(&path)?; + let yaml = serde_yaml::to_string(&self.frontmatter()).map_err(io::Error::other)?; + let body = existing + .strip_prefix("---\n") + .and_then(|s| s.find("\n---\n").map(|pos| &s[pos + 5..])) + .unwrap_or(""); + fs::write(&path, format!("---\n{}---\n{}", yaml, body)) } /// Sorts tasks by status group (TODO, DOING, DONE) and by `created_at` within each group. @@ -272,9 +284,12 @@ mod tests { #[test] fn update_status_moves_file_between_directories() { - // GIVEN: a saved task with TODO status + // GIVEN: a saved task with TODO status and a markdown body appended to the file let mut task = Task::new("status move test".to_string()); task.save().unwrap(); + let body = "## Notes\n\nsome content here\n"; + let existing = fs::read_to_string(task.file_path()).unwrap(); + fs::write(task.file_path(), format!("{}{}", existing, body)).unwrap(); let old_path = task.file_path(); assert!(old_path.exists()); @@ -283,11 +298,13 @@ mod tests { let before_update = task.updated_at; task.update_status(TaskStatus::Doing); - // THEN: the file is moved to doing/ directory and updated_at is refreshed + // THEN: the file is moved to doing/ directory, updated_at is refreshed, and body content is preserved assert!(!old_path.exists()); assert!(task.file_path().exists()); assert!(task.file_path().to_str().unwrap().contains("/doing/")); assert!(task.updated_at > before_update); + let content = fs::read_to_string(task.file_path()).unwrap(); + assert!(content.contains(body)); let _ = fs::remove_file(task.file_path()); } diff --git a/tests/integration_test.rs b/tests/integration_test.rs index ec7bd8b..d82ccb1 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -3,7 +3,7 @@ use rem_cli::app::App; use rem_cli::task::TaskStatus; use std::fs; -/// Scenario 1: Adding a task via rem creates a new md file. +/// Scenario: Adding a task via rem creates a new md file. /// /// Simulates pressing 'a', typing a task name, and pressing Enter. /// Verifies that a corresponding md file is created in the todo/ directory. @@ -34,7 +34,7 @@ fn adding_task_creates_md_file() { let _ = fs::remove_file(file_path); } -/// Scenario 2: Pressing 'n' moves the md file to the next status directory. +/// Scenario: Pressing 'n' moves the md file to the next status directory. /// /// Creates a task, then presses 'n' to forward status from TODO to DOING. /// Verifies the file is moved from todo/ to doing/. @@ -52,6 +52,9 @@ fn forward_status_moves_md_file_to_next_directory() { .expect("task should exist"); let todo_path = task.file_path(); assert!(todo_path.exists()); + let body = "## Notes\n\nsome content here\n"; + let existing = fs::read_to_string(&todo_path).unwrap(); + fs::write(&todo_path, format!("{}{}", existing, body)).unwrap(); // WHEN: navigate to the task and press 'n' to forward status (TODO -> DOING) let task_index = app.tasks.iter() @@ -60,7 +63,7 @@ fn forward_status_moves_md_file_to_next_directory() { app.selected_index = Some(task_index); app.handle_key_event(KeyCode::Char('n')); - // THEN: the file is moved from todo/ to doing/ + // THEN: the file is moved from todo/ to doing/, and body content is preserved assert!(!todo_path.exists(), "file should no longer exist in todo/"); let task = app.tasks.iter() .find(|t| t.name == "forward status test") @@ -69,12 +72,13 @@ fn forward_status_moves_md_file_to_next_directory() { let doing_path = task.file_path(); assert!(doing_path.exists(), "file should exist in doing/"); assert!(doing_path.to_str().unwrap().contains("/doing/")); + assert!(fs::read_to_string(&doing_path).unwrap().contains(body), "file body should be preserved after status update"); // Cleanup let _ = fs::remove_file(doing_path); } -/// Scenario 2 (reverse): Pressing 'N' moves the md file to the previous status directory. +/// Scenario: Pressing 'N' moves the md file to the previous status directory. /// /// Creates a task in DOING status, then presses 'N' to backward status from DOING to TODO. /// Verifies the file is moved from doing/ to todo/.