feat(tui): add multi-tab system core (manager + persistence)#2864
Conversation
Sister PR to Hmbown#2753, scoped to the narrow tab-core/persistence slice Hmbown asked for in the v0.9 stewardship review. Adds the `tab` module under `crates/tui/src/tui/` and a one-line module registration in `tui/mod.rs`. Nothing else in the host changes here — the switcher / picker / meeting UI pass and the host wiring (`App::tab_manager`, keyboard shortcuts, mouse menu, tab-bar layout in `ui.rs`) live on Hmbown#2753 and land in a follow-up PR. Scope: * `tab::TabManager` with monotonic `next_tab_id`, max 9 tabs by default, snapshot/restore round-trip, group assignment, cross-tab event/links, and persistence integration * `tab::delegator::TaskDelegator` — bounded pending queue with `MAX_COMPLETED_RESULTS` auto-prune; `take_pending_for_tab` marks InProgress in place and returns a clone so subsequent `start_task` / `complete` / `fail_task` / `cancel_task` can still find the task (the previous `swap_remove` would have dropped it on the first call) * `tab::meeting::MeetingManager` — participants, messages by type (Regular / Question / Answer / Proposal / Agreement / Objection / Summary), decisions * `tab::cross_tab` — `CrossTabEvent` (TaskDelegation / ReviewRequest / MeetingInvite / ContextSync / ResultReturn) and `SharedContext` * `tab::group` — `TabGroup` / `TabGroupManager` / `GroupColor` (Red/Orange/Yellow/Green/Cyan/Blue/Magenta/Gray) * `tab::mention` — `@Tab<N>`, `@N`, `@tab<n>` (case-insensitive) parser, with `resolve_tab_mention` for 1-indexed tab lookups * `tab::persistence` — JSON file in the user's data dir, `PersistedTabState` / `PersistedTab` / `PersistedDelegation` / `PersistedGroup` with schema-version header, atomic save, bounded file size, corruption-tolerant load * `tab::benches` and `tab::key_e2e` regression suites (roundtrip, save/load, end-to-end save→load with keymap) Lint posture: * `#![allow(dead_code, unused_imports)]` on `tab/mod.rs` because the collab/UI pass is not on this branch; the public surface is intentionally exposed for the follow-up wiring in Hmbown#2753 * One pre-existing `tools/shell.rs` fix piggy-backed: drop the redundant `as *mut c_void` cast on `child.as_raw_handle()` (the return type is already `*mut c_void`). Pre-existing on `codex/v0.9.0-stewardship`; promoted to `clippy::unnecessary_cast` by rust 1.95 Local CI matrix on this branch (Windows runner, same flags as `.github/workflows/ci.yml`): * `cargo fmt --all -- --check` — pass * `cargo clippy --workspace --all-features --locked -- -D warnings` — pass, 0 errors * `cargo test --bin codewhale-tui tab::` — 63/63 pass * `git diff --exit-code -- Cargo.lock` — clean GitHub Actions will run on the standard fork-PR approval gate; the `on.pull_request` branch filter on this repo matches `codex/v0.9.0-stewardship` so the same matrix will run on ubuntu-latest, macos-latest, and windows-latest. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks @ljm3790865 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 a comprehensive multi-tab system for the CodeWhale TUI, implementing core tab management, task delegation, multi-agent meetings, tab grouping, @-mention parsing, and state persistence. Feedback on these changes highlights several critical areas for improvement: cleaning up orphaned delegation tasks and active meetings when a tab is closed to prevent state leaks; returning an error instead of silently returning a default state when the persistence file is oversized; preserving the visual order of tabs in resolve_tab_mention by removing the sorting step; persisting the status of delegations and cross_tab_links across restarts; and adding defensive validation checks for tab IDs in delegate_task and start_meeting.
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.
| /// Close a tab by index | ||
| pub fn close_tab(&mut self, index: usize) -> bool { | ||
| if index >= self.tabs.len() { | ||
| return false; | ||
| } | ||
|
|
||
| // Unassign from any group before removing | ||
| let tab_id = self.tabs[index].metadata.id; | ||
| self.groups.unassign_tab(tab_id); | ||
|
|
||
| self.tabs.remove(index); |
There was a problem hiding this comment.
When a tab is closed via close_tab, its associated delegation tasks in self.delegator.pending_tasks and its participation in active meetings in self.meeting_manager are not cleaned up. This leads to orphaned references and state leaks, as these tasks and meetings can never be completed or updated once the tab is gone. Consider cleaning up these references on tab closure (e.g., by retaining only tasks where from_tab and to_tab are still active, and removing the closed tab from active meetings).
| if metadata.len() > MAX_FILE_SIZE { | ||
| tracing::warn!( | ||
| size = metadata.len(), | ||
| max = MAX_FILE_SIZE, | ||
| path = %path.display(), | ||
| "Tab state file too large, ignoring" | ||
| ); | ||
| return Ok(PersistedTabState::default()); | ||
| } |
There was a problem hiding this comment.
Returning Ok(PersistedTabState::default()) when the tab state file exceeds MAX_FILE_SIZE is dangerous. If the file is oversized (e.g., due to corruption or unexpected growth), silently ignoring it and returning an empty state means that the next save operation will overwrite the file, permanently destroying the user's previous tabs. It is much safer to return an io::Error (similar to how JSON parsing errors are handled) so the application can detect the failure and avoid silently wiping the user's data.
if metadata.len() > MAX_FILE_SIZE {
tracing::error!(
size = metadata.len(),
max = MAX_FILE_SIZE,
path = %path.display(),
"Tab state file too large"
);
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidData,
format!("Tab state file size {} exceeds maximum allowed size {}", metadata.len(), MAX_FILE_SIZE),
));
}| pub fn resolve_tab_mention<'a, I>(tab_number: usize, tab_ids: I) -> Option<u64> | ||
| where | ||
| I: IntoIterator<Item = &'a u64>, | ||
| { | ||
| let mut sorted: Vec<u64> = tab_ids.into_iter().copied().collect(); | ||
| sorted.sort_unstable(); | ||
| if tab_number == 0 || tab_number > sorted.len() { | ||
| return None; | ||
| } | ||
| Some(sorted[tab_number - 1]) | ||
| } |
There was a problem hiding this comment.
Sorting the tab_ids in resolve_tab_mention is a semantic bug. Tab mentions (like @Tab2 or @2) should correspond to the visual order of the tabs in the tab bar (i.e., their index in self.tabs), which is the order passed by the caller. By sorting the IDs, you destroy the caller's intended order and map mentions to arbitrary ID values instead of their visual positions. Removing the sorting step preserves the caller's order and ensures correct mapping. Note that the corresponding test test_resolve_tab_mention should also be updated to reflect the unsorted visual order.
pub fn resolve_tab_mention<'a, I>(tab_number: usize, tab_ids: I) -> Option<u64>
where
I: IntoIterator<Item = &'a u64>,
{
let ids: Vec<u64> = tab_ids.into_iter().copied().collect();
if tab_number == 0 || tab_number > ids.len() {
return None;
}
Some(ids[tab_number - 1])
}| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct PersistedDelegation { | ||
| pub task_id: String, | ||
| pub from_tab: u64, | ||
| pub to_tab: u64, | ||
| pub description: String, | ||
| pub priority: Priority, | ||
| pub created_at: DateTime<Utc>, | ||
| pub completed_at: Option<DateTime<Utc>>, | ||
| pub result: Option<String>, | ||
| pub was_successful: Option<bool>, | ||
| } |
There was a problem hiding this comment.
The PersistedDelegation struct lacks a status field, which causes all active delegations to be restored as Pending (via DelegationStatus::Pending in restore_from_snapshot), losing their InProgress state. Additionally, the fields completed_at, result, and was_successful are always serialized as None because completed delegations are not persisted. Consider adding a status field to preserve the active state of delegations across restarts, and cleaning up unused fields if completed delegations are not intended to be persisted.
| pub fn snapshot(&self) -> super::persistence::PersistedTabState { | ||
| use super::persistence::PersistedTab; | ||
| let tabs: Vec<PersistedTab> = self | ||
| .tabs | ||
| .iter() | ||
| .map(|t| super::persistence::from_metadata(&t.metadata)) | ||
| .collect(); |
There was a problem hiding this comment.
The cross_tab_links map is completely omitted from the snapshot and restore_from_snapshot process. This means any established collaboration links between tabs will be silently lost when the session is persisted and restored. Consider adding cross_tab_links to the persisted state schema to preserve the collaboration topology across restarts.
| pub fn delegate_task( | ||
| &mut self, | ||
| from: TabId, | ||
| to: TabId, | ||
| description: String, | ||
| priority: Priority, | ||
| ) -> Option<String> { | ||
| self.delegator | ||
| .create_delegation(from, to, description, priority) | ||
| } | ||
|
|
There was a problem hiding this comment.
The delegate_task method does not validate whether the from and to tab IDs actually exist in self.tabs. Creating delegations with invalid or non-existent tab IDs can lead to orphaned tasks and inconsistent state. Adding defensive checks to ensure both tabs are active before creating the delegation prevents this.
| pub fn delegate_task( | |
| &mut self, | |
| from: TabId, | |
| to: TabId, | |
| description: String, | |
| priority: Priority, | |
| ) -> Option<String> { | |
| self.delegator | |
| .create_delegation(from, to, description, priority) | |
| } | |
| pub fn delegate_task( | |
| &mut self, | |
| from: TabId, | |
| to: TabId, | |
| description: String, | |
| priority: Priority, | |
| ) -> Option<String> { | |
| let has_from = self.tabs.iter().any(|t| t.metadata.id == from); | |
| let has_to = self.tabs.iter().any(|t| t.metadata.id == to); | |
| if !has_from || !has_to { | |
| return None; | |
| } | |
| self.delegator | |
| .create_delegation(from, to, description, priority) | |
| } |
| /// Start a meeting | ||
| pub fn start_meeting(&mut self, topic: String, participants: Vec<TabId>) -> Option<String> { | ||
| self.meeting_manager.start_meeting(topic, participants) | ||
| } |
There was a problem hiding this comment.
The start_meeting method does not validate whether the participant tab IDs actually exist in self.tabs. Starting meetings with invalid or non-existent tab IDs can lead to inconsistent state in MeetingManager. Adding defensive checks to ensure all participants are active tabs before starting the meeting prevents this.
pub fn start_meeting(&mut self, topic: String, participants: Vec<TabId>) -> Option<String> {
for p in &participants {
if !self.tabs.iter().any(|t| t.metadata.id == *p) {
return None;
}
}
self.meeting_manager.start_meeting(topic, participants)
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Six fixes for the bot review comments landed on PR Hmbown#2864 head 649d399. See phase2-playbook.md §7 for the triage rationale. * persistence.rs: oversized state file now surfaces an InvalidData error instead of silently returning a default. The old behaviour would let the next save overwrite the oversized file and destroy the user's data. Test updated to expect the error. * persistence.rs: PersistedDelegation gains a `status` field so in-flight `InProgress` delegations aren't silently demoted to `Pending` on restart. The snapshot now writes the live status and restore_from_snapshot honours it. Adds a regression test. * mention.rs: resolve_tab_mention no longer sorts its input — tab mentions (@tab2) must map to the visual order in the tab bar, not to an arbitrary ID sort. Test updated. * manager.rs: `pending_tasks` renamed to `completed_delegations` because the getter returns completed DelegationResults, not in-flight tasks. Docstring points to the in-flight getter `pending_delegations` to avoid the same confusion recurring. * manager.rs: delegate_task and start_meeting now validate that the from/to tab IDs (or all participant IDs) currently exist in the manager. Returns `None` on any unknown ID, preventing orphaned tasks / meetings with stale tab references. Two new regression tests cover both methods. Local CI matrix (Windows runner, flags matching ci.yml): - cargo fmt --all -- --check: exit 0 - cargo clippy --workspace --all-features --locked -- -D warnings: exit 0 - cargo test --workspace --all-features --locked: 4282 pass, 6 fail (the 6 failures are pre-existing on the baseline; not caused by this PR) - git diff --exit-code -- Cargo.lock: exit 0 The three deferred threads (#1 close_tab cleanup, Hmbown#5 cross_tab_links snapshot, Hmbown#8 TabGroup::new collision risk) are explicitly out of scope here; they belong to the follow-up collab/UI PR per the narrow-harvest promise to Hmbown. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@gemini-code-assist 7 bot threads addressed in 7fcd7d7. Summary per thread:
Local CI: cargo fmt / clippy -D warnings / cargo test all clean (4282 pass, 6 fail — all pre-existing on baseline). |
|
Thanks for the update, @ljm3790865. The progress on these threads is clear, and the renaming of Regarding the remaining items:
Overall, the architecture for the tab system is looking solid. Once the |
Six fixes for the bot review comments landed on PR Hmbown#2864 head 649d399. See phase2-playbook.md §7 for the triage rationale. * persistence.rs: oversized state file now surfaces an InvalidData error instead of silently returning a default. The old behaviour would let the next save overwrite the oversized file and destroy the user's data. Test updated to expect the error. * persistence.rs: PersistedDelegation gains a `status` field so in-flight `InProgress` delegations aren't silently demoted to `Pending` on restart. The snapshot now writes the live status and restore_from_snapshot honours it. Adds a regression test. * mention.rs: resolve_tab_mention no longer sorts its input — tab mentions (@tab2) must map to the visual order in the tab bar, not to an arbitrary ID sort. Test updated. * manager.rs: `pending_tasks` renamed to `completed_delegations` because the getter returns completed DelegationResults, not in-flight tasks. Docstring points to the in-flight getter `pending_delegations` to avoid the same confusion recurring. * manager.rs: delegate_task and start_meeting now validate that the from/to tab IDs (or all participant IDs) currently exist in the manager. Returns `None` on any unknown ID, preventing orphaned tasks / meetings with stale tab references. Two new regression tests cover both methods. Local CI matrix (Windows runner, flags matching ci.yml): - cargo fmt --all -- --check: exit 0 - cargo clippy --workspace --all-features --locked -- -D warnings: exit 0 - cargo test --workspace --all-features --locked: 4282 pass, 6 fail (the 6 failures are pre-existing on the baseline; not caused by this PR) - git diff --exit-code -- Cargo.lock: exit 0 The three deferred threads (#1 close_tab cleanup, Hmbown#5 cross_tab_links snapshot, Hmbown#8 TabGroup::new collision risk) are explicitly out of scope here; they belong to the follow-up collab/UI PR per the narrow-harvest promise to Hmbown. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
Summary
Sister PR to #2753, scoped to the narrow tab-core/persistence slice Hmbown asked for in the v0.9 stewardship review. Adds the
tabmodule undercrates/tui/src/tui/and a one-line module registration intui/mod.rs. Nothing else in the host changes here — the switcher / picker / meeting UI pass and the host wiring (App::tab_manager, keyboard shortcuts, mouse menu, tab-bar layout inui.rs) live on #2753 and land in a follow-up PR.This PR is intentionally an additive slice against
codex/v0.9.0-stewardship:crates/tui/src/tui/tab/directory, no overlapping commitspub mod tab;added tocrates/tui/src/tui/mod.rs(alphabetical)crates/tui/src/tools/shell.rsredundant cast fix (unrelated but required to keep the local CI matrix green on the currentclippy -D warnings)The conflicts Hmbown called out in #2753 (
app.rs,ui.rs,views/mod.rs,mouse_ui.rs) are deliberately not on this branch, so a rebase on top ofcodex/v0.9.0-stewardshipis a fast-forward with no merge resolution.What's in the slice
tab/mod.rspub use TabManager, type re-exports, base types (TabId,TabMetadata,TabType,Priority,TabStatus)tab/manager.rsTabManagerwith monotonicnext_tab_id, max 9 tabs by default, snapshot/restore round-trip, group assignment, cross-tab event/links, persistence integrationtab/persistence.rsPersistedTabState/PersistedTab/PersistedDelegation/PersistedGroupwith schema-version header, atomic save, bounded file size, corruption-tolerant loadtab/delegator.rsTaskDelegator— bounded pending queue withMAX_COMPLETED_RESULTSauto-prune;take_pending_for_tabmarks InProgress in place and returns a clone so subsequentstart_task/complete/fail_task/cancel_taskcan still find the task (the previousswap_removewould have dropped it on the first call)tab/meeting.rsMeetingManager— participants, messages by type (Regular / Question / Answer / Proposal / Agreement / Objection / Summary), decisionstab/cross_tab.rsCrossTabEvent(TaskDelegation / ReviewRequest / MeetingInvite / ContextSync / ResultReturn) andSharedContexttab/group.rsTabGroup/TabGroupManager/GroupColor(Red/Orange/Yellow/Green/Cyan/Blue/Magenta/Gray)tab/mention.rs@Tab<N>,@N,@tab<n>(case-insensitive) parser, withresolve_tab_mentionfor 1-indexed tab lookupstab/benches.rstab/key_e2e.rsLint posture
#![allow(dead_code, unused_imports)]ontab/mod.rsbecause the collab/UI pass is not on this branch; the public surface is intentionally exposed for the follow-up wiring in feat(tui): multi-tab system with cross-tab collaboration #2753. Each#[allow]is co-located with a one-line WHY comment so a future PR can drop the allow as the collab/UI pass lands.tools/shell.rsredundant cast (child.as_raw_handle() as *mut c_void) is fixed in passing because rust 1.95 promotes it toclippy::unnecessary_cast. Without the fix,cargo clippy --workspace --all-features --locked -- -D warnings(the CI command) would fail on this branch before it ever builds the new code.Local CI matrix
Same flags as
.github/workflows/ci.yml, run on a Windows runner:cargo fmt --all -- --checkcargo clippy --workspace --all-features --locked -- -D warningscargo test --bin codewhale-tui tab::cargo test --bin codewhale-tui delegator::cargo test --bin codewhale-tui meeting::git diff --exit-code -- Cargo.lockThe GitHub Actions run will need the standard fork-PR approval gate; once approved, the same matrix will execute on
ubuntu-latest,macos-latest, andwindows-latest. The 9-tab test suite and persistence round-trip cover the parts of the slice that the steward can break from the host side.Test plan
cargo test --bin codewhale-tui tab::— 63 unit tests cover TabManager invariants (max-tabs, monotonic id, snapshot/restore of delegations and groups, post-restore id uniqueness), DelegationTask lifecycle, MeetingManager state machine, @-mention parsing including case-insensitivity, persistence round-trip, schema-version warning, atomic save, oversized-file rejection, keymap end-to-end save/load@Tab2parsing in agent messages (the parse function is unit-tested; the in-flight wiring is in feat(tui): multi-tab system with cross-tab collaboration #2753)Follow-up
A second PR on top of this one will add the
tab/tab_bar.rsrenderer and the host wiring (App::tab_manager,ui.rskeyboard/mouse integration,views/tab_switcher.rs,views/tab_picker.rs,views/meeting_view.rs,views/mod.rs::ModalKind::Meeting). That work is staged on #2753 to keep this slice narrow per Hmbown's plan.🤖 Generated with Claude Code
Greptile Summary
Adds the
tabmodule undercrates/tui/src/tui/— a self-contained multi-tab core (manager, persistence, delegation, groups, meetings, @-mention parser) intentionally staged without host wiring, plus a one-line Clippy cast fix intools/shell.rs.TabManagermanages up to 9 tabs with monotonic ID assignment, snapshot/restore that correctly advances all counters past restored IDs, and delegation validated against live tab existence.TaskDelegatoruses aVecDequebounded at 256 for completed results,take_pending_for_tabmarks in-place (noswap_removeon take) so lifecycle calls (complete/fail_task/cancel_task) can still locate the task by ID.close_tabdoes not cancel delegations referencing the removed tab, so those tasks survive into the next snapshot and are blindly restored even when the referenced tab no longer exists.Confidence Score: 3/5
Merging now introduces a persistence defect: closing a tab leaves its in-flight delegations in the queue, they get written to disk, and are fully restored on the next startup even though the referenced tab is gone.
The additive slice is well-structured and the ID-collision, naming, and mention-ordering issues called out in previous rounds have all been addressed. The remaining gap is
close_tab: it cleans up group membership but leavesdelegator.pending_tasksholding tasks whosefrom_tab/to_tabpoint at the now-deleted tab.snapshot()includes those tasks viaall_pending(), andrestore_from_snapshotpushes them back without validating that the referenced tabs exist — so orphaned delegations accumulate across restarts. Because the persistence round-trip tests do not close tabs before saving, the defect is not caught by the current test suite.manager.rs(close_tabandrestore_from_snapshot) is the focus — the delegation cleanup gap lives there.persistence.rsis secondary (temp-file cleanup on rename failure).Important Files Changed
advance_next_id_past_existing_tasks.historyvec is unbounded unlike the delegation completed-results queue.parse_usize;resolve_tab_mentionuses caller-supplied positional order rather than sorting by ID (previous concern addressed).TabGroup::new()still uses timestamp IDs (flagged previously), but that constructor is not called by the manager.as *mut core::ffi::c_voidcast fromchild.as_raw_handle()for Windows job object assignment.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[create_tab] -->|monotonic next_tab_id| B[TabState in tabs vec] B --> C{delegate_task} C -->|both tabs exist| D[TaskDelegator.pending_tasks\nstatus=Pending] C -->|unknown tab| E[return None] D --> F[take_next_delegation\nmarks InProgress in-place] F --> G{complete / fail / cancel} G -->|complete| H[swap_remove → DelegationResult\nin completed_results VecDeque\nbounded at 256] G -->|fail / cancel| H B --> I[close_tab] I -->|unassign from group| J[TabGroupManager] I -->|⚠️ does NOT cancel| D B --> K[snapshot] K -->|all_pending incl. orphans| L[PersistedTabState JSON] L -->|atomic write via .tmp rename| M[tabs.json on disk] M --> N[restore_from_snapshot] N -->|no tab-ID validation| DComments Outside Diff (2)
crates/tui/src/tui/tab/delegator.rs, line 596-603 (link)next_idreset on restore causes task-ID collisionTaskDelegator::new()setsnext_id = 1. Whenrestore_from_snapshotcreates a freshTaskDelegatorand pushes back pending tasks (which may have IDs like"delegation_42"), the counter stays at 1. After the user creates 41 more delegations in the same session, the 42nd will get ID"delegation_42"— colliding with the restored pending task.start_task,complete,fail_task, andcancel_taskall find tasks by string ID viaiter().find(|t| t.task_id == task_id), so the collision silently mutates or completes the wrong task. The snapshot should persistnext_id(or compute it asmax(restored_task_numeric_suffix) + 1on restore) so the counter never re-issues an already-used ID.crates/tui/src/tui/tab/mention.rs, line 3127-3137 (link)resolve_tab_mentionsorts by ID value, not by visual/display orderThe function sorts the caller-supplied tab IDs numerically, then treats index
tab_number - 1of the sorted slice as the target. ButTabManagerorders tabs by insertion (thetabs: Vec<TabState>vector), so the user sees@Tab1as the leftmost tab in the bar,@Tab2as the second, and so on — regardless of their rawTabIdvalues. After a session restore, IDs are preserved from before but insertion order may differ; in any case the sorted-ID order does not match what the user sees.Concretely: if the user has three tabs with IDs
[100, 50, 200]in that visual order,resolve_tab_mention(1, [100, 50, 200])returns50(the smallest ID, Tab B), but the user intended100(the leftmost visible tab, Tab A). The function should instead accept IDs already in display order and use positional lookup without sorting.Reviews (4): Last reviewed commit: "fix(tui): advance tab restore counters" | Re-trigger Greptile