fix(tui): cache Work panel summary to survive transient mutex misses && fix compile error#2616
fix(tui): cache Work panel summary to survive transient mutex misses && fix compile error#2616idling11 wants to merge 1 commit into
Conversation
|
Thanks @idling11 for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces caching for the sidebar work summary in the TUI application to prevent the panel from being wiped when shared mutexes are busy. It adds a 'cached_work_summary' field to the 'App' struct, updates 'sidebar_work_summary' to fall back to this cache, and includes comprehensive unit tests. The feedback highlights two key improvements: first, changing the visibility of 'cached_work_summary' to 'pub(crate)' to avoid a potential compilation error (E0446) due to exposing a private type in a public interface; second, refactoring 'sidebar_work_summary' to consolidate the copying of live fields and eliminate redundant code across different execution paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub sidebar_hover_tooltip: Option<String>, | ||
| /// Cached Work panel summary — used as fallback when `try_lock()` on | ||
| /// the shared todo/plan mutexes fails during a frame render. | ||
| pub cached_work_summary: Option<SidebarWorkSummary>, |
There was a problem hiding this comment.
Since SidebarWorkSummary is defined as pub(crate) in sidebar.rs, declaring cached_work_summary as pub in the public App struct can lead to a compilation error (E0446: private type in public interface) if App is exported publicly from the tui crate. Changing the field visibility to pub(crate) resolves this safely, as it is only accessed within the tui crate.
| pub cached_work_summary: Option<SidebarWorkSummary>, | |
| pub(crate) cached_work_summary: Option<SidebarWorkSummary>, |
| fn sidebar_work_summary(app: &mut App) -> SidebarWorkSummary { | ||
| // Try to build a fresh summary from the shared mutexes. When either | ||
| // lock is busy (engine is updating state in another tokio task), | ||
| // fall back to the last cached snapshot instead of wiping the panel. | ||
| let fresh = (|| { | ||
| let todos = app.todos.try_lock().ok()?; | ||
| let plan = app.plan_state.try_lock().ok()?; | ||
|
|
||
| let todos_snapshot = todos.snapshot(); | ||
| let checklist_completion_pct = todos_snapshot.completion_pct; | ||
| let checklist_items: Vec<SidebarWorkChecklistItem> = todos_snapshot | ||
| .items | ||
| .into_iter() | ||
| .map(|item| SidebarWorkChecklistItem { | ||
| id: item.id, | ||
| content: item.content, | ||
| status: item.status, | ||
| }) | ||
| .collect(); | ||
|
|
||
| match app.plan_state.try_lock() { | ||
| Ok(plan) => { | ||
| if !plan.is_empty() { | ||
| summary.strategy_explanation = plan.explanation().map(str::to_string); | ||
| summary.strategy_steps = plan | ||
| .steps() | ||
| let (strategy_explanation, strategy_steps) = if plan.is_empty() { | ||
| (None, Vec::new()) | ||
| } else { | ||
| ( | ||
| plan.explanation().map(str::to_string), | ||
| plan.steps() | ||
| .iter() | ||
| .map(|step| SidebarWorkStrategyStep { | ||
| text: step.text.clone(), | ||
| status: step.status.clone(), | ||
| elapsed: step.elapsed_str(), | ||
| }) | ||
| .collect(); | ||
| } | ||
| } | ||
| Err(_) => { | ||
| summary.state_updating = true; | ||
| } | ||
| .collect(), | ||
| ) | ||
| }; | ||
|
|
||
| Some(SidebarWorkSummary { | ||
| goal_objective: app.hunt.quarry.clone(), | ||
| goal_token_budget: app.hunt.token_budget, | ||
| goal_completed: app.hunt.verdict == HuntVerdict::Hunted, | ||
| goal_started_at: app.hunt.started_at, | ||
| tokens_used: app.session.total_conversation_tokens, | ||
| checklist_completion_pct, | ||
| checklist_items, | ||
| strategy_explanation, | ||
| strategy_steps, | ||
| state_updating: false, | ||
| }) | ||
| })(); | ||
|
|
||
| if let Some(summary) = fresh { | ||
| app.cached_work_summary = Some(summary.clone()); | ||
| return summary; | ||
| } | ||
|
|
||
| // Fall back to cached snapshot. Only emit "updating" when there is | ||
| // genuinely nothing to show (first render before any todos exist). | ||
| if let Some(ref cached) = app.cached_work_summary { | ||
| let mut summary = cached.clone(); | ||
| // Keep hunt/goal fields live even when locks are busy. | ||
| summary.goal_objective = app.hunt.quarry.clone(); | ||
| summary.goal_token_budget = app.hunt.token_budget; | ||
| summary.goal_completed = app.hunt.verdict == HuntVerdict::Hunted; | ||
| summary.goal_started_at = app.hunt.started_at; | ||
| summary.tokens_used = app.session.total_conversation_tokens; | ||
| return summary; | ||
| } | ||
|
|
||
| summary | ||
| SidebarWorkSummary { | ||
| goal_objective: app.hunt.quarry.clone(), | ||
| goal_token_budget: app.hunt.token_budget, | ||
| goal_completed: app.hunt.verdict == HuntVerdict::Hunted, | ||
| goal_started_at: app.hunt.started_at, | ||
| tokens_used: app.session.total_conversation_tokens, | ||
| state_updating: true, | ||
| ..SidebarWorkSummary::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation duplicates the copying of live app.hunt and app.session fields across three different code paths (the fresh closure, the cache fallback, and the default fallback). We can simplify this significantly by constructing/retrieving the base summary first, and then applying the live fields once at the end of the function. This improves maintainability and eliminates redundant code.
fn sidebar_work_summary(app: &mut App) -> SidebarWorkSummary {
// Try to build a fresh summary from the shared mutexes. When either
// lock is busy (engine is updating state in another tokio task),
// fall back to the last cached snapshot instead of wiping the panel.
let fresh = (|| {
let todos = app.todos.try_lock().ok()?;
let plan = app.plan_state.try_lock().ok()?;
let todos_snapshot = todos.snapshot();
let checklist_completion_pct = todos_snapshot.completion_pct;
let checklist_items: Vec<SidebarWorkChecklistItem> = todos_snapshot
.items
.into_iter
.map(|item| SidebarWorkChecklistItem {
id: item.id,
content: item.content,
status: item.status,
})
.collect();
let (strategy_explanation, strategy_steps) = if plan.is_empty() {
(None, Vec::new())
} else {
(
plan.explanation().map(str::to_string),
plan.steps()
.iter()
.map(|step| SidebarWorkStrategyStep {
text: step.text.clone(),
status: step.status.clone(),
elapsed: step.elapsed_str(),
})
.collect(),
)
};
Some(SidebarWorkSummary {
checklist_completion_pct,
checklist_items,
strategy_explanation,
strategy_steps,
state_updating: false,
..SidebarWorkSummary::default()
})
})();
let mut summary = if let Some(summary) = fresh {
app.cached_work_summary = Some(summary.clone());
summary
} else if let Some(ref cached) = app.cached_work_summary {
cached.clone()
} else {
SidebarWorkSummary {
state_updating: true,
..SidebarWorkSummary::default()
}
};
// Keep hunt/goal fields live on every frame, even during cache fallback.
summary.goal_objective = app.hunt.quarry.clone();
summary.goal_token_budget = app.hunt.token_budget;
summary.goal_completed = app.hunt.verdict == HuntVerdict::Hunted;
summary.goal_started_at = app.hunt.started_at;
summary.tokens_used = app.session.total_conversation_tokens;
summary
}
idling11
force-pushed
the
fix/sidebar-work-stale-cache-v2
branch
6 times, most recently
from
75739fc to
114c4d8
Compare
|
@Hmbown It seems the main branch has many compilation issues. My PR fixes some parts that weren't originally my work. |
idling11
force-pushed
the
fix/sidebar-work-stale-cache-v2
branch
4 times, most recently
from
678d987 to
64738b0
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[allow(non_snake_case)] | ||
| pub siliconflow_CN: ProviderConfigToml, |
There was a problem hiding this comment.
siliconflow_cn key
Every other provider in ProvidersToml uses all-lowercase snake_case (siliconflow, wanjie_ark, xiaomi_mimo, etc.). A user who follows that convention and writes [providers.siliconflow_cn] will silently receive an empty default config — serde silently ignores unknown fields and ProvidersToml.get(&ProviderKind::SiliconflowCN) returns &self.siliconflow_CN (which stays at Default). Adding #[serde(alias = "siliconflow_cn")] on the field would prevent the silent data loss without changing the primary documented key.
The sidebar Work panel reads checklist data from `app.todos` (`tokio::sync::Mutex`) via non-blocking `try_lock()`. When the engine holds the lock (executing `checklist_write` in a tokio task), the panel silently resets to "Work state updating..." and discards all prior data. The Tasks panel does not suffer from this because it reads plain App fields (no lock needed). Fix: cache the last successful `SidebarWorkSummary` in `App` and return it when `try_lock()` fails. Live hunt/goal fields (quarry, verdict, cycle count, token usage) are still refreshed from the current App state on every frame, even during cache fallback. - Make `SidebarWorkSummary`, `SidebarWorkChecklistItem`, `SidebarWorkStrategyStep` pub(crate) so App can store them. - Add `cached_work_summary: Option<SidebarWorkSummary>` to App. - Rewrite `sidebar_work_summary(&mut App)` to use a fallback chain: fresh locks → cache → state_updating empty state. - Add 4 unit tests covering cache populate, lock-busy fallback, no-cache+lock fallback, and live-field refresh during fallback. Closes Hmbown#2604
idling11
force-pushed
the
fix/sidebar-work-stale-cache-v2
branch
from
64738b0 to
f134267
Compare
idling11
changed the title
|
Thanks @idling11 for both the Work-panel diagnosis/fix and the compile-break repair. I reproduced the compile failure on main and have already pulled the narrow SiliconFlow-CN repair into the v0.8.52 stabilization branch. I am keeping the Work-panel cache piece separate for review so it does not get hidden inside the compile fix. Sorry main was in such a noisy state here; your report and PR made the failure much easier to unwind. |
|
Update: after a second pass for v0.8.52, I pulled the narrow Work-panel cache slice from this PR into #2626 as well, with the four focused sidebar regression tests. I kept the SiliconFlow provider wiring on the #2626 implementation path because that branch uses the shared You are credited in the v0.8.52 changelog/community section for both #2606 and this PR. Once #2626 lands, I expect this PR to be superseded for the release-blocking pieces, while any broader Work-panel cleanup can stay separate. Thanks again, and sorry the release branch made your fix harder to review cleanly. |
|
Closing as superseded by #2626, which is now merged on |
2 participants
The sidebar Work panel reads checklist data from
app.todos(tokio::sync::Mutex) via non-blockingtry_lock(). When the engine holds the lock (executingchecklist_writein a tokio task), the panel silently resets to "Work state updating..." and discards all prior data. The Tasks panel does not suffer from this because it reads plain App fields (no lock needed).Fix: cache the last successful
SidebarWorkSummaryinAppand return it whentry_lock()fails. Live hunt/goal fields (quarry, verdict, cycle count, token usage) are still refreshed from the current App state on every frame, even during cache fallback.SidebarWorkSummary,SidebarWorkChecklistItem,SidebarWorkStrategySteppub(crate) so App can store them.cached_work_summary: Option<SidebarWorkSummary>to App.sidebar_work_summary(&mut App)to use a fallback chain: fresh locks → cache → state_updating empty state.Closes #2606
Greptile Summary
This PR fixes Work panel flickering in the TUI sidebar by caching the last successful
SidebarWorkSummaryinAppand returning it as a fallback when thetodos/plan_statemutexes are held by the engine, instead of resetting to "Work state updating…". It also addsSiliconflowCNas a distinct provider alongside the existingSiliconflow.sidebar_work_summaryis rewritten to use a three-tier fallback chain (fresh lock → cached snapshot → empty updating state), with live hunt/goal fields refreshed on every frame even during fallback. Four unit tests cover all branches.SiliconflowCNis wired into the CLI enum, TUI config,ProvidersToml, env-var mapping, and documentation. Several duplicated match arms introduced in an earlier commit are also removed.config.rs,client.rs,main.rs, andui.rs.Confidence Score: 4/5
Safe to merge after fixing the SiliconflowCN credential write/read key mismatch in config.rs; the sidebar caching change is correct.
The sidebar caching logic, tests, and App field addition are all correct. The SiliconflowCN wiring has one concrete defect:
save_api_key_forandprovider_config_keyboth use"siliconflow-CN"(hyphen) as the TOML table key, butProvidersToml.siliconflow_CN(underscore) is what serde will read — so any credential written viacodewhale auth set --provider siliconflow-cnis immediately orphaned and authentication for SiliconflowCN will always fall back to the default (empty) config.crates/tui/src/config.rs —
save_api_key_forandprovider_config_keyuse the wrong separator for the SiliconflowCN TOML key.Important Files Changed
sidebar_work_summaryto cache the last successfulSidebarWorkSummaryinApp, falling back to it whentry_lock()misses. Logic and fallback chain are correct; live hunt/goal fields are refreshed even on cache fallback. Four unit tests cover the main scenarios.save_api_key_forandprovider_config_key) use"siliconflow-CN"(hyphen) as the TOML key for SiliconflowCN, butProvidersToml.siliconflow_CN(underscore) is what serde deserializes — the written credential is silently orphaned and never read back. The rest of the SiliconflowCN match-arm deduplication and formatting cleanups are correct.cached_work_summary: Option<SidebarWorkSummary>field toApp, initialized toNone. Clean, minimal change to support the caching fix in sidebar.rs.siliconflow_CN: ProviderConfigTomlfield (with#[allow(non_snake_case)]) and splits SiliconflowCN from Siliconflow inget/get_mut. The field namesiliconflow_CN(underscore) correctly reflects what serde will read from TOML, but callers inconfig.rswrite the hyphenated form, causing the read/write mismatch flagged above.SiliconflowCNto the CLI provider enum, slot function (returning"siliconflow-cn"), env-var list, and provider list. Straightforward additions with a matching test case.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[render frame - sidebar_work_summary called] --> B{try_lock todos AND plan_state} B -- both succeed --> C[Build fresh SidebarWorkSummary\nstate_updating = false] C --> D[Store clone in app.cached_work_summary] D --> E[Return fresh summary] B -- either lock busy --> F{app.cached_work_summary is Some?} F -- yes --> G[Clone cached summary] G --> H[Overwrite live fields:\nquarry, token_budget,\nverdict, tokens_used] H --> I[Return updated cache\nstate_updating stays false] F -- no --> J[Return empty SidebarWorkSummary\nstate_updating = true] E --> K[Render Work panel] I --> K J --> KComments Outside Diff (1)
crates/tui/src/tui/sidebar.rs, line 68 (link)sidebar_work_summaryper framesidebar_work_summary(app)is called here just to computehas_useful_content(), and then called again at line 587 insiderender_sidebar_workfor actual rendering. Each call now acquires both mutexes, builds a fullSidebarWorkSummary, and writes aclone()into the cache — so every frame where the Work panel is visible pays for two mutex acquisitions, two snapshot copies, and two heap allocations.This double-call pattern predates this PR, but the added
clone()on every successful path makes it marginally more expensive. A straightforward fix would be to compute the summary once inrender_sidebar_autoand pass it by reference torender_sidebar_work, eliminating the redundant invocation.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (7): Last reviewed commit: "fix(tui): cache Work panel summary to su..." | Re-trigger Greptile