Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions docs/reviews/ai-review-result_bug-file-vanish_20260304.md
Original file line number Diff line number Diff line change
@@ -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();
```
23 changes: 20 additions & 3 deletions src/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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());

Expand All @@ -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());
}
Expand Down
12 changes: 8 additions & 4 deletions tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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/.
Expand All @@ -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()
Expand All @@ -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")
Expand All @@ -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/.
Expand Down