Conversation
There was a problem hiding this comment.
Code Review - Parallel Sessions & Tabs
Found 3 bugs in the added code that need attention:
Summary
- 🔴 2 HIGH severity bugs that could cause state leaks and data races
- 🟡 1 MEDIUM severity bug with double cleanup execution
All issues are in newly added code. Please review the inline comments for details and suggested fixes.
3aed7d5 to
2fa7f89
Compare
|
/review |
There was a problem hiding this comment.
Code Review Summary
I've analyzed this PR which adds parallel session support with tabs to the TUI. The implementation looks solid with good attention to concurrency and resource management.
Key Areas Reviewed
✅ Resource cleanup patterns: The supervisor uses goroutines for cleanup operations outside locks to prevent deadlocks - this is the correct pattern.
✅ SQL migration: Migration 017 adds split_diff_view BOOLEAN without a DEFAULT, but the code properly handles NULL via *bool pointer type and sql.NullBool.
✅ Context management: Proper use of context.WithoutCancel in cleanup functions to allow cleanup to complete even after parent context cancellation.
✅ Session spawner: Creates new runtimes per session with appropriate cleanup handlers.
✅ Tab bar click handling: Includes proper bounds checking before array access.
Architecture Notes
The supervisor pattern is well-designed:
- Uses mutex for state protection
- Runs cleanup in goroutines outside locks to avoid deadlocks
- Proper context cancellation for session lifecycle
- Event routing through RoutedMsg wrapper
No bugs found in the changed code. This is a substantial refactoring that maintains code quality.
|
@krissetto seems to work for me. Some feedback:
|
|
1bbb254 to
fc0d482
Compare
|
@dgageot addressed all your feedback (well, maybe the code could be even simpler, but i think we can iterate lol). Give it a try 😉 |
a3dec32 to
ecf92d2
Compare
|
Something confusing: Ctrl+p was used to open the commands panel and now it's to navigate to the next tab. It's now Ctrl+k (not displayed in the bar) and not related to your change but I also found another issue related to keys mapping: #1741 Also when we have several tabs and in the chat we use UP key to navigate to the previous message I am seeing the last one in any tab. Not sure if it would make sense to isolate them? I agree that classical shells have the same issue thus it's probably ok |
ecf92d2 to
ed1530f
Compare
That was intentional, mostly to allow users to switch tabs via keyboard with somewhat reasonable default bindings (p -> previous, n -> next). I can add the help text (that bottom bar is also getting crowded, maybe we could have something like F1 etc to open a dialog showing all combos instead?) Key mapping is becoming a bit of a mess as there are many bindings, and many quite specific user preferences that we'll likely never be able to fully satisfy with defaults. IMHO the move here should be to introduce user configurable mapping settings, and have a set of defaults that make sense for new users and the majority let's say.. Possibly with a couple profiles? (e.g. vim/emacs styles?) I wouldn't add all that work to this PR, but I'm definitely open to suggestions here regarding these specific changes. |
Users may have preferences around that too, mostly because I'm also unsure as to what my expectations would be.. but it's definitely something worth exploring |
The main issue is that Ctrl+p which was visible in the menu is now hidden (Ctrl+k). But you cannot indefinitely add new menu entries. New choices are making sense and should be highlighted as breaking change to avoid confusion. Agree with you that in an ideal world we would love to have everything configurable (using profiles probably) and for sure finding "acceptable" defaults is hard |
ed1530f to
6b94bf1
Compare
Changed that so its visible again with the new binding, and updated the PR description. I'll make sure to check that the generated changelog also includes that info if we merge this |
6b94bf1 to
a3dd447
Compare
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
a3dd447 to
82f6bcf
Compare
This is a big boi.
Scary, I know.
The goal is simple, bring the TUI forward a notch and allow users to do more at the same time without needing to juggle multiple terminals/cagent instances 😄
ctrl+p/nmove between tabs;ctrl+kctrl+tcreates a tab,ctrl+wcloses the current tab;/split-diffcommand/split-diffdefaults to true, persists on the session if changed, can be configured to always false in the user config;Please test if out and let me know of any issues, especially major ones.
Minor ux issues found can be iterated on after merging, to not over-complicate this already big PR
Screencast
Screencast.From.2026-02-08.16-37-05.mp4
Replaces #1628