fix: /clear now resets the Todos sidebar panel#1311
fix: /clear now resets the Todos sidebar panel#1311Giggitycountless wants to merge 2 commits intoHmbown:mainfrom
Conversation
The /clear command was not reliably clearing the Todos sidebar because clear_todos() used a single try_lock() attempt. When the engine held the mutex during tool execution, the lock acquisition failed and todos persisted across /clear commands. Replace the non-blocking try_lock with a retry loop (up to 100 attempts, 1ms apart) so /clear waits briefly for the mutex and always clears todos. Also add a regression test clear_todos_resets_todos_list that seeds todos and asserts they are empty after /clear. Fixes Hmbown#1258
There was a problem hiding this comment.
Code Review
This pull request implements a retry mechanism in the clear_todos function to ensure mutex locks are acquired for resetting the todo list and plan state, and adds a corresponding unit test. Review feedback suggests refactoring the duplicated retry logic into a helper method to improve maintainability and mitigate potential UI performance issues caused by sleeping on the main thread.
| let todos_cleared = { | ||
| let mut cleared = false; | ||
| for _ in 0..100 { | ||
| if let Ok(mut todos) = self.todos.try_lock() { | ||
| todos.clear(); | ||
| cleared = true; | ||
| break; | ||
| } | ||
| std::thread::sleep(std::time::Duration::from_millis(1)); | ||
| } | ||
| cleared | ||
| }; | ||
| // Also clear the plan state — /clear means a full reset. | ||
| if let Ok(mut plan) = self.plan_state.try_lock() { | ||
| *plan = crate::tools::plan::PlanState::default(); | ||
| for _ in 0..100 { | ||
| if let Ok(mut plan) = self.plan_state.try_lock() { | ||
| *plan = crate::tools::plan::PlanState::default(); | ||
| break; | ||
| } | ||
| std::thread::sleep(std::time::Duration::from_millis(1)); | ||
| } | ||
| todos_cleared | ||
| } |
There was a problem hiding this comment.
The retry logic is duplicated for self.todos and self.plan_state. Additionally, using std::thread::sleep within sequential loops on the UI thread can block the interface for up to 200ms (or more, depending on OS scheduler precision for 1ms sleeps), leading to noticeable stuttering.
Consider refactoring this into a helper method to reduce duplication and ensure a consistent timeout across both operations.
let todos_cleared = self.try_lock_with_retry(&self.todos, |todos| todos.clear());
self.try_lock_with_retry(&self.plan_state, |plan| {
*plan = crate::tools::plan::PlanState::default();
});
todos_cleared
}
fn try_lock_with_retry<T, F>(&self, mutex: &tokio::sync::Mutex<T>, f: F) -> bool
where
F: FnOnce(&mut T),
{
for _ in 0..100 {
if let Ok(mut guard) = mutex.try_lock() {
f(&mut guard);
return true;
}
std::thread::sleep(std::time::Duration::from_millis(1));
}
false
}Extract the duplicated try_lock retry loop into a generic helper method retry_lock<T>() that retries up to N times with 1ms pauses. This improves readability and makes the retry pattern reusable. Addresses review feedback from gemini-code-assist.
Problem
Running
/cleardoes not reliably clear the Todos sidebar. When the engine holds the todos mutex during tool execution,clear_todos()fails because it uses a singletry_lock()attempt. The todos persist across/clearcommands until the app is restarted.Root Cause
clear_todos()incrates/tui/src/tui/app.rsusestry_lock()which is non-blocking. If the mutex is held by the engine, the method returnsfalseand shows a busy message instead of clearing the todos.Fix
Replace the single
try_lock()attempt with a retry loop (up to 100 attempts, 1ms apart). This gives the engine up to 100ms to release the mutex while keeping the UI responsive.Testing
Added a regression test
clear_todos_resets_todos_listthat:clear_todos()Fixes #1258