Conversation
marcus
left a comment
There was a problem hiding this comment.
Code Review: Shells tracked in kanban
Good PR — the architectural direction makes sense and the implementation is clean. The kanbanData struct as a computed snapshot is a solid pattern. A few things to address:
🚨 Must-fix: Accidental result symlink
The diff includes:
diff --git a/result b/result
new file mode 120000
--- /dev/null
+++ b/result
@@ -0,0 +1 @@
+/nix/store/sw4mkz9fjpirdvfk95m0qf7y6rhf320n-sidecar-v0.72.0-dev+dirty
This is a Nix build artifact (nix build output symlink). It must be removed from this PR — it references a path on your local machine and will be broken for everyone else. Add result to .gitignore if it isn't already, and drop it from this branch.
Architecture ✅
The kanbanData struct approach is right: compute column assignments once per render/update cycle rather than calling buildKanbanData() multiple times. However, I see buildKanbanData() is currently called multiple times per operation (e.g., moveKanbanColumn builds it, moveKanbanRow builds it again, syncKanbanToList builds it again). This is fine for correctness but redundant work. Worth a follow-up to thread kd through the call stack where operations are batched, though not a blocker.
Shell-to-column Mapping
The status mapping in kanbanShellStatus:
case AgentStatusError:
return StatusPaused, trueRouting error shells to StatusPaused is consistent with how worktrees handle StatusError (also mapped to StatusPaused since both need user intervention). Good consistency.
The ordering within columns — worktrees first, then agent-shells — is a reasonable choice, but it means a shell in the Active column will always appear below all worktrees regardless of recency. Is this intentional? If agent-shells should be interleaved by last-activity time, that's a separate feature, but worth calling out as a known limitation.
syncKanbanToList Fix
The old code did p.selectedShellIdx = p.kanbanRow which assumed kanban row directly mapped to shell slice index. The new code correctly searches by TmuxName:
for i, s := range p.shells {
if s.TmuxName == shell.TmuxName {This is the right fix, and the test updates (TmuxName: "shell-1") confirm the intent. ✓
syncListToKanban
The new implementation correctly handles agent-shells that have been routed to status columns:
for colIdx, status := range kanbanColumnOrder {
wtCount := len(kd.worktrees[status])
for i, s := range kd.shells[status] {
if s.TmuxName == shell.TmuxName {
p.kanbanCol = colIdx + 1
p.kanbanRow = wtCount + iThe wtCount + i offset is correct since worktrees come first in each column. ✓
renderKanbanShellCardLine Improvements
The new status text logic is much more informative — showing claude · waiting, claude · done, claude · error etc. is a clear UX improvement over just shell · running. The orphaned shell case (◌ icon, · offline) is a nice touch.
One nit: the orphaned check runs before the agent-chosen check, which means an orphaned shell with an agent will show ◌ · <agent> · offline but reach the first branch. Trace through:
if shell.IsOrphaned {
statusIcon = "◌"
} else if shell.ChosenAgent != AgentNone && shell.ChosenAgent != "" {
if shell.Agent != nil { ... }
} else if shell.Agent != nil {
statusIcon = "●"
}For line 1 (status text), the orphaned branch correctly shows · offline using ChosenAgent. The logic is correct, just requires careful reading. A comment on the case-1 branch would help the next reader.
view_list.go
The agent status label update (running → active/waiting/done/error) is consistent with the kanban card changes. Good to keep the list view and kanban in sync. ✓
Minor
The comment removal churn (removing // Highlight selected column header, // Build content for panel, // Apply styling, etc.) is fine — most were redundant. But a few (like // td-693fc7 references) are tracking IDs that provide useful context; keep those.
The renamed variable i → j in the empty-cells loop is correct (avoiding shadowing the outer loop variable). ✓
Summary
- Remove the
resultsymlink — blocker, must fix before merge - Multiple
buildKanbanData()calls per operation — minor, follow-up - Worktrees-before-shells ordering within columns — intentional? If so, document it
Everything else looks solid. The kanban integration is architecturally clean and the shell routing logic handles the edge cases (orphaned, no agent, agent with various statuses) correctly.
|
Hey @boozedog! This is a great first PR — you identified the gap, filed the issue, then went ahead and implemented it. That's exactly the kind of contribution we love. 🙌 Review notes: ✅ The struct is a clean abstraction — pre-computing column assignments before rendering is the right move. ✅ ✅ The status label improvements in One thing to address: There's a (A Once that's cleaned up, this is looking solid. ✦ |
(Note: please feel free to reject if this isn't consistent with your vision for how the kanban board is supposed to work!)
As mentioned in #176 I was thinking it'd be nice if shells that are started with an associated agent appear in the kanban board under the relevant status column to make it easier to see what they are doing. Useful for doing work on the "main" worktree. (Which maybe I shouldn't be doing, LOL!)
Screenshot of feature: