feat(shell): /sessions — browse the session tree and switch - #419
Conversation
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the ChangesSession Navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AishShell
participant SessionStore
participant SearchSelectPanel
AishShell->>SessionStore: load roots and child sessions
SessionStore-->>AishShell: return session tree
AishShell->>SearchSelectPanel: show selectable tree
SearchSelectPanel-->>AishShell: return selected session
AishShell->>SessionStore: persist and resume session
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/aish-session/src/store.rs (1)
303-312: 🚀 Performance & Scalability | 🔵 TrivialConsider an index on
parent_session_uuidif session counts grow.
list_childrenis called once per node while building the/sessionsforest, and each call full-scanssessions. An index keeps the tree walk linear-ish as history accumulates.CREATE INDEX IF NOT EXISTS idx_sessions_parent ON sessions(parent_session_uuid);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-session/src/store.rs` around lines 303 - 312, Add an index on sessions.parent_session_uuid during database initialization or migration, using the established schema setup path and the name idx_sessions_parent. Keep list_children unchanged so its parent_session_uuid filter can use the index.crates/aish-shell/src/app.rs (1)
3542-3555: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the recursion depth.
Acyclicity is only an invariant of the
/forkpath —fork_sessiondoesn't rejectnew_uuid == parent_uuid, andsessions.dbis a user-writable file. A self-parent or cycle row makes this recurse until the stack overflows, killing the shell. A depth cap keeps it a display artifact instead.🛡️ Cap the walk depth
fn collect_session_tree( &self, store: &aish_session::SessionStore, node: &aish_session::SessionRecord, depth: usize, items: &mut Vec<aish_ui::SearchSelectItem>, ) { items.push(self.session_tree_item(node, depth)); + // Defensive: a malformed/hand-edited sessions.db could contain a + // parent cycle, which would otherwise recurse until stack overflow. + if depth >= MAX_SESSION_TREE_DEPTH { + return; + } if let Ok(children) = store.list_children(&node.session_uuid) {Plus a module-level constant:
/// Maximum nesting rendered by `/sessions`. const MAX_SESSION_TREE_DEPTH: usize = 32;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aish-shell/src/app.rs` around lines 3542 - 3555, Bound recursion in collect_session_tree using a module-level MAX_SESSION_TREE_DEPTH constant set to 32. Stop descending once the current depth reaches the cap, while still rendering the current node and preserving normal child traversal below the limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aish-session/src/store.rs`:
- Around line 598-625: Update add_column_if_missing to derive the target column
name and query PRAGMA table_info(sessions) before executing ALTER TABLE,
returning success when the column is already present. Preserve the
duplicate-column error fallback only as a documented concurrent-migration race
guard for this ADD COLUMN operation, while propagating unrelated SQLite errors
unchanged through the existing AishError::Session path; keep migrate_schema’s
two-column migration calls intact.
In `@crates/aish-shell/src/app.rs`:
- Around line 3404-3434: Update the manual-recovery hint in the
resume_session_with_options failure branch to pass the full new_uuid to the
shell.fork.switch_manual translation arguments instead of new_short, while
keeping the existing shortened identifier for the success message.
---
Nitpick comments:
In `@crates/aish-session/src/store.rs`:
- Around line 303-312: Add an index on sessions.parent_session_uuid during
database initialization or migration, using the established schema setup path
and the name idx_sessions_parent. Keep list_children unchanged so its
parent_session_uuid filter can use the index.
In `@crates/aish-shell/src/app.rs`:
- Around line 3542-3555: Bound recursion in collect_session_tree using a
module-level MAX_SESSION_TREE_DEPTH constant set to 32. Stop descending once the
current depth reaches the cap, while still rendering the current node and
preserving normal child traversal below the limit.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f6eb1938-fb6a-4c0e-9739-08c5c50c8fa0
📒 Files selected for processing (11)
crates/aish-i18n/locales/de-DE.yamlcrates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/es-ES.yamlcrates/aish-i18n/locales/fr-FR.yamlcrates/aish-i18n/locales/ja-JP.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-session/src/models.rscrates/aish-session/src/store.rscrates/aish-shell/src/app.rscrates/aish-shell/src/readline.rscrates/aish-shell/tests/slash_popup_commands.rs
fe89d91 to
4ba0ffa
Compare
fe4b7d8 to
38ae2b4
Compare
`/sessions` opens an interactive panel showing the session forest (roots + all descendants, indented by depth) and switches to the chosen session on submit — switching persists the current state first so no cwd is lost. - Depth-first descent (collect_session_tree) so nested forks stay visible, not just roots + direct children. - Switch uses persist_current=true (like /resume) to avoid losing cwd. - Reuses the existing SearchSelectPanel + resume path; no new UI primitives. Depends on the /fork data layer (session_roots / list_children / parent_session_uuid) — stacked on the /fork PR. Closes AI-Shell-Team#412
38ae2b4 to
412d4f5
Compare
Summary
Adds the
/sessionsbuilt-in command: an interactive panel that browses the session tree (roots + all descendants, indented by depth) and switches to the chosen session on submit.Closes #412
What it does
collect_session_tree) so nested forks stay visible, not just roots + direct children.persist_current=true, like/resume) so no cwd is lost.Stacked on #418 (
/fork)This builds on the session data layer introduced by the
/forkPR —session_roots/list_children/parent_session_uuid. Merge #418 first. The diff below includes that PR's commit until it lands.Scope
Reuses the existing
SearchSelectPanel+resume_session_with_options; no new UI primitives. Self-contained to the/sessionscommand + i18n.Verification
cargo clippy -p aish-shell --all-targets -- -D warningsclean.aish-sessiontests pass (9);slash_popup_commandstests pass (count + i18n).Summary by CodeRabbit
/sessionscommand to browse the session tree, including roots and forks.