feat(omp): browse oh-my-pi sub-agent transcripts, folded under their parent#27
Conversation
…yer) oh-my-pi writes each `task`/`job` sub-agent's full conversation to its own jsonl in a sibling directory named after the parent file's stem (a single parent can spawn hundreds). The omp provider only scanned top-level `*.jsonl` files, so sub-agent content was completely invisible. - SessionMeta gains `parent_id: Option<String>` (sub-agent → parent linkage) and `child_count: usize` (cheap directory-listing count, no file reads). - omp::summarize counts a parent's children at discovery; omp::list_children lazily parses them on demand (so startup never reads the hundreds of child files — they load only when the user expands a parent). - Sub-agent SessionMetas get a unique cache-safe id (`omp:sub:<parent>:<stem>`), inherit the parent's cwd, and use the filename slug as a readable label. - All other providers set the two new fields to None / 0. Top-level discovery still returns only parents, so grouping is unchanged. TUI fold/expand rendering comes next. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSessionMeta gains parent/child relationship tracking. All providers initialize the new fields. OMP adds child-transcript discovery via a new public function. The TUI extends list rendering to show lazy-loaded sub-agent rows with per-parent folding, chevron markers, and indentation, plus CLI diagnostics for sub-agent relationships. ChangesProvider and Session Schema Updates
TUI Sub-agent UI Implementation
Diagnostic Output
Sequence DiagramsequenceDiagram
participant User as User Selection
participant TUI as tui.rs
participant SessionMod as session.rs
participant OMP as providers/omp.rs
participant Render as List Renderer
User->>TUI: toggle_expand_at_selection (Solo/Child)
TUI->>TUI: toggle_subagents(parent_sid)
TUI->>SessionMod: list_children(parent_meta)
SessionMod->>OMP: list_children(parent_meta)
OMP->>OMP: discover sibling JSONL files
OMP-->>SessionMod: Vec<SessionMeta> (children)
SessionMod-->>TUI: children
TUI->>TUI: cache children, update expanded_parents
TUI->>TUI: list_rows()
TUI->>TUI: push_subagent_rows(parent, expanded)
TUI->>Render: SubAgent rows with Indent::SubAgent
Render->>Render: list_item_for_session(Indent::SubAgent)
Render-->>User: display with child chevrons and counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Sub-agents are shown as a second indentation tier under the session that spawned them, collapsed by default. The existing group-fold machinery is reused: a new `ListRow::SubAgent` variant, an `expanded_parents` set, and a lazily-populated `children` map keyed by parent id. - A parent that spawned sub-agents shows a `▶N⤷` / `▼N⤷` fold marker with the child count; Space (or Enter on a group header) toggles it. - Expanding a parent lazily calls `session::list_children` once and caches the result, so the hundreds of child files are never read at startup. - `meta_by_sid` resolves a selected sub-agent to its meta (searching the children cache), so preview/detail render the sub-agent transcript via the normal read_transcript path — no special-casing downstream. - Auto-refresh clears the children cache and fold state (child_count/parentage may have shifted). - `--dry-run` gains an omp sub-agent diagnostic (host count, total children, and a live list_children probe) — used to smoke-test against real data. Verified end-to-end against a real ~/.omp sample: parent indexed at top level, child_count=2 counted, list_children loaded both with correct slug-based ids. Co-Authored-By: Claude <noreply@anthropic.com>
803d4f8 to
c525951
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tui/src/tui.rs (1)
1417-1420: ⚖️ Poor tradeoffConsider refactoring to avoid cloning the groups vector.
Cloning
self.groups().to_vec()to work around the borrow checker copies potentially hundreds ofSessionGroupinstances on every list render. The clone is necessary becausegroups()takes&mut self(to populate the cache), and thenpush_subagent_rows()needs&selfto accesschildren.Possible alternatives:
- Cache the groups vector at a higher level to avoid the &mut self borrow in this path
- Restructure to build rows without needing simultaneous access to groups and children
- Split groups() into separate "get cached" and "compute if needed" methods
This is a minor performance concern—the code is correct—but worth addressing if rendering performance becomes noticeable.
🤖 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 `@tui/src/tui.rs` around lines 1417 - 1420, The code clones self.groups().to_vec() to avoid a mutable borrow while later calling push_subagent_rows which only needs an immutable view, causing expensive copies of SessionGroup; change the API so you can access groups without requiring &mut self here — e.g., add a groups_cached(&self) -> &[SessionGroup] accessor (or split groups() into cached_get() and compute_if_missing(&mut self) variants) or compute/populate the cache earlier so this path can call groups_cached() and then call push_subagent_rows(&self, groups_slice, &expanded_parents) to avoid cloning; update references to expanded_parents, groups(), push_subagent_rows(), SessionGroup, and children accordingly.
🤖 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 `@tui/src/tui.rs`:
- Around line 1492-1494: The synchronous call to session::list_children(&parent)
on the UI thread blocks the TUI for large parents; replace it with an
asynchronous load: when a parent is expanded set a loading state in the node (so
the UI shows a spinner), then spawn a background thread/task (analogous to the
existing cache-warming background work) to call session::list_children(parent)
off the UI thread, send the resulting Vec back to the UI via a channel, and on
the main thread update self.children.insert(sid.to_string(), kids) and clear the
loading state and trigger a re-render when the channel message arrives; remove
the direct synchronous call from the expand handler so the UI never blocks.
---
Nitpick comments:
In `@tui/src/tui.rs`:
- Around line 1417-1420: The code clones self.groups().to_vec() to avoid a
mutable borrow while later calling push_subagent_rows which only needs an
immutable view, causing expensive copies of SessionGroup; change the API so you
can access groups without requiring &mut self here — e.g., add a
groups_cached(&self) -> &[SessionGroup] accessor (or split groups() into
cached_get() and compute_if_missing(&mut self) variants) or compute/populate the
cache earlier so this path can call groups_cached() and then call
push_subagent_rows(&self, groups_slice, &expanded_parents) to avoid cloning;
update references to expanded_parents, groups(), push_subagent_rows(),
SessionGroup, and children 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 Plus
Run ID: 14e9708f-7078-4696-bad2-6329509eff62
📒 Files selected for processing (8)
core/src/providers/claude.rscore/src/providers/codex.rscore/src/providers/hermes.rscore/src/providers/omp.rscore/src/providers/qwen.rscore/src/session.rstui/src/main.rstui/src/tui.rs
| if !self.children.contains_key(sid) { | ||
| let kids = session::list_children(&parent); | ||
| self.children.insert(sid.to_string(), kids); |
There was a problem hiding this comment.
Synchronous I/O on the UI thread will freeze the TUI when expanding large parents.
session::list_children(&parent) reads and parses child transcript files from disk synchronously. According to the PR objectives, real-world parents can spawn 900+ children. Reading and summarizing that many jsonl files on the main thread will block the UI for seconds, leaving the user unable to interact with the TUI.
The lazy-loading strategy (defer until expansion) is good for startup performance, but the implementation should be asynchronous:
- Show a loading indicator when the user expands a parent
- Spawn the load in a background thread (similar to the existing cache warming at line 555-563)
- Send the results back via a channel
- Update
self.childrenand re-render when results arrive
Without async loading, expanding large parents creates a poor user experience.
🤖 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 `@tui/src/tui.rs` around lines 1492 - 1494, The synchronous call to
session::list_children(&parent) on the UI thread blocks the TUI for large
parents; replace it with an asynchronous load: when a parent is expanded set a
loading state in the node (so the UI shows a spinner), then spawn a background
thread/task (analogous to the existing cache-warming background work) to call
session::list_children(parent) off the UI thread, send the resulting Vec back to
the UI via a channel, and on the main thread update
self.children.insert(sid.to_string(), kids) and clear the loading state and
trigger a re-render when the channel message arrives; remove the direct
synchronous call from the expand handler so the UI never blocks.
apply_refreshed_sessions cleared `expanded_parents` and the `children` cache on every refresh, so each 30s auto-refresh tick collapsed any open sub-agent fold and reset the detail pane back to the parent — making sub-agents look "merged into one / impossible to view separately" while idly reading. Keep folds open across a refresh; only drop cache/fold entries for parents that vanished. We deliberately do not re-list children of still-expanded parents here, so a parent with hundreds of children never re-scans its directory on a tick (new children surface on the next manual collapse+expand). Verified with a tmux-driven real-data run: expand a 6-child host, select a sub-agent, cross several auto-refresh ticks — fold stays open, selection and detail stay put. Also extend the `--dry-run` omp probe to report uniq_ids / uniq_paths and the first few child id/file/prompt, which is how the id/path uniqueness was confirmed. Co-Authored-By: Claude <noreply@anthropic.com>
Update: fixed a fold-collapse-on-refresh bug + real-data validationAdded Bug: Validated on dev against the real 918-child session (tmux-driven, fresh binary built in place):
Also verified the smaller 6-child host end-to-end the same way. |
1 participant
Problem
oh-my-pi (omp) writes each
task/jobsub-agent's full conversation to its own jsonl in a sibling directory named after the parent file's stem — a single parent can spawn hundreds (one real session here has 918). The omp provider only scanned top-level*.jsonlfiles and never entered that directory, so the parent'staskresult was just[Output truncated - N tokens]and the actual sub-agent content was completely unreadable.What this does
Sub-agents are now browsable, shown as a second indentation tier folded under the session that spawned them (collapsed by default), reusing the existing group-fold machinery.
Data layer (
core)SessionMetagainsparent_id: Option<String>andchild_count: usize.omp::summarizecounts a parent's children at discovery via a directory listing only (no file reads), so startup cost is unchanged.omp::list_childrenparses the children lazily, only when a parent is expanded — the hundreds of child files are never read at startup.omp:sub:<parent>:<stem>), inherit the parent's cwd, and use the filename slug as a readable label.TUI (
tui)ListRow::SubAgenttier, anexpanded_parentsset, and a lazily-populatedchildrenmap.▶N⤷/▼N⤷fold marker; Space toggles it.meta_by_sidresolves a selected sub-agent to its meta, so preview/detail render its transcript through the normalread_transcriptpath — no downstream special-casing.--dry-rungains an omp sub-agent diagnostic (host count, total children, livelist_childrenprobe).Verification
cargo test(workspace) — green, incl. a newchild_sub_agents_are_discovered_and_linkedtest.~/.omp, 816 sessions): top level indexes parents only;omp sub-agent hosts: 5 (total children=1016); probing the largest parent →child_count=918 loaded=918with correct slug-based ids, no crash, startup still fast.Interactive TUI fold/expand was validated on dev by building locally there (dev runs Ubuntu 20.04 / glibc 2.31, which the GitHub
linux-gnurelease tarball is too new for — a separate musl-target follow-up would let the prebuilt binary run there too).🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Release Notes
New Features
Tests