Add project-based grouped view, session archiving, and visual improvements - #2
Add project-based grouped view, session archiving, and visual improvements#2mabulgu wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a grouped (tree) vs flat TUI view toggle, project grouping and tree navigation, archive/move/title-edit workflows with filesystem operations, fork/new-session deferred commands via shell execution, changes session discovery/indexing to use encoded project dir names, and adds related tests and a dev tempfile dependency. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Input as Input Handler
participant App as App State
participant FS as File System
participant Shell as $SHELL
User->>Input: press 'a' on a session
Input->>App: set archive_confirm = Some(idx), mode = ConfirmArchive
User->>Input: press 'y' (confirm)
Input->>App: request archive_session(idx)
App->>FS: move session file -> projects-archive/...
FS-->>App: move result
App->>App: remove session, rebuild rows, clear archive_confirm, mode = Browsing
App-->>User: UI updates
Note right of App: deferred commands (NewSession/ForkSession)
User->>Input: press 'n' or 'f'
Input->>App: produce Action::NewSession(cmd) / ForkSession(cmd)
App->>Shell: sh -ic "<cmd>"
Shell-->>App: exit status
App-->>User: UI updates / status
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/tui/mod.rs (2)
338-343: Collapsing viatoggle_projectmay leaveselectedout of bounds.When a project is collapsed from an index below its header (e.g.,
selectedis on a sibling row after an expanded group),tree_rowsshrinks butselectedis not clamped.selected_tree_row()then returnsNoneuntil the user navigates. Clamp here so the UI stays responsive.🔧 Clamp `selected` after rebuilding rows
pub fn toggle_project(&mut self, group_idx: usize) { if group_idx < self.project_groups.len() { self.project_groups[group_idx].expanded = !self.project_groups[group_idx].expanded; self.tree_rows = build_tree_rows(&self.project_groups); + if !self.tree_rows.is_empty() && self.selected >= self.tree_rows.len() { + self.selected = self.tree_rows.len() - 1; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 338 - 343, toggle_project may shrink self.tree_rows and leave self.selected pointing past the end; after flipping self.project_groups[group_idx].expanded and rebuilding self.tree_rows in toggle_project, clamp self.selected to at most self.tree_rows.len().saturating_sub(1) (handle the empty case) so selected_tree_row() won't return None; update the toggle_project method to perform this clamp immediately after calling build_tree_rows.
101-149: Group sort depends on caller passing timestamp-sorteddisplay_entries.
group_by_projectpicks each group's sort key asdisplay_entries[session_indices.first()].timestamp— i.e., the timestamp of the first entry inserted, not the newest in the group.rebuild_display_entriessorts by timestamp desc so this holds after any filter change, butApp::newbuildsdisplay_entriesin0..sessions.len()order, so the initial grouped view's ordering silently relies ondiscover_sessionsalready returning newest-first. Make the invariant explicit by computing each group's max timestamp during the sort:🔧 Sort by true latest-in-group
groups.sort_by(|a, b| { - let ts_a = a.session_indices.first() - .map(|&i| display_entries[i].timestamp) - .unwrap_or_else(|| DateTime::<Utc>::MIN_UTC); - let ts_b = b.session_indices.first() - .map(|&i| display_entries[i].timestamp) - .unwrap_or_else(|| DateTime::<Utc>::MIN_UTC); + let ts_a = a.session_indices.iter() + .map(|&i| display_entries[i].timestamp) + .max() + .unwrap_or(DateTime::<Utc>::MIN_UTC); + let ts_b = b.session_indices.iter() + .map(|&i| display_entries[i].timestamp) + .max() + .unwrap_or(DateTime::<Utc>::MIN_UTC); ts_b.cmp(&ts_a) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 101 - 149, The current group sorting in group_by_project_with_content uses the first session_indices entry's timestamp (which depends on caller ordering) as the group's sort key; change it to compute the group's latest timestamp (max of display_entries[index].timestamp for all indices in ProjectGroup.session_indices) and use that for sorting so groups are ordered by true newest activity; update the sort closure in group_by_project_with_content to iterate over each group's session_indices, compute the maximum timestamp (fallback to DateTime::<Utc>::MIN_UTC), and compare those max values (keep function and struct names: group_by_project_with_content, group_by_project, ProjectGroup, display_entries, session_indices, timestamp).tests/grouping_test.rs (1)
51-59: Verify hidden coupling withdiscover_sessionsordering.
groups_sorted_by_latest_activityonly passes becausegroup_by_projectsorts bysession_indices.first()timestamp, and the fixture happens to put newest-first. Ifdiscover_sessionsever changes its ordering (or an unsorted source is used), the initial sort becomes unreliable becausedisplay_entriespassed intogroup_by_projectfromApp::neware not sorted by timestamp — they are built infiltered_indicesorder (0..sessions.len()). Consider sortingsession_indicesby timestamp insidegroup_by_projectitself so this invariant doesn't depend on the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/grouping_test.rs` around lines 51 - 59, The test relies on discover_sessions producing newest-first order, which hides a coupling: group_by_project currently assumes session_indices are pre-sorted; make this robust by sorting the session_indices by their session timestamp inside group_by_project itself (using the timestamps from display_entries/session entries) before computing groups so callers like App::new or tests that pass filtered_indices (0..sessions.len()) no longer need to guarantee ordering; update group_by_project to stable-sort session_indices by the associated timestamp (referencing group_by_project, session_indices, display_entries, make_display_entries, discover_sessions, and App::new) and adjust any comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/input.rs`:
- Around line 141-157: The current handler intercepts key 'a' whenever
app.filter_active is false even if there's no archiveable target, blocking
starting a filter with 'a'; change the logic in the 'a' branch to first compute
the potential archive target exactly as done (using app.grouped_view and
app.selected_tree_row() matching TreeRow::Session or app.selected <
app.display_entries.len()), and only set app.archive_confirm and app.mode =
Mode::ConfirmArchive and return Action::Continue when that computed display_idx
is Some; otherwise do not return early—let the key fall through to the existing
filter activation path so users can start a filter with 'a'.
- Around line 367-384: The confirm handler handle_confirm_archive currently only
cancels on Esc/'n'/'q' but leaves Mode::ConfirmArchive and archive_confirm set
for any other key; change the default match arm (`_`) to clear
app.archive_confirm = None, set app.mode = Mode::Browsing, and return
Action::Continue so any non-confirm key cancels; also update the test
archive_confirm_other_key_cancels to exercise a non-explicit key (e.g.
KeyCode::Char('x') or KeyCode::Down) to validate the new behavior.
In `@src/tui/mod.rs`:
- Around line 450-483: The archive_session function currently only removes the
session when entry.source is DisplaySource::Sessions(sidx) leaving stale
in-memory entries for DisplaySource::Content(cidx); update archive_session to
handle DisplaySource::Content(cidx) similarly by removing the item at cidx from
self.content_results and then calling self.apply_filter() (or
rebuild_display_entries via apply_filter path) so the in-memory content_results
and display_entries are updated after moving the .jsonl file; keep the existing
index-removal approach used for Sessions to stay consistent with
apply_filter/rebuild behavior.
- Around line 659-673: Change the shell fallback from "/bin/zsh" to the
POSIX-guaranteed "/bin/sh" and stop forcing an interactive shell; in the
deferred_command block update the call that builds the child process
(Command::new(&shell) and the .arg("-ic") usage) to use "/bin/sh" as the default
when SHELL is unset and replace "-ic" with "-c" so the command runs
non-interactively (keep interactive flag only if you intentionally need
aliases/functions).
In `@tests/tui_input_test.rs`:
- Around line 268-301: The test sets CLAUDE_HOME with std::env::set_var then
calls discover_sessions and App::new and archive_session, but
std::env::remove_var is unguarded so CLAUDE_HOME can leak on panic; either wrap
the env change in a RAII guard that removes CLAUDE_HOME in Drop (create a small
helper type used around the set_var/remove_var) or refactor the test to avoid
mutating process state by passing a claude_home: &Path into discover_sessions
and App::new (and adjust archive_session to accept a claude_home or rely on the
App field) so you no longer call std::env::set_var/remove_var in the test;
update references to set_var/remove_var, discover_sessions, App::new,
App::archive_session and get_claude_home accordingly.
---
Nitpick comments:
In `@src/tui/mod.rs`:
- Around line 338-343: toggle_project may shrink self.tree_rows and leave
self.selected pointing past the end; after flipping
self.project_groups[group_idx].expanded and rebuilding self.tree_rows in
toggle_project, clamp self.selected to at most
self.tree_rows.len().saturating_sub(1) (handle the empty case) so
selected_tree_row() won't return None; update the toggle_project method to
perform this clamp immediately after calling build_tree_rows.
- Around line 101-149: The current group sorting in
group_by_project_with_content uses the first session_indices entry's timestamp
(which depends on caller ordering) as the group's sort key; change it to compute
the group's latest timestamp (max of display_entries[index].timestamp for all
indices in ProjectGroup.session_indices) and use that for sorting so groups are
ordered by true newest activity; update the sort closure in
group_by_project_with_content to iterate over each group's session_indices,
compute the maximum timestamp (fallback to DateTime::<Utc>::MIN_UTC), and
compare those max values (keep function and struct names:
group_by_project_with_content, group_by_project, ProjectGroup, display_entries,
session_indices, timestamp).
In `@tests/grouping_test.rs`:
- Around line 51-59: The test relies on discover_sessions producing newest-first
order, which hides a coupling: group_by_project currently assumes
session_indices are pre-sorted; make this robust by sorting the session_indices
by their session timestamp inside group_by_project itself (using the timestamps
from display_entries/session entries) before computing groups so callers like
App::new or tests that pass filtered_indices (0..sessions.len()) no longer need
to guarantee ordering; update group_by_project to stable-sort session_indices by
the associated timestamp (referencing group_by_project, session_indices,
display_entries, make_display_entries, discover_sessions, and App::new) and
adjust any comments accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9790d19e-bd5a-4ae6-b89e-de7ce4617ffc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/main.rssrc/session.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/view.rstests/grouping_test.rstests/tui_input_test.rs
💤 Files with no reviewable changes (1)
- src/session.rs
| fn archive_session_moves_file() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let project_dir = tmp.path().join("projects").join("-Users-test-myproject"); | ||
| fs::create_dir_all(&project_dir).unwrap(); | ||
|
|
||
| let session_id = "abc123"; | ||
| let session_file = project_dir.join(format!("{session_id}.jsonl")); | ||
| fs::write(&session_file, r#"{"type":"user","cwd":"/Users/test/myproject","sessionId":"abc123","message":{"role":"user","content":"hello"},"uuid":"u1","timestamp":"2025-01-01T00:00:00.000Z"} | ||
| {"type":"assistant","cwd":"/Users/test/myproject","sessionId":"abc123","message":{"role":"assistant","content":[{"type":"text","text":"hi"}]},"uuid":"u2","timestamp":"2025-01-01T00:01:00.000Z"} | ||
| "#).unwrap(); | ||
|
|
||
| std::env::set_var("CLAUDE_HOME", tmp.path().to_str().unwrap()); | ||
|
|
||
| let sessions = discover_sessions(tmp.path()); | ||
| assert!(!sessions.is_empty(), "should discover the test session"); | ||
|
|
||
| let session_index = HashMap::new(); | ||
| let theme = Theme::dark(); | ||
| let mut app = App::new(sessions, session_index, theme, false); | ||
| assert!(!app.display_entries.is_empty()); | ||
|
|
||
| let result = app.archive_session(0); | ||
| assert!(result.is_ok(), "archive should succeed: {:?}", result); | ||
|
|
||
| assert!(!session_file.exists(), "original file should be moved"); | ||
|
|
||
| let archive_file = tmp.path() | ||
| .join("projects-archive") | ||
| .join("-Users-test-myproject") | ||
| .join(format!("{session_id}.jsonl")); | ||
| assert!(archive_file.exists(), "file should exist in archive"); | ||
|
|
||
| std::env::remove_var("CLAUDE_HOME"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other tests/production code that also read or mutate CLAUDE_HOME.
rg -nP '\bCLAUDE_HOME\b' -C2
rg -nP 'env::(set_var|remove_var)' -C2Repository: cc-deck/cc-session
Length of output: 9013
🏁 Script executed:
#!/bin/bash
# Check for other tests that call get_claude_home() or discover_sessions()
echo "=== Tests calling get_claude_home or discover_sessions ==="
rg -l 'get_claude_home|discover_sessions' tests/ --type rust
echo ""
echo "=== All test files in tests/ ==="
fd '\.rs$' tests/
echo ""
echo "=== Check if there's a test framework or serial runner already in use ==="
rg 'serial_test|#\[serial\]|#\[test\]' tests/tui_input_test.rs -A1 -B1
echo ""
echo "=== Full context around archive_session_moves_file test ==="
sed -n '260,305p' tests/tui_input_test.rsRepository: cc-deck/cc-session
Length of output: 3358
🏁 Script executed:
#!/bin/bash
# Check what discovery_test, filter_test, grouping_test do with get_claude_home/discover_sessions
echo "=== discovery_test.rs usage ==="
rg -B3 -A3 'get_claude_home|discover_sessions' tests/discovery_test.rs | head -60
echo ""
echo "=== filter_test.rs usage ==="
rg -B3 -A3 'get_claude_home|discover_sessions' tests/filter_test.rs | head -40
echo ""
echo "=== grouping_test.rs usage ==="
rg -B3 -A3 'get_claude_home|discover_sessions' tests/grouping_test.rs | head -40
echo ""
echo "=== Check if discover_sessions is called with tmp.path() in those tests ==="
rg 'discover_sessions\(' tests/discovery_test.rs tests/filter_test.rs tests/grouping_test.rs -A1 -B1Repository: cc-deck/cc-session
Length of output: 8818
🏁 Script executed:
#!/bin/bash
# Check the signature and implementation of discover_sessions
echo "=== discover_sessions function signature and first 30 lines ==="
rg -A30 'pub fn discover_sessions' src/discovery.rs
echo ""
echo "=== Does discover_sessions() call get_claude_home() internally? ==="
sed -n '30,100p' src/discovery.rs | rg 'get_claude_home|fn discover_sessions'Repository: cc-deck/cc-session
Length of output: 1249
🏁 Script executed:
#!/bin/bash
# Check app.archive_session() implementation to see if it uses get_claude_home() or reads CLAUDE_HOME
echo "=== Search for archive_session implementation ==="
rg -A40 'fn archive_session' src/ --type rust
echo ""
echo "=== Check if archive_session or any called functions read CLAUDE_HOME ==="
rg 'get_claude_home' src/ --type rustRepository: cc-deck/cc-session
Length of output: 2791
Unguarded env::remove_var risks leaking CLAUDE_HOME on panic.
std::env::set_var / remove_var mutate process-wide state. If any assertion between lines 282 and 298 panics, remove_var never executes and CLAUDE_HOME leaks into subsequent tests. Although the current test suite doesn't exercise get_claude_home() in other tests (most pass explicit paths to discover_sessions), the production code archive_session() calls get_claude_home() internally, making the test's reliance on the env var explicit and the leak risk real.
Guard cleanup with a drop type or refactor to pass claude_home: &Path through App construction and archive_session to avoid env mutation entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tui_input_test.rs` around lines 268 - 301, The test sets CLAUDE_HOME
with std::env::set_var then calls discover_sessions and App::new and
archive_session, but std::env::remove_var is unguarded so CLAUDE_HOME can leak
on panic; either wrap the env change in a RAII guard that removes CLAUDE_HOME in
Drop (create a small helper type used around the set_var/remove_var) or refactor
the test to avoid mutating process state by passing a claude_home: &Path into
discover_sessions and App::new (and adjust archive_session to accept a
claude_home or rely on the App field) so you no longer call
std::env::set_var/remove_var in the test; update references to
set_var/remove_var, discover_sessions, App::new, App::archive_session and
get_claude_home accordingly.
b6385f3 to
bcc638d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/search.rs (1)
243-321:⚠️ Potential issue | 🟠 Major
project_pathconvention is inconsistent with discovery — fallback sessions will break archive/move/grouping.After this PR,
project_pathis the encoded directory name (e.g.-Users-abulgu-github-repos-mabulgu-cc-session) everywhere else (src/discovery.rsline 183, this file'sbuild_session_indexat line 25,src/tui/mod.rsarchive/move at lines 480‑486 and 553‑558, and grouping ingroup_by_projectline 118). Butsearch_file_with_metadatastill setsproject_path: cwd.clone()at line 313.Practical consequences when a content-search hit falls back to this function (session not in the pre-built index):
- Archiving/moving the session will try
claude_home/projects/<cwd>/<id>.jsonl, which is an absolute path underprojects/—rename/read_to_stringwill fail.- Grouping will place the content-only session under a distinct group keyed on its real cwd, so the same project appears as two separate groups.
🔧 Use the encoded parent directory name instead
fn search_file_with_metadata(path: &Path, re: &Regex) -> Option<Session> { let session_id = path.file_stem()?.to_str()?.to_string(); + let encoded_dir = path.parent()?.file_name()?.to_str()?.to_string(); let file = fs::File::open(path).ok()?; @@ Some(Session { id: session_id, - project_path: cwd.clone(), + project_path: encoded_dir, project_name, git_branch: entry.git_branch, timestamp, first_message, cwd, project_exists, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/search.rs` around lines 243 - 321, search_file_with_metadata currently sets project_path to the raw cwd (project_path: cwd.clone()), which is inconsistent with the encoded directory name used elsewhere (discovery/build_session_index and grouping/archiving logic); change search_file_with_metadata to compute project_path using the same encoding routine used by discovery/build_session_index (i.e., replace project_path: cwd.clone() with the encoded parent directory name produced by the project's encoding function used in discovery.rs), keeping cwd and project_exists based on the original cwd so file operations still use the real path.
♻️ Duplicate comments (1)
tests/tui_input_test.rs (1)
268-301:⚠️ Potential issue | 🟠 Major
CLAUDE_HOMEmutation in two tests races with Cargo's parallel test runner and leaks on panic.Both
archive_session_moves_fileandmove_session_moves_filecallstd::env::set_var("CLAUDE_HOME", …)/std::env::remove_var(…). Two issues:
- Cargo runs integration tests in the same binary concurrently by default — if both tests run in parallel (or alongside any other test that indirectly hits
get_claude_home()), one test'sset_varclobbers the other's temp dir for the duration of the overlap, producing flaky failures.remove_varis unreachable if any assertion between set and remove panics, leakingCLAUDE_HOMEinto subsequent tests within the same binary.The cleanest fix is to thread
claude_home: &PaththroughApp::archive_session/App::move_session(andget_claude_home()callers insrc/tui/mod.rs) so tests don't need to mutate process env at all. A lighter fix is a sharedMutex-guarded "env lock" in a test helper plus an RAII drop guard that restores/removes the var. Either way, unguardedset_var/remove_varpairs around unprotected assertions will bite eventually.Also applies to: 378-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui_input_test.rs` around lines 268 - 301, The tests mutate CLAUDE_HOME unsafely causing races and leaks; update the code to avoid global env mutation by either (preferred) threading a claude_home: &Path (or PathBuf) parameter through get_claude_home() callers and the App methods (App::archive_session and App::move_session) so tests pass explicit temp dirs, or (lighter) add a test helper that acquires a global Mutex and returns an RAII guard which sets CLAUDE_HOME on creation and restores/removes it on Drop, then use that guard in archive_session_moves_file and move_session_moves_file; change tests to stop calling std::env::set_var/remove_var directly and reference App::archive_session/App::move_session/get_claude_home to locate where to accept the injected path or use the guarded env helper.
🧹 Nitpick comments (2)
src/tui/mod.rs (1)
177-182: Optional: replace the(String, String, String)tuple with a named struct.
MoveState::projects: Vec<(String, String, String)>with a comment saying(encoded_path, display_name, cwd)is easy to misread, especially at the call sites inhandle_move_select(let (target, _, cwd) = …) andstart_move. A smallMoveTarget { encoded_path, display_name, cwd }struct would make the intent self-evident and eliminate the positional unpack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 177 - 182, Replace the projects Vec tuple with a named struct to improve readability: define a MoveTarget struct with fields encoded_path: String, display_name: String, cwd: String and change MoveState::projects to Vec<MoveTarget>; update call sites (e.g., handle_move_select and start_move) to pattern-match or access fields by name (instead of let (target, _, cwd) = …) so code uses target.encoded_path / target.display_name / target.cwd or destructures MoveTarget { encoded_path, display_name, cwd } where needed.src/discovery.rs (1)
71-96: Consider memoizingdecode_encoded_diracross sessions sharing the same project directory.All sessions under the same project share an identical
encoded_dir, butparse_session_fileinvokesdecode_encoded_dirfor every JSONL file, and each invocation issues up to O(K²)is_dirstat calls for a path with K dash-separated segments. For users with many sessions per project this is avoidable I/O on the parallel hot path.A small memoization keyed on
encoded_dir(e.g., compute the decoded path once per directory indiscover_sessionsbefore thepar_iterand pass the result or a sharedHashMapintoparse_session_file) would eliminate the repeated probing without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/discovery.rs` around lines 71 - 96, The decode_encoded_dir function performs expensive filesystem probing per call; memoize its results keyed by encoded_dir to avoid repeated O(K²) is_dir checks. In discover_sessions, compute decode_encoded_dir(encoded) once per unique encoded_dir before spawning the par_iter used by parse_session_file, store results in a shared HashMap (or compute a Vec of (encoded_dir, decoded_path) and pass the decoded_path into parse_session_file), and update parse_session_file to accept the precomputed decoded path (or lookup the HashMap) so individual workers no longer call decode_encoded_dir repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/mod.rs`:
- Around line 748-773: The post-operation selection clamp in the UI handler is
dead because archive_session and move_session call apply_filter (which resets
self.selected = 0 and self.scroll_offset = 0), so restore and clamp the previous
selection inside those methods before they call apply_filter (or make
apply_filter accept a preserve_selected flag) so the selection survives: capture
current selected into a local (e.g. old_selected) inside
archive_session/move_session, perform the list mutation, call apply_filter while
preserving selection, then set self.selected = min(old_selected,
self.visible_row_count().saturating_sub(1)) and adjust scroll_offset as needed
to keep the selected visible; reference archive_session, move_session,
apply_filter, selected, visible_row_count, and scroll_offset when making the
change.
- Around line 544-599: move_session currently only removes the moved session
from self.sessions for DisplaySource::Sessions, leaving stale entries when
entry.source is DisplaySource::Content; update the match on entry.source in
move_session to also handle DisplaySource::Content(cidx) by removing the item
from self.content_results (using the cidx index) and then call
self.apply_filter() just like the Sessions branch so UI state matches the moved
JSONL on disk; reference DisplaySource::Content, self.content_results,
self.sessions, move_session, and apply_filter when making the change.
In `@src/tui/view.rs`:
- Around line 297-322: The current byte-slice truncation of project names (using
&name[..inner_w.saturating_sub(5)]) can panic on multi-byte UTF‑8 and miscompute
visible widths; replace that logic by calling the char-aware truncate_str helper
to produce the label and base the truncation width on the available space after
the prefix (e.g. truncate_str(name, inner_w.saturating_sub(prefix.len()))), then
compute pad using inner_w.saturating_sub(prefix.len() + label.len()) and keep
the rest of the Line/Span construction the same so non-ASCII names won't panic
or misrender.
---
Outside diff comments:
In `@src/search.rs`:
- Around line 243-321: search_file_with_metadata currently sets project_path to
the raw cwd (project_path: cwd.clone()), which is inconsistent with the encoded
directory name used elsewhere (discovery/build_session_index and
grouping/archiving logic); change search_file_with_metadata to compute
project_path using the same encoding routine used by
discovery/build_session_index (i.e., replace project_path: cwd.clone() with the
encoded parent directory name produced by the project's encoding function used
in discovery.rs), keeping cwd and project_exists based on the original cwd so
file operations still use the real path.
---
Duplicate comments:
In `@tests/tui_input_test.rs`:
- Around line 268-301: The tests mutate CLAUDE_HOME unsafely causing races and
leaks; update the code to avoid global env mutation by either (preferred)
threading a claude_home: &Path (or PathBuf) parameter through get_claude_home()
callers and the App methods (App::archive_session and App::move_session) so
tests pass explicit temp dirs, or (lighter) add a test helper that acquires a
global Mutex and returns an RAII guard which sets CLAUDE_HOME on creation and
restores/removes it on Drop, then use that guard in archive_session_moves_file
and move_session_moves_file; change tests to stop calling
std::env::set_var/remove_var directly and reference
App::archive_session/App::move_session/get_claude_home to locate where to accept
the injected path or use the guarded env helper.
---
Nitpick comments:
In `@src/discovery.rs`:
- Around line 71-96: The decode_encoded_dir function performs expensive
filesystem probing per call; memoize its results keyed by encoded_dir to avoid
repeated O(K²) is_dir checks. In discover_sessions, compute
decode_encoded_dir(encoded) once per unique encoded_dir before spawning the
par_iter used by parse_session_file, store results in a shared HashMap (or
compute a Vec of (encoded_dir, decoded_path) and pass the decoded_path into
parse_session_file), and update parse_session_file to accept the precomputed
decoded path (or lookup the HashMap) so individual workers no longer call
decode_encoded_dir repeatedly.
In `@src/tui/mod.rs`:
- Around line 177-182: Replace the projects Vec tuple with a named struct to
improve readability: define a MoveTarget struct with fields encoded_path:
String, display_name: String, cwd: String and change MoveState::projects to
Vec<MoveTarget>; update call sites (e.g., handle_move_select and start_move) to
pattern-match or access fields by name (instead of let (target, _, cwd) = …) so
code uses target.encoded_path / target.display_name / target.cwd or destructures
MoveTarget { encoded_path, display_name, cwd } where needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c01372f5-4fdf-4d1e-bd32-ed5ee9563244
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/discovery.rssrc/main.rssrc/search.rssrc/session.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/view.rstests/grouping_test.rstests/tui_input_test.rs
💤 Files with no reviewable changes (1)
- src/session.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/grouping_test.rs
feb1253 to
04d2b73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tui/view.rs (2)
66-70:⚠️ Potential issue | 🟡 MinorUse the dash prefix in flat view too.
Grouped rows use
-for non-selected sessions, but flat view still renders blanks, so--flatmisses the new visual affordance described by the PR.🎨 Proposed fix
let (cursor, cursor_len) = if is_selected { ("\u{27A4} ", 2) } else { - (" ", 2) + ("- ", 2) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/view.rs` around lines 66 - 70, The flat view currently renders blanks for non-selected rows; change the cursor assignment so non-selected rows use "- " instead of " " so the dash prefix appears in flat view too: update the block that sets (cursor, cursor_len) based on is_selected (the variables cursor and cursor_len) to return ("- ", 2) for the non-selected case while keeping the selected case ("\u{27A4} ", 2) unchanged; ensure any code that relies on cursor_len still aligns with the new prefix.
540-564:⚠️ Potential issue | 🟡 MinorUpdate stale “copy & exit” help text.
Action::CopyCommandis now executed as a deferred shell command, so the conversation status bar should not advertise clipboard copy.📝 Proposed text update
- Span::styled("n/N next/prev / search Esc clear Enter copy & exit", dim), + Span::styled("n/N next/prev / search Esc clear Enter resume & exit", dim), ... - Span::styled("n/N next/prev / search Esc clear Enter copy & exit", dim), + Span::styled("n/N next/prev / search Esc clear Enter resume & exit", dim), ... - "Space/b scroll g/G top/bottom / search Enter copy & exit Esc back", + "Space/b scroll g/G top/bottom / search Enter resume & exit Esc back",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/view.rs` around lines 540 - 564, Help text still reads "Enter copy & exit" even though Action::CopyCommand now executes a deferred shell command; find the Span::styled occurrences that contain the literal "Enter copy & exit" (used alongside conv.search_query and conv.initial_search_terms when building the Line::from spans after format_project_label) and update the text to not advertise clipboard copy (for example "Enter run command & exit" or "Enter execute command & exit"); make the same replacement in every branch where that span appears so the status bar matches the new Action::CopyCommand behavior.
♻️ Duplicate comments (1)
tests/tui_input_test.rs (1)
279-300:⚠️ Potential issue | 🟠 MajorGuard
CLAUDE_HOMEcleanup in filesystem tests.Both tests mutate process-wide environment and rely on tail-position
remove_var; a panic before cleanup leaksCLAUDE_HOMEinto later tests. Wrap this in a small RAII guard or avoid env mutation by injectingclaude_home.🧪 Minimal RAII guard sketch
+struct EnvVarGuard { + key: &'static str, + previous: Option<String>, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let previous = std::env::var(key).ok(); + std::env::set_var(key, value); + Self { key, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + if let Some(previous) = &self.previous { + std::env::set_var(self.key, previous); + } else { + std::env::remove_var(self.key); + } + } +}Then replace each
set_var/remove_varpair with a scoped guard.Also applies to: 392-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui_input_test.rs` around lines 279 - 300, The tests mutate the process-wide CLAUDE_HOME via std::env::set_var/remove_var which can leak on panic; replace the pair in tests (around discover_sessions, App::new, and archive_session usage) with a scoped RAII guard that sets CLAUDE_HOME on creation and removes/restores it in Drop (or alternately refactor to inject a claude_home path into discover_sessions/App::new to avoid touching env), and update both occurrences (around the block using discover_sessions and the other block at 392-417) to use that guard so cleanup happens even on panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/input.rs`:
- Around line 177-187: The handler currently only creates a NewSession when
app.grouped_view is true, so pressing 'n' in flat view falls through to search;
update the branch around app.grouped_view to also handle flat mode by extracting
cwd from app.selected_tree_row() for the selected Session or Project (use the
same TreeRow pattern matches: Some(TreeRow::Project(gi)) and
Some(TreeRow::Session { project_idx, .. }) to compute cwd), filter out empty
cwd, escape single quotes, and return Action::NewSession(format!("cd '{}' &&
claude", escaped)) in that case; keep the existing logic that builds cwd and
escapes quotes and ensure the condition allows this path when grouped_view is
false.
In `@src/tui/mod.rs`:
- Around line 583-586: The current move logic writes the new JSONL then calls
std::fs::remove_file(&src) but ignores errors, causing duplicate sessions if
removal fails; change the code that performs the move (the block using dst_dir,
session.id, updated, dst, src and calling std::fs::remove_file) to check and
propagate or handle the removal error instead of ignoring it — e.g., replace the
ignored call with a checked result (map_err or if let Err(e) { return
Err(format!("failed to remove source session file {src:?}: {e}")) }) so the
function returns an error when remove_file fails and the in-memory entry is only
removed on success.
---
Outside diff comments:
In `@src/tui/view.rs`:
- Around line 66-70: The flat view currently renders blanks for non-selected
rows; change the cursor assignment so non-selected rows use "- " instead of " "
so the dash prefix appears in flat view too: update the block that sets (cursor,
cursor_len) based on is_selected (the variables cursor and cursor_len) to return
("- ", 2) for the non-selected case while keeping the selected case ("\u{27A4}
", 2) unchanged; ensure any code that relies on cursor_len still aligns with the
new prefix.
- Around line 540-564: Help text still reads "Enter copy & exit" even though
Action::CopyCommand now executes a deferred shell command; find the Span::styled
occurrences that contain the literal "Enter copy & exit" (used alongside
conv.search_query and conv.initial_search_terms when building the Line::from
spans after format_project_label) and update the text to not advertise clipboard
copy (for example "Enter run command & exit" or "Enter execute command & exit");
make the same replacement in every branch where that span appears so the status
bar matches the new Action::CopyCommand behavior.
---
Duplicate comments:
In `@tests/tui_input_test.rs`:
- Around line 279-300: The tests mutate the process-wide CLAUDE_HOME via
std::env::set_var/remove_var which can leak on panic; replace the pair in tests
(around discover_sessions, App::new, and archive_session usage) with a scoped
RAII guard that sets CLAUDE_HOME on creation and removes/restores it in Drop (or
alternately refactor to inject a claude_home path into
discover_sessions/App::new to avoid touching env), and update both occurrences
(around the block using discover_sessions and the other block at 392-417) to use
that guard so cleanup happens even on panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98aadb60-2c15-43b5-93c3-9eb168dfd188
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/discovery.rssrc/main.rssrc/search.rssrc/session.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/view.rstests/grouping_test.rstests/tui_input_test.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/search.rs
- src/session.rs
- tests/grouping_test.rs
04d2b73 to
792bed8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tui/view.rs (1)
536-554:⚠️ Potential issue | 🟡 MinorUpdate search-mode Enter help to match direct resume behavior.
Lines 543 and 553 still advertise
Enter copy & exit, but the PR now resumes via deferred shell execution and the non-search help saysEnter resume.📝 Suggested text fix
- Span::styled("n/N next/prev / search Esc clear Enter copy & exit", dim), + Span::styled("n/N next/prev / search Esc clear Enter resume", dim), @@ - Span::styled("n/N next/prev / search Esc clear Enter copy & exit", dim), + Span::styled("n/N next/prev / search Esc clear Enter resume", dim),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/view.rs` around lines 536 - 554, Update the search-mode help text to reflect the new resume behavior: in the branches that render when conv.search_confirmed && !conv.match_positions.is_empty() and when !conv.initial_search_terms.is_empty(), replace the trailing help string "Enter copy & exit" with "Enter resume" so the Span::styled(...) that currently uses dim shows the correct action; you can find these spans in the code that builds the Line from vec! alongside format_project_label(&conv.session) and the Style::default().fg(app.theme.status_label_bg) usage.src/tui/mod.rs (1)
628-645:⚠️ Potential issue | 🟠 MajorPreserve grouped selection using tree-row identity, not display index.
In grouped view,
self.selectedindexestree_rows, but this block treats it as adisplay_entriesindex. When content results arrive, selection can jump to the wrong row or pasttree_rows.len().Track a selected project path or session id from
selected_tree_row()before rebuilding, then restore by searchingtree_rowsafterrebuild_display_entries().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 628 - 645, Before replacing content_results and calling rebuild_display_entries(), capture the identity of the currently selected tree row (use selected_tree_row() to get its project path or session id) instead of treating self.selected as a display_entries index; then after self.rebuild_display_entries() restore selection by searching self.tree_rows for the matching identity (e.g., compare saved project path/session id with each tree_row) and set self.selected to that tree_rows index. Replace the existing selected_id/display_entries.position logic (which uses display_session and display_entries) with this tree-row identity preservation flow around content_results, content_search_state, and rebuild_display_entries.
♻️ Duplicate comments (3)
tests/tui_input_test.rs (1)
279-300:⚠️ Potential issue | 🟠 MajorGuard and serialize
CLAUDE_HOMEmutation in filesystem tests.These tests mutate process-wide env state. A panic skips cleanup, and parallel test execution can make
archive_session()/move_session()read the other test’s tempCLAUDE_HOME.Prefer avoiding env mutation by injecting the Claude home into
App; otherwise use a shared mutex plus an RAII guard that restores the previous value inDrop.🧪 Minimal test-side guard pattern
+static CLAUDE_HOME_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +struct ClaudeHomeGuard { + previous: Option<String>, +} + +impl ClaudeHomeGuard { + fn set(path: &std::path::Path) -> Self { + let previous = std::env::var("CLAUDE_HOME").ok(); + std::env::set_var("CLAUDE_HOME", path); + Self { previous } + } +} + +impl Drop for ClaudeHomeGuard { + fn drop(&mut self) { + if let Some(previous) = &self.previous { + std::env::set_var("CLAUDE_HOME", previous); + } else { + std::env::remove_var("CLAUDE_HOME"); + } + } +} + #[test] fn archive_session_moves_file() { + let _lock = CLAUDE_HOME_LOCK.lock().unwrap(); let tmp = tempfile::tempdir().unwrap(); @@ - std::env::set_var("CLAUDE_HOME", tmp.path().to_str().unwrap()); + let _guard = ClaudeHomeGuard::set(tmp.path()); @@ - std::env::remove_var("CLAUDE_HOME"); }Apply the same guard to
move_session_moves_file.Also applies to: 392-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui_input_test.rs` around lines 279 - 300, The tests mutate the global CLAUDE_HOME env var causing flakiness and leaked state; fix by guarding and restoring the env or injecting the path into App: create an RAII env guard (sets CLAUDE_HOME on creation and restores previous value in Drop) or modify App::new/discover_sessions to accept an explicit claude_home path and update tests to pass tmp.path() directly; apply the same guard/pattern to both the archive_session test (which calls discover_sessions and App::new then app.archive_session) and the move_session_moves_file test so cleanup always runs and parallel tests cannot observe the temp CLAUDE_HOME.src/tui/mod.rs (1)
583-586:⚠️ Potential issue | 🟠 MajorDo not report a move as successful if source removal fails.
Line 586 ignores
remove_fileerrors. If removal fails after writingdst, the next discovery can show duplicate sessions while the UI has already removed the in-memory entry.🛡️ Suggested fix
let dst = dst_dir.join(format!("{}.jsonl", session.id)); std::fs::write(&dst, &updated) .map_err(|e| format!("failed to write moved session: {e}"))?; - let _ = std::fs::remove_file(&src); + std::fs::remove_file(&src).map_err(|e| { + let _ = std::fs::remove_file(&dst); + format!("failed to remove original session: {e}") + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 583 - 586, The code currently ignores errors from std::fs::remove_file(&src) after writing dst (dst_dir.join(format!("{}.jsonl", session.id))), which can leave duplicates; change the logic in src/tui/mod.rs so remove_file(&src) is not silently dropped: call std::fs::remove_file(&src).map_err(|e| format!("failed to remove original session file {}: {}", src.display(), e))? and, to avoid leaving the copied dst on failure, attempt to roll back by removing dst (std::fs::remove_file(&dst).ok()) before returning the error; ensure you reference the existing dst, src, session.id, and updated variables when implementing this.src/tui/input.rs (1)
159-169:⚠️ Potential issue | 🟠 MajorHandle
nin flat view too.
n newis advertised in both views, but Line 159 limits the action toapp.grouped_view; in flat mode,nbecomes filter input instead of starting a new session.🐛 Suggested fix
- if c == 'n' && !app.filter_active && app.grouped_view { - let cwd = match app.selected_tree_row().cloned() { - Some(TreeRow::Project(gi)) => Some(app.project_groups[gi].cwd.clone()), - Some(TreeRow::Session { project_idx, .. }) => Some(app.project_groups[project_idx].cwd.clone()), - None => None, - }; + if c == 'n' && !app.filter_active { + let cwd = if app.grouped_view { + match app.selected_tree_row().cloned() { + Some(TreeRow::Project(gi)) => Some(app.project_groups[gi].cwd.clone()), + Some(TreeRow::Session { project_idx, .. }) => { + Some(app.project_groups[project_idx].cwd.clone()) + } + None => None, + } + } else if app.selected < app.display_entries.len() { + let entry = &app.display_entries[app.selected]; + Some(app.display_session(entry).cwd.clone()) + } else { + None + }; if let Some(cwd) = cwd.filter(|c| !c.is_empty()) { let escaped = cwd.replace('\'', "'\\''"); return Action::NewSession(format!("cd '{}' && claude", escaped)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/input.rs` around lines 159 - 169, The keypress handler currently only starts a new session when app.grouped_view is true; remove that restriction so pressing 'n' when !app.filter_active works in both grouped and flat modes. In the block with the c == 'n' check in src/tui/input.rs (around the selected_tree_row() usage and the Action::NewSession return), remove the app.grouped_view condition (keep !app.filter_active), leaving the existing logic that derives cwd from selected_tree_row() (TreeRow::Project / TreeRow::Session) and returns Action::NewSession(format!("cd '{}' && claude", escaped)); ensure selected_tree_row() is used unchanged so behavior is consistent in flat view.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/mod.rs`:
- Around line 498-505: The match is attempting to move `entry.source` out of a
`&DisplayEntry`, which is illegal because `DisplaySource` is Clone but not Copy;
fix by cloning the source or matching on a reference: obtain a cloned value
(`let source = entry.source.clone()`) or match on `&entry.source` and adjust
arms to use `sidx`/`cidx` accordingly, then call `self.sessions.remove(sidx)`
and `self.content_results.remove(cidx)`; apply the same change for the other
occurrence handling lines 594-601 as well.
---
Outside diff comments:
In `@src/tui/mod.rs`:
- Around line 628-645: Before replacing content_results and calling
rebuild_display_entries(), capture the identity of the currently selected tree
row (use selected_tree_row() to get its project path or session id) instead of
treating self.selected as a display_entries index; then after
self.rebuild_display_entries() restore selection by searching self.tree_rows for
the matching identity (e.g., compare saved project path/session id with each
tree_row) and set self.selected to that tree_rows index. Replace the existing
selected_id/display_entries.position logic (which uses display_session and
display_entries) with this tree-row identity preservation flow around
content_results, content_search_state, and rebuild_display_entries.
In `@src/tui/view.rs`:
- Around line 536-554: Update the search-mode help text to reflect the new
resume behavior: in the branches that render when conv.search_confirmed &&
!conv.match_positions.is_empty() and when !conv.initial_search_terms.is_empty(),
replace the trailing help string "Enter copy & exit" with "Enter resume" so the
Span::styled(...) that currently uses dim shows the correct action; you can find
these spans in the code that builds the Line from vec! alongside
format_project_label(&conv.session) and the
Style::default().fg(app.theme.status_label_bg) usage.
---
Duplicate comments:
In `@src/tui/input.rs`:
- Around line 159-169: The keypress handler currently only starts a new session
when app.grouped_view is true; remove that restriction so pressing 'n' when
!app.filter_active works in both grouped and flat modes. In the block with the c
== 'n' check in src/tui/input.rs (around the selected_tree_row() usage and the
Action::NewSession return), remove the app.grouped_view condition (keep
!app.filter_active), leaving the existing logic that derives cwd from
selected_tree_row() (TreeRow::Project / TreeRow::Session) and returns
Action::NewSession(format!("cd '{}' && claude", escaped)); ensure
selected_tree_row() is used unchanged so behavior is consistent in flat view.
In `@src/tui/mod.rs`:
- Around line 583-586: The code currently ignores errors from
std::fs::remove_file(&src) after writing dst (dst_dir.join(format!("{}.jsonl",
session.id))), which can leave duplicates; change the logic in src/tui/mod.rs so
remove_file(&src) is not silently dropped: call
std::fs::remove_file(&src).map_err(|e| format!("failed to remove original
session file {}: {}", src.display(), e))? and, to avoid leaving the copied dst
on failure, attempt to roll back by removing dst
(std::fs::remove_file(&dst).ok()) before returning the error; ensure you
reference the existing dst, src, session.id, and updated variables when
implementing this.
In `@tests/tui_input_test.rs`:
- Around line 279-300: The tests mutate the global CLAUDE_HOME env var causing
flakiness and leaked state; fix by guarding and restoring the env or injecting
the path into App: create an RAII env guard (sets CLAUDE_HOME on creation and
restores previous value in Drop) or modify App::new/discover_sessions to accept
an explicit claude_home path and update tests to pass tmp.path() directly; apply
the same guard/pattern to both the archive_session test (which calls
discover_sessions and App::new then app.archive_session) and the
move_session_moves_file test so cleanup always runs and parallel tests cannot
observe the temp CLAUDE_HOME.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d15b5222-8afa-4643-ab76-235eb19cdccd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlsrc/discovery.rssrc/main.rssrc/search.rssrc/session.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/view.rstests/grouping_test.rstests/tui_input_test.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/session.rs
- src/main.rs
- src/search.rs
| match entry.source { | ||
| DisplaySource::Sessions(sidx) => { | ||
| self.sessions.remove(sidx); | ||
| } | ||
| DisplaySource::Content(cidx) => { | ||
| self.content_results.remove(cidx); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify DisplaySource is not Copy and locate non-Copy moves from &DisplayEntry.
rg -n -C3 '#\[derive\(Debug, Clone\)\]|enum DisplaySource|match entry\.source' src/tui/mod.rsRepository: cc-deck/cc-session
Length of output: 1118
🏁 Script executed:
# Check context around both match expressions to confirm entry binding
rg -n -B5 'match entry\.source' src/tui/mod.rs | head -40Repository: cc-deck/cc-session
Length of output: 522
🏁 Script executed:
# Show more context before the match statements to see entry binding
rg -n -B15 'match entry\.source' src/tui/mod.rs | head -80Repository: cc-deck/cc-session
Length of output: 1380
🏁 Script executed:
# Show function signatures and entry binding - look 30+ lines before the match
rg -n -B30 'match entry\.source' src/tui/mod.rs | grep -A30 'fn ' | head -60Repository: cc-deck/cc-session
Length of output: 1391
🏁 Script executed:
# Show function signature and context for move_session function
sed -n '540,610p' src/tui/mod.rsRepository: cc-deck/cc-session
Length of output: 2872
🏁 Script executed:
# Check if the codebase compiles with these match statements
# First, let's verify the exact type of DisplaySource fields
rg -n -A10 'pub enum DisplaySource' src/tui/mod.rsRepository: cc-deck/cc-session
Length of output: 1887
Clone the display source before matching it.
entry is &DisplayEntry, and DisplaySource derives Clone but not Copy. Matching entry.source attempts to move a field out of a shared reference, which is not allowed. The patterns need to handle references properly or the source must be cloned first.
🛠️ Suggested fix
let entry = &self.display_entries[display_idx];
let session = self.display_session(entry).clone();
+ let source = entry.source.clone();
@@
- match entry.source {
+ match source {
DisplaySource::Sessions(sidx) => {
self.sessions.remove(sidx);
}
DisplaySource::Content(cidx) => {
self.content_results.remove(cidx);
}
}Also applies to: 594-601
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/mod.rs` around lines 498 - 505, The match is attempting to move
`entry.source` out of a `&DisplayEntry`, which is illegal because
`DisplaySource` is Clone but not Copy; fix by cloning the source or matching on
a reference: obtain a cloned value (`let source = entry.source.clone()`) or
match on `&entry.source` and adjust arms to use `sidx`/`cidx` accordingly, then
call `self.sessions.remove(sidx)` and `self.content_results.remove(cidx)`; apply
the same change for the other occurrence handling lines 594-601 as well.
44f0c02 to
5325329
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/search.rs (1)
303-321:⚠️ Potential issue | 🟠 MajorFallback Session uses
cwdforproject_path, inconsistent with the new encoded-dir semantics.After this PR,
Session.project_pathis the Claude-encoded directory name (e.g.-Users-foo-bar, no slashes) everywhere else (discovery.rs,build_session_index,archive_session,move_session,save_custom_title,load_conversation). But this fallback still assignscwd.clone()(an absolute POSIX path with slashes).Any fallback-produced Session flowing into those code paths will produce a wrong path: on Unix
claude_home.join("projects").join("/Users/x/y")resets to/Users/x/y, so archive/move/title-save/load will silently target the wrong location.Derive the encoded dir from
path.parent()?.file_name()here too, the same wayparse_session_filedoes.🛠️ Suggested direction
- let project_name = Path::new(&cwd) + let encoded_dir = path + .parent() + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + .unwrap_or("") + .to_string(); + + let project_name = Path::new(&cwd) .file_name() .and_then(|n| n.to_str()) .unwrap_or("unknown") .to_string(); @@ - project_path: cwd.clone(), + project_path: encoded_dir,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/search.rs` around lines 303 - 321, The fallback Session currently sets project_path to the raw cwd (cwd.clone()), which breaks the new Claude-encoded-dir semantics used elsewhere; change the fallback to compute the encoded project directory the same way parse_session_file does (derive the parent directory's file_name, convert to &str, and use the same encoding/normalization logic to produce the Claude-encoded directory string) and assign that encoded string to Session.project_path instead of cwd.clone(); update variable project_name accordingly if needed so project_name and project_path stay consistent with parse_session_file and other functions like build_session_index/archive_session/move_session/save_custom_title/load_conversation.
♻️ Duplicate comments (2)
tests/tui_input_test.rs (1)
267-301:⚠️ Potential issue | 🟠 MajorProcess-wide
CLAUDE_HOMEmutation races across parallel tests + leaks on panic.Both
archive_session_moves_fileandmove_session_moves_filecallstd::env::set_var("CLAUDE_HOME", …)and laterremove_var. Cargo runs tests within one binary in parallel by default, so these two tests can stomp on each other’s env, and a panic in either test leavesCLAUDE_HOMEset for every subsequent test in the binary.Options, in order of preference:
- Have
discover_sessions,App::new,archive_session,move_session(and anything they transitively call, e.g.get_claude_home) accept/thread an explicitclaude_home: &Pathso the tests never mutate process env.- Short-term, serialize these two tests with
#[serial](e.g.serial_testcrate) and use an RAII guard that removes the var onDrop.Also applies to: lines 392 and 417.
Also applies to: 378-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui_input_test.rs` around lines 267 - 301, The tests mutate the process-wide CLAUDE_HOME causing races — refactor to pass an explicit claude_home path instead: update discover_sessions, App::new, archive_session, move_session (and any helper like get_claude_home) to accept a &Path (or &PathBuf) parameter and thread that path through callsites, then change the tests (archive_session_moves_file and move_session_moves_file) to create a tempdir and pass its path rather than calling std::env::set_var/remove_var; as a short-term alternative you may mark the tests serial and use an RAII guard that removes CLAUDE_HOME on Drop, but prefer the explicit-parameter approach to eliminate global state and the related races.src/tui/mod.rs (1)
576-599:⚠️ Potential issue | 🟠 MajorTwo filesystem-level bugs in
move_session.1. Trailing newline is dropped, corrupting subsequent JSONL appends.
content.lines().…join("\n")strips the terminal\nfrom the original file.std::fs::write(&dst, &updated)then writes a file that doesn’t end with a newline. When anything later appends a JSONL record viawriteln!(for instancesave_custom_titleinsrc/titles.rs— see separate comment), the new record is concatenated onto the previous last line, producing invalid JSONL like{"type":"assistant",…}{"type":"custom-title",…}.2. Ignored
remove_fileerror (duplicate of prior review).If
remove_file(&src)fails, the UI reports success and removes the in-memory entry while the original JSONL remains, producing a duplicate session on the next discovery run.🛡️ Proposed combined fix
- let updated: String = content + let mut updated: String = content .lines() .map(|line| { if let Ok(mut val) = serde_json::from_str::<serde_json::Value>(line) { if let Some(obj) = val.as_object_mut() { if obj.get("cwd").and_then(|v| v.as_str()) == Some(old_cwd) { obj.insert("cwd".to_string(), serde_json::Value::String(target_cwd.to_string())); } } serde_json::to_string(&val).unwrap_or_else(|_| line.to_string()) } else { line.to_string() } }) .collect::<Vec<_>>() .join("\n"); + // Preserve JSONL trailing newline so appends don't fuse onto the last line. + if !updated.is_empty() && !updated.ends_with('\n') { + updated.push('\n'); + } let dst = dst_dir.join(format!("{}.jsonl", session.id)); std::fs::write(&dst, &updated) .map_err(|e| format!("failed to write moved session: {e}"))?; - let _ = std::fs::remove_file(&src); + std::fs::remove_file(&src).map_err(|e| { + let _ = std::fs::remove_file(&dst); + format!("failed to remove original session: {e}") + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 576 - 599, The code that rewrites the session file (reading into content, transforming lines via content.lines().map(...).collect().join("\n") and then std::fs::write(&dst, &updated)) drops a terminal newline and corrupts future JSONL appends, and the final std::fs::remove_file(&src) swallows errors; update move_session so the line-preserving transform preserves a trailing newline (e.g. use an iterator that keeps line endings or detect content.ends_with('\n') and append a '\n' after join) before calling std::fs::write(&dst, …), and do not ignore errors from std::fs::remove_file(&src) — return or log the error (map_err / propagate) so failures to remove the original file are surfaced; refer to the read-to-string, the content.lines()->join("\n") transformation, std::fs::write(&dst, &updated) and std::fs::remove_file(&src) sites when making the change.
🧹 Nitpick comments (3)
src/tui/mod.rs (1)
484-522:archive_session: consider guarding against cross-devicerename.
std::fs::renamefails withEXDEVacross filesystems. If a user has~/.claude/projectson a different mount than~/.claude/projects-archive(unusual but possible — overlays, symlinks to external disks), archive will fail. Arename-then-copy+removefallback makes this resilient without changing the happy path:if let Err(e) = std::fs::rename(&src, &dst) { std::fs::copy(&src, &dst).map_err(|e2| format!("failed to move (rename: {e}, copy: {e2})"))?; std::fs::remove_file(&src).map_err(|e| format!("failed to remove source after copy: {e}"))?; }Low priority — only flagging because the recovery path after a failed archive (partial file at destination + original still present) is unpleasant to clean up manually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/mod.rs` around lines 484 - 522, In archive_session, std::fs::rename(&src, &dst) can fail with EXDEV across filesystems; update the move logic in archive_session to try rename first and on Err fall back to copying then removing the source (i.e., call std::fs::copy(&src,&dst) and if that succeeds call std::fs::remove_file(&src)), returning appropriate Err messages that include both errors if copy fails; keep the existing create_dir_all, label generation, and removal from self.sessions/self.content_results unchanged so the success path and state updates after a successful move remain identical.src/session.rs (1)
25-33: Minor: extract the shared escape/cd prefix to avoid drift betweenresume_commandandfork_command.Both methods duplicate
self.cwd.replace('\'', "'\\''")and thecd '...' && claude -r {id}prefix. A small helper prevents the two commands from subtly diverging later.impl Session { + fn escaped_cwd(&self) -> String { + self.cwd.replace('\'', "'\\''") + } + pub fn resume_command(&self) -> String { - let escaped_cwd = self.cwd.replace('\'', "'\\''"); - format!("cd '{}' && claude -r {}", escaped_cwd, self.id) + format!("cd '{}' && claude -r {}", self.escaped_cwd(), self.id) } pub fn fork_command(&self) -> String { - let escaped_cwd = self.cwd.replace('\'', "'\\''"); - format!("cd '{}' && claude -r {} --fork-session", escaped_cwd, self.id) + format!("cd '{}' && claude -r {} --fork-session", self.escaped_cwd(), self.id) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session.rs` around lines 25 - 33, Extract the shared escaped cwd and command prefix into a small helper used by both resume_command and fork_command: create a private method (e.g., cmd_prefix or cd_and_base_cmd) that returns the "cd '... ' && claude -r {id}" prefix or returns the escaped cwd string, then have resume_command and fork_command call that helper and append their respective suffixes (resume has no extra flag, fork adds "--fork-session"); ensure the helper performs the existing escaping self.cwd.replace('\'', "'\\''") so both functions rely on one implementation to avoid future drift.tests/tui_input_test.rs (1)
379-418: Assertion!content.contains("/Users/test/src-project")is fragile.If the moved JSONL happens to retain any
message.contenttext that mentions the old cwd (e.g. a user prompt quoting the path), this assertion fails even thoughmove_sessionis correct —move_sessiononly rewrites the top-levelcwdfield, not message text. The current fixture avoids this by chance; consider asserting on the parsedcwdof every JSONL line instead:for line in content.lines() { let v: serde_json::Value = serde_json::from_str(line).unwrap(); assert_eq!(v.get("cwd").and_then(|v| v.as_str()), Some(target_cwd)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tui_input_test.rs` around lines 379 - 418, The failing assertion in the test move_session_moves_file is brittle because it scans the entire file text for the old cwd; instead parse each JSONL line and assert the top-level "cwd" field equals the intended target_cwd. Update the test in tests/tui_input_test.rs (move_session_moves_file) to iterate over content.lines(), serde_json::from_str each line into a serde_json::Value, and for each value assert v.get("cwd").and_then(|v| v.as_str()) == Some(target_cwd); keep the other checks (file moved, original deleted) intact and remove the brittle string containment assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/titles.rs`:
- Around line 18-32: The append_custom_title function currently uses writeln!
which will concatenate onto the previous line if the file doesn't end with a
newline; modify append_custom_title so it (a) opens the file with
OpenOptions::new().append(true).create(true) to allow creating missing files,
(b) checks the file's last byte (seek to end, if file length > 0 seek back one
byte and read) and writes a single '\n' before writing the JSON entry only when
the last byte is not '\n', and (c) then write the entry (with writeln! or write!
as appropriate) using the same file handle so the record is always a valid JSONL
line; refer to append_custom_title, the OpenOptions setup, and the writeln!
usage to locate where to implement these changes.
In `@src/tui/mod.rs`:
- Around line 636-668: The uniqueness check in save_title (method save_title)
only inspects in-memory self.sessions and self.content_results, so collisions
with on-disk titles can slip through; update save_title to consult the on-disk
title store before persisting by calling an API in crate::titles (e.g., a new or
existing function like list_all_custom_titles or fetch_all_custom_titles) and
treat any match where id != state.session_id as a duplicate (same behavior as
the current in-memory duplicate branch: restore TitleEditState, set
Mode::TitleEdit, and return Err), or alternatively add a doc comment above
save_title explaining that the check is intentionally UI-scoped if you do not
want global enforcement; ensure you still call crate::titles::save_custom_title
only after the on-disk uniqueness check passes and use the same session_id/title
comparisons as the in-memory check.
---
Outside diff comments:
In `@src/search.rs`:
- Around line 303-321: The fallback Session currently sets project_path to the
raw cwd (cwd.clone()), which breaks the new Claude-encoded-dir semantics used
elsewhere; change the fallback to compute the encoded project directory the same
way parse_session_file does (derive the parent directory's file_name, convert to
&str, and use the same encoding/normalization logic to produce the
Claude-encoded directory string) and assign that encoded string to
Session.project_path instead of cwd.clone(); update variable project_name
accordingly if needed so project_name and project_path stay consistent with
parse_session_file and other functions like
build_session_index/archive_session/move_session/save_custom_title/load_conversation.
---
Duplicate comments:
In `@src/tui/mod.rs`:
- Around line 576-599: The code that rewrites the session file (reading into
content, transforming lines via content.lines().map(...).collect().join("\n")
and then std::fs::write(&dst, &updated)) drops a terminal newline and corrupts
future JSONL appends, and the final std::fs::remove_file(&src) swallows errors;
update move_session so the line-preserving transform preserves a trailing
newline (e.g. use an iterator that keeps line endings or detect
content.ends_with('\n') and append a '\n' after join) before calling
std::fs::write(&dst, …), and do not ignore errors from
std::fs::remove_file(&src) — return or log the error (map_err / propagate) so
failures to remove the original file are surfaced; refer to the read-to-string,
the content.lines()->join("\n") transformation, std::fs::write(&dst, &updated)
and std::fs::remove_file(&src) sites when making the change.
In `@tests/tui_input_test.rs`:
- Around line 267-301: The tests mutate the process-wide CLAUDE_HOME causing
races — refactor to pass an explicit claude_home path instead: update
discover_sessions, App::new, archive_session, move_session (and any helper like
get_claude_home) to accept a &Path (or &PathBuf) parameter and thread that path
through callsites, then change the tests (archive_session_moves_file and
move_session_moves_file) to create a tempdir and pass its path rather than
calling std::env::set_var/remove_var; as a short-term alternative you may mark
the tests serial and use an RAII guard that removes CLAUDE_HOME on Drop, but
prefer the explicit-parameter approach to eliminate global state and the related
races.
---
Nitpick comments:
In `@src/session.rs`:
- Around line 25-33: Extract the shared escaped cwd and command prefix into a
small helper used by both resume_command and fork_command: create a private
method (e.g., cmd_prefix or cd_and_base_cmd) that returns the "cd '... ' &&
claude -r {id}" prefix or returns the escaped cwd string, then have
resume_command and fork_command call that helper and append their respective
suffixes (resume has no extra flag, fork adds "--fork-session"); ensure the
helper performs the existing escaping self.cwd.replace('\'', "'\\''") so both
functions rely on one implementation to avoid future drift.
In `@src/tui/mod.rs`:
- Around line 484-522: In archive_session, std::fs::rename(&src, &dst) can fail
with EXDEV across filesystems; update the move logic in archive_session to try
rename first and on Err fall back to copying then removing the source (i.e.,
call std::fs::copy(&src,&dst) and if that succeeds call
std::fs::remove_file(&src)), returning appropriate Err messages that include
both errors if copy fails; keep the existing create_dir_all, label generation,
and removal from self.sessions/self.content_results unchanged so the success
path and state updates after a successful move remain identical.
In `@tests/tui_input_test.rs`:
- Around line 379-418: The failing assertion in the test move_session_moves_file
is brittle because it scans the entire file text for the old cwd; instead parse
each JSONL line and assert the top-level "cwd" field equals the intended
target_cwd. Update the test in tests/tui_input_test.rs (move_session_moves_file)
to iterate over content.lines(), serde_json::from_str each line into a
serde_json::Value, and for each value assert v.get("cwd").and_then(|v|
v.as_str()) == Some(target_cwd); keep the other checks (file moved, original
deleted) intact and remove the brittle string containment assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d212b3d-92f7-41f0-8014-f22c10b19586
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlsrc/discovery.rssrc/lib.rssrc/main.rssrc/search.rssrc/session.rssrc/titles.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/view.rstests/grouping_test.rstests/tui_input_test.rs
✅ Files skipped from review due to trivial changes (2)
- src/lib.rs
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/grouping_test.rs
- src/tui/view.rs
- src/tui/input.rs
| pub fn save_title(&mut self) -> Result<(), String> { | ||
| let state = self.title_edit.take().ok_or("no title edit in progress")?; | ||
| let title = state.query.trim().to_string(); | ||
| let return_mode = state.return_mode; | ||
|
|
||
| if !title.is_empty() { | ||
| let duplicate = self.sessions.iter() | ||
| .chain(self.content_results.iter()) | ||
| .any(|s| s.id != state.session_id && s.custom_title.as_deref() == Some(&title)); | ||
| if duplicate { | ||
| let cursor = title.len(); | ||
| self.title_edit = Some(TitleEditState { | ||
| session_id: state.session_id, | ||
| query: title, | ||
| cursor, | ||
| return_mode, | ||
| }); | ||
| self.mode = Mode::TitleEdit; | ||
| return Err("title already in use".to_string()); | ||
| } | ||
| } | ||
|
|
||
| let project_path = self.find_session(&state.session_id) | ||
| .map(|s| s.project_path.clone()) | ||
| .ok_or("session not found")?; | ||
|
|
||
| crate::titles::save_custom_title(&project_path, &state.session_id, &title)?; | ||
|
|
||
| self.update_session_title(&state.session_id, if title.is_empty() { None } else { Some(title) }); | ||
|
|
||
| self.mode = return_mode; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
save_title uniqueness check is scoped to loaded sessions only.
self.sessions.iter().chain(self.content_results.iter()) only considers currently loaded sessions, so a title that collides with a session that wasn’t discovered this run (e.g. filtered out earlier by --since/--last, or belonging to an archived session) will pass the check and get persisted. On next launch you’d then have two sessions sharing the same custom title.
If uniqueness is meant to be a global invariant, it needs to be enforced against the on-disk set (scan all JSONL custom-title records in ~/.claude/projects); if it’s only a UI-level collision check for “sessions you can currently see,” the current code is fine but a doc comment would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/mod.rs` around lines 636 - 668, The uniqueness check in save_title
(method save_title) only inspects in-memory self.sessions and
self.content_results, so collisions with on-disk titles can slip through; update
save_title to consult the on-disk title store before persisting by calling an
API in crate::titles (e.g., a new or existing function like
list_all_custom_titles or fetch_all_custom_titles) and treat any match where id
!= state.session_id as a duplicate (same behavior as the current in-memory
duplicate branch: restore TitleEditState, set Mode::TitleEdit, and return Err),
or alternatively add a doc comment above save_title explaining that the check is
intentionally UI-scoped if you do not want global enforcement; ensure you still
call crate::titles::save_custom_title only after the on-disk uniqueness check
passes and use the same session_id/title comparisons as the in-memory check.
There was a problem hiding this comment.
Accepted as a known limitation. Title uniqueness across loaded sessions is sufficient for practical UX. Checking archived or filtered-out sessions would require scanning all JSONL files on every title save, which is not worth the cost.
There was a problem hiding this comment.
@mabulgu, that's a reasonable trade-off — scanning all JSONL files on every title save would be expensive and the in-memory check covers the sessions a user can actually see and act on. Acknowledged!
✏️ Learnings added
Learnt from: mabulgu
Repo: cc-deck/cc-session PR: 2
File: src/tui/mod.rs:0-0
Timestamp: 2026-05-22T10:32:17.631Z
Learning: In `src/tui/mod.rs`, the `save_title` (and `finish_title_edit`) uniqueness check for custom session titles is intentionally scoped to in-memory loaded sessions (`self.sessions` and `self.content_results`) only. It does NOT scan archived or filtered-out sessions on disk. This is an accepted limitation: the check is a UI-level collision guard, not a global invariant, and a full on-disk scan was deemed too costly. Do not flag this as a bug in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
5325329 to
73ed37e
Compare
…mprovements Grouped view: - Collapsible tree view grouping sessions by project (Tab to toggle, --flat flag for ungrouped mode). Projects default to collapsed state. - Keyboard navigation: Left/Right to collapse/expand projects, Enter on project to toggle, End key respects grouped row count. Session archiving: - Press 'a' to archive a session with confirmation prompt (y/Enter to confirm, Esc/any other key to cancel). Archived sessions are moved to a projects-archive/ directory. Move to project: - Press 'm' to open a project picker overlay, select target with arrow keys and Enter. The session JSONL file is moved to the chosen project directory and all cwd fields are rewritten to match the target project. Visual improvements: - Dash prefix on non-selected sessions for better visual separation. - Execute resume command directly via interactive shell on session select. Internal: - Use on-disk directory name as project_path instead of JSONL cwd field, so file location is always the source of truth for project grouping. Tests: - Comprehensive tests for grouping, tree rows, input handling, archive and move file operations (67 tests total).
73ed37e to
a995046
Compare
…tion - Prevent 'a' key from falling through to filter on non-session rows - Enable 'n' (new session) key in flat view mode - Preserve trailing newline in move_session file rewrite - Propagate remove_file errors instead of silently ignoring - Add defensive session ID validation before index-based removal
|
@rhuss would you be interested in these improvements? |
There was a problem hiding this comment.
Pull request overview
This PR expands the cc-session TUI with project-grouped (tree) browsing and several session management workflows (archive, move-to-project, fork, and custom titles), plus corresponding CLI/view updates and new automated tests.
Changes:
- Add grouped-by-project tree view with expand/collapse navigation and
--flatto start in ungrouped mode. - Add session actions: archive with confirmation, move-to-project picker, fork session, and title edit/new-session title prompt with persistence to JSONL.
- Add tests covering grouping/tree rows and key handling, plus archive/move filesystem operations.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/tui_input_test.rs |
Adds input-handling tests for grouped navigation, archive confirmation flow, and move picker behavior, plus archive/move file operation tests. |
tests/grouping_test.rs |
Adds unit tests for project grouping and tree-row construction behavior (sorting, collapsed defaults, filtering). |
src/tui/view.rs |
Splits session list rendering into flat vs grouped renderers, adds move picker overlay, and adds title-edit UI/status updates. |
src/tui/mod.rs |
Introduces new modes/actions and state for grouping, archiving, moving, and title editing; executes resume/new/fork commands via shell. |
src/tui/input.rs |
Implements keybindings for grouped view toggling/navigation plus archive/move/title flows and confirmation handling. |
src/titles.rs |
Adds persistence of custom-title JSONL entries for Claude-compatible session naming. |
src/session.rs |
Adds custom_title field and includes -n in resume command when titled; adds fork command helper. |
src/search.rs |
Updates session index path handling to use encoded project directories; adds custom_title initialization for fallback parsing. |
src/discovery.rs |
Changes project derivation to use filesystem probing of encoded directory names and parses custom-title entries. |
src/main.rs |
Adds --flat flag and passes initial grouped/flat mode into the TUI runner. |
src/lib.rs |
Exposes the new titles module. |
Cargo.toml / Cargo.lock |
Adds tempfile dev dependency for new filesystem-based tests. |
Comments suppressed due to low confidence (1)
src/discovery.rs:120
parse_session_filenow iteratesfor line in reader.lines()over the entire JSONL, even after the first message metadata has been extracted, because laterline_count > 50justcontinues rather than terminating the loop. For large sessions this can significantly slow discovery; consider keeping the fast-path (stop after N lines for metadata/first message) and separately scanning only the tail for the most recentcustom-titleentry (or break once both message metadata and the latest title have been found).
for line in reader.lines() {
let line = match line {
Ok(l) => l,
Err(_) => continue,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Some(Session { | ||
| id: session_id, | ||
| project_path: cwd.clone(), | ||
| project_name, | ||
| git_branch: entry.git_branch, | ||
| timestamp, | ||
| first_message, | ||
| cwd, | ||
| project_exists, | ||
| custom_title: None, | ||
| }) |
| KeyCode::Backspace => { | ||
| if let Some(state) = &mut app.title_edit { | ||
| if state.cursor > 0 { | ||
| state.query.remove(state.cursor - 1); | ||
| state.cursor -= 1; | ||
| } | ||
| } | ||
| Action::Continue | ||
| } | ||
| KeyCode::Left => { | ||
| if let Some(state) = &mut app.title_edit { | ||
| state.cursor = state.cursor.saturating_sub(1); | ||
| } | ||
| Action::Continue | ||
| } | ||
| KeyCode::Right => { | ||
| if let Some(state) = &mut app.title_edit { | ||
| if state.cursor < state.query.len() { | ||
| state.cursor += 1; | ||
| } | ||
| } | ||
| Action::Continue | ||
| } | ||
| KeyCode::Char(c) => { | ||
| if let Some(state) = &mut app.title_edit { | ||
| state.query.insert(state.cursor, c); | ||
| state.cursor += 1; | ||
| } | ||
| Action::Continue |
| let cursor_pos = state.cursor.min(state.query.len()); | ||
| let (before, rest) = state.query.split_at(cursor_pos); | ||
| if !before.is_empty() { | ||
| spans.push(Span::styled(before.to_string(), Style::default().fg(Color::White))); | ||
| } | ||
| if let Some(ch) = rest.chars().next() { | ||
| spans.push(Span::styled( | ||
| ch.to_string(), | ||
| Style::default().fg(Color::Black).bg(Color::White), | ||
| )); | ||
| let after = &rest[ch.len_utf8()..]; | ||
| if !after.is_empty() { | ||
| spans.push(Span::styled(after.to_string(), Style::default().fg(Color::White))); | ||
| } |
| (" ", 2) | ||
| }; | ||
|
|
||
| let label = app.session_display_label(session); | ||
| let max_msg_len = width.saturating_sub(cursor_len + right_len + 2); | ||
| let msg = truncate_str(&session.first_message, max_msg_len); | ||
| let msg = truncate_str(&label, max_msg_len); | ||
| let msg_len = msg.chars().count(); | ||
| let pad = width.saturating_sub(cursor_len + msg_len + right_len); | ||
| let padding = " ".repeat(pad); |
| pub fn resume_command(&self) -> String { | ||
| // Single-quote the path, escaping any embedded single quotes | ||
| let escaped_cwd = self.cwd.replace('\'', "'\\''"); | ||
| format!("cd '{}' && claude -r {}", escaped_cwd, self.id) | ||
| match &self.custom_title { | ||
| Some(title) => { | ||
| let escaped_title = title.replace('\'', "'\\''"); | ||
| format!("cd '{}' && claude -r {} -n '{}'", escaped_cwd, self.id, escaped_title) | ||
| } | ||
| None => format!("cd '{}' && claude -r {}", escaped_cwd, self.id), | ||
| } |
|
Preparing review... |
|
Thanks for this contribution, @mabulgu! I appreciate the effort that went into this, it's a substantial piece of work. I'm definitely open to contributions like this. Let me share some feedback after reviewing the code in detail. Overall impressionThe grouped tree view is a nice addition, and the archive/move features address real workflows. The test coverage is solid (580 lines of new tests), and the tree navigation (Left/Right to collapse/expand, Enter to toggle) feels natural. UX: single-key shortcuts conflict with seamless searchcc-session's core interaction model is "just start typing to search." There's no mode switch, no prefix required. This is a deliberate design choice (see the project notes on seamless search). The PR adds single-key shortcuts ( A few related issues:
These features would work better with a modifier key (e.g., Ctrl-a, Ctrl-m) or a command palette approach that doesn't conflict with the seamless search. Behavioral changes to flag
Clipboard copy replaced with direct shell exec: The current behavior copies the resume command to clipboard. The PR removes clipboard entirely and spawns an interactive shell that runs the command directly, then calls Full file scan on startup: The old code reads only the first 50 lines per session file. The PR iterates every line to find Concerns
Dead code: The Custom titles: proposal for a different approachWhether Beyond the dual-write concern, scanning every line of every session file on startup just to find title entries appended at the end has a real performance cost. cc-session currently reads only the first 50 lines per file, which is key to keeping startup fast across thousands of sessions. I'd propose a different approach: store titles in a separate cc-session-owned index file (e.g., a simple JSON map of session ID to title). This would:
Titled sessions could then be rendered with a distinct color or marker in the list, making them easy to spot visually as "sessions I care about." There's also a functional gap: SummaryThe grouped view, archiving, move, and fork features are great additions. I'd suggest:
Happy to discuss any of this further. Thanks again for the work! |
|
One more thing I noticed: when searching in the grouped/hierarchical view, matching sessions stay hidden inside their collapsed project groups. The tree should auto-expand groups that contain matches so you can immediately see the hits in context, rather than having to manually expand each project to find where the results are. |
|
FYI: The direct-exec-on-Enter behavior from this PR has been extracted and merged separately in #4. Enter now launches |
…ling - Make cursor navigation UTF-8 safe in title edit and conversation search by tracking byte offsets via char_indices instead of +/- 1 - Fix search_file_with_metadata deriving project_path from filesystem path (encoded dir) instead of raw cwd, matching discover_sessions - Guard append_custom_title against missing trailing newline in JSONL - Skip -n flag for empty custom_title in resume_command - Use chars().count() for right-side label width in session list
Show a title edit bar pre-filled with "{title} (fork)" when pressing f
in conversation view, so forked sessions are distinguishable in the list.
Empty title forks without -n flag, Esc cancels the fork entirely.
- Include custom_title in filter search haystack so named sessions are discoverable by their title - Remove dead clipboard module and arboard dependency - Auto-expand all project groups when a filter is active so matching sessions inside collapsed groups are always visible
|
Pushed two commits addressing the review feedback: fb42a43 fixes the CodeRabbit/Copilot findings: UTF-8 safe cursor navigation (no more panics with non-ASCII input like Turkish chars), a4109bf addresses @rhuss's items that were straightforward to apply: The earlier commits already covered the |
|
A few items from @rhuss's review need some discussion before jumping into implementation: Shortcut keys vs seamless search: The single-key shortcuts ( Archive/move from conversation view: Right now these only work in the list view without a filter. The "search, open, then archive" workflow doesn't work yet. Happy to add this once we settle on the keybinding approach above. Title storage: Currently titles are appended to Claude Code's session JSONL files. @rhuss suggests a separate cc-session-owned index file for O(1) lookup. I lean toward keeping the current approach since it avoids sync issues (titles travel with their sessions through moves/archives), but open to the index if startup perf becomes a real problem.
@rhuss, items 1 and 3 especially touch the core interaction model, so your input would be great. |
|
Hey @mabulgu , thanks for coming back! In the meantime I already moved on a bit and added the (a) the exec feature (greate idea!) and (b) also the group mode, albeit a bit differently. It uses an ALT-G shortcut ("g" for group). For the title I'm not yet convinced that it justifies the complexity. What I wanted to go next is that the session text shown will be the context around the search term when entered, so that you have some context instead of only the first line of the conversation. I think that already helps to identify the right sessions. Sorry for now, but let's think later how we could add a 'favourite' system with titles. |
Rework single-key action shortcuts so they no longer conflict with cc-session's "just start typing to search" model (rhuss review): - Archive/title/new/move now use Ctrl shortcuts (Ctrl+a, Ctrl+t, Ctrl+n, Ctrl+v). Move is Ctrl+v because Ctrl+m is indistinguishable from Enter. Bare letters always feed the search filter again, so queries starting with a/t/n/m (e.g. "table") work. - Ctrl shortcuts now fire whether or not a filter is active, so you can search first and then act on a result. - Update status bar hints to the ^a/^t/^n/^v form. Make the CLAUDE_HOME-mutating tests serial to avoid a data race with rayon's parallel discovery (rhuss review): add serial_test and mark archive_session_moves_file and move_session_moves_file with #[serial].
This PR adds the following improvements:
Assisted-by: Claude Opus 4.6