Skip to content

feat(orchestrator): add view-mode navigation, compact switcher, and tmux keybindings - #588

Merged
flora131 merged 11 commits into
mainfrom
flora131/feature/workflow-designs
Apr 13, 2026
Merged

feat(orchestrator): add view-mode navigation, compact switcher, and tmux keybindings#588
flora131 merged 11 commits into
mainfrom
flora131/feature/workflow-designs

Conversation

@flora131

@flora131 flora131 commented Apr 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds full keyboard navigation between the orchestrator graph view and attached agent views, a compact agent-switcher popup (/), and prefix-free tmux keybindings (Ctrl+G, Ctrl+\) for navigating the multi-agent workflow from any window.

Key Changes

View Mode State (orchestrator-panel-types.ts, orchestrator-panel-store.ts)

  • Added ViewMode type ("graph" | "attached") to orchestrator-panel-types.ts
  • PanelStore gains viewMode / activeAgentId state plus setViewMode(), getSubagents(), and getActiveAgentIndex() methods

Orchestrator Navigation (session-graph-panel.tsx)

  • Auto-reset: any key received while viewMode === "attached" (i.e. user returned to the orchestrator tmux window) automatically resets to graph mode
  • Polls active tmux window index every 300ms to detect Ctrl+G graph-return (tmux swallows the key before React sees it)
  • Enter on the orchestrator node stays in / returns to graph mode instead of switching windows
  • Removed Tab/Shift+Tab graph-grid cycling (now handled at the tmux level)
  • Syncs navigation hints into the tmux status bar while in attached mode; restores defaults on unmount

Compact Switcher (compact-switcher.tsx)

  • New CompactSwitcher popup component, opened with / from any view
  • Lists all agents with status icons, numbering, and elapsed duration
  • / or j/k to navigate, Enter to attach, Esc to close
  • Pre-selects the currently active or focused agent when opened

Tmux Keybindings (tmux.conf, tmux.ts)

  • Ctrl+G — jump straight back to graph (window 0) from any agent window (prefix-free)
  • Ctrl+\ — cycle to the next agent window (prefix-free); note: overrides the default SIGQUIT signal
  • tmux.ts now runs source-file after session creation so keybindings are always current in a running server
  • Exported TMUX_DEFAULT_STATUS_* constants and escapeTmuxFormat() to keep status-bar sync centralized

Statusline (statusline.tsx)

  • Shows focused node status as icon (///) instead of a text label
  • Adds / agents hint to the navigation bar
  • Removes Tab/Shift+Tab hints (no longer applicable)

Tests & Config

  • New tests for setViewMode, getSubagents, getActiveAgentIndex in orchestrator-panel-store.test.ts
  • Removed Tab/Shift+Tab graph-navigation tests from session-graph-panel.test.tsx
  • Updated statusline.test.tsx to assert icon-based status rendering () instead of text label
  • Excluded new UI components (compact-switcher.tsx, session-graph-panel.tsx) from coverage in bunfig.toml

Design Prototype

  • Added research/designs/option3-refined-prototype.html — interactive single-file prototype demonstrating the full keyboard UX (graph nav, attach, switcher, breadcrumb statusline)

Notes

  • Ctrl+\ overrides the default SIGQUIT signal. Agent CLIs in these panes will not receive SIGQUIT via Ctrl+\. The integrated agents (Claude Code, OpenCode, Copilot CLI) use Ctrl+C for interrupts and are unaffected.
  • The prefix-free bindings only apply inside Atomic's isolated tmux server (-L atomic) and do not affect the user's regular tmux sessions.

Remove all Esc key bindings to avoid interrupting running agents.
Use g to return to graph view, add Shift+Tab for prev agent, and
route orchestrator attachment back to graph view automatically.

Assistant-model: Claude Code
Use a dedicated subagents list (excluding orchestrator) for Tab/Shift+Tab
cycling and breadcrumb position display, so attached-view navigation only
rotates through actual sub-agents.

Assistant-model: Claude Code
Introduce graph/attached view-mode tracking in PanelStore so the UI
layer can coordinate between the session graph overview and an
attached agent window.

- Add ViewMode type ("graph" | "attached") to panel types
- Add viewMode, activeAgentId state and setViewMode() to PanelStore
- Add getSubagents() (filters out orchestrator + pending) and
  getActiveAgentIndex() helpers for Tab-cycling
- Full test coverage for new methods

Assistant-model: Claude Code
Add prefix-free tmux keybindings for workflow navigation:
- Ctrl+G: jump back to graph window (window 0) from any agent
- Ctrl+\: cycle to next agent window

Also source-file tmux.conf after createSession so keybindings
stay current in long-running sessions.

Assistant-model: Claude Code
…igation

Wire up the full graph-to-agent navigation flow:

- Add CompactSwitcher overlay (/ key) with j/k selection, Enter to
  jump, number keys for direct access, Esc to close
- Implement auto-reset: receiving any key while in attached mode
  automatically transitions back to graph mode
- Poll tmux window index to detect Ctrl+G return from agent windows
- Sync tmux status bar with attached-mode hints (agent name, index,
  Ctrl+G / Ctrl+\ shortcuts)
- Simplify Statusline to graph-only; add "/" agents hint
- Tab now attaches to first available subagent instead of spatial nav
- Update statusline test for icon-based status display

Assistant-model: Claude Code
Add compact-switcher.tsx and session-graph-panel.tsx to
coveragePathIgnorePatterns to match the components introduced
in the compact agent switcher feature.

Assistant-model: Claude Code
Copilot AI review requested due to automatic review settings April 12, 2026 22:44
@claude claude Bot changed the title Flora131/feature/workflow designs feat(orchestrator): add graph/attached navigation and compact agent switcher Apr 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the orchestrator “graph” UX to match new workflow navigation designs by introducing an in-app compact agent switcher, tmux-level navigation bindings, and refreshed statusline behavior.

Changes:

  • Redesign statusline to show status via icons and add new navigation hints (including / for agents).
  • Add a compact agent switcher overlay (/) plus attached/graph view mode tracking in the panel store.
  • Add tmux bindings and runtime syncing to support prefix-free navigation and attached-mode status hints.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/sdk/components/statusline.test.tsx Updates expectation to match icon-based status display.
src/sdk/runtime/tmux.ts Sources tmux config after session creation to refresh keybindings.
src/sdk/runtime/tmux.conf Adds prefix-free workflow navigation bindings (Ctrl+G / Ctrl+\).
src/sdk/components/statusline.tsx Updates statusline content to icon-based status + new hints.
src/sdk/components/session-graph-panel.tsx Adds compact switcher, attached/graph mode handling, tmux status sync, and return-to-graph detection.
src/sdk/components/orchestrator-panel-types.ts Introduces ViewMode type.
src/sdk/components/orchestrator-panel-store.ts Adds viewMode/activeAgentId plus helper methods for subagents/active index.
src/sdk/components/orchestrator-panel-store.test.ts Adds unit tests for the new store view-mode and helper APIs.
src/sdk/components/compact-switcher.tsx New compact agent list overlay UI component.
research/designs/option3-refined-prototype.html Adds a design prototype for the refined Option 3 workflow UI.
bunfig.toml Excludes new UI components from coverage collection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/sdk/runtime/tmux.ts
const paneId = tmux(args);
// Reload config into the running server so keybindings are always current
// (tmux only loads -f on first server start; source-file updates a running server).
tmuxRun(["source-file", CONFIG_PATH]);

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tmuxRun(["source-file", CONFIG_PATH]) ignores the return value, so a failure to reload the config (bad path/permissions, missing file, tmux error) will be silently swallowed and keybindings/status settings may not match expectations. Consider using the throwing helper (tmuxExec) or explicitly checking result.ok and surfacing an error when sourcing fails.

Suggested change
tmuxRun(["source-file", CONFIG_PATH]);
tmux(["source-file", CONFIG_PATH]);

Copilot uses AI. Check for mistakes.
return;
}
if (key.name === "down" || key.name === "j") {
setSwitcherSel((s) => Math.min(store.sessions.length - 1, s + 1));

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When store.sessions.length is 0, Math.min(store.sessions.length - 1, s + 1) evaluates to -1, allowing switcherSel to become negative. This produces an invalid selection state (and makes Enter a no-op). Clamp the upper bound to at least 0 (e.g., Math.max(0, store.sessions.length - 1)) or early-return when there are no sessions.

Suggested change
setSwitcherSel((s) => Math.min(store.sessions.length - 1, s + 1));
setSwitcherSel((s) =>
Math.min(Math.max(0, store.sessions.length - 1), s + 1),
);

Copilot uses AI. Check for mistakes.
Comment on lines +372 to +385
useEffect(() => {
if (store.viewMode !== "attached") return;

const check = () => {
const result = tmuxRun([
"display-message", "-t", tmuxSession, "-p", "#{window_index}",
]);
if (result.ok && result.stdout.trim() === "0") {
store.setViewMode("graph");
}
};

const id = setInterval(check, 300);
return () => clearInterval(id);

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ctrl+G return-to-graph detection polls tmux every 300ms using tmuxRun, which calls Bun.spawnSync each time. This can become a noticeable CPU/battery cost while attached. Consider reducing the polling frequency, or switching to a tmux hook-driven approach (e.g., a tmux client-* hook that triggers a lightweight signal/flag) to avoid tight synchronous polling.

Copilot uses AI. Check for mistakes.
Comment on lines +127 to +134
/**
* Return non-orchestrator agents that have started (not pending).
* These are the agents Tab/Shift+Tab cycles through.
*/
getSubagents(): SessionData[] {
return this.sessions.filter(
(s) => s.name !== "orchestrator" && s.status !== "pending",
);

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment says these are the agents that "Tab/Shift+Tab cycles through", but the updated keyboard handling in SessionGraphPanel no longer cycles with Tab (it attaches to the first subagent) and tmux-level navigation is bound to Ctrl+\. Please update the comment to match the actual navigation behavior so it doesn’t mislead future changes/tests.

Copilot uses AI. Check for mistakes.
@claude

claude Bot commented Apr 12, 2026

Copy link
Copy Markdown

Code Review — Workflow designs / compact switcher

Nice prototype work — the interaction model is well thought out, the store additions are clean, and the tests for setViewMode/getSubagents/getActiveAgentIndex are thorough. A few things I'd like to flag before this lands:

Bugs / Correctness

  1. Tab in graph mode ignores Shift and always jumps to the first subagent. src/sdk/components/session-graph-panel.tsx:272-277

    if (key.name === "tab") {
      const subs = store.getSubagents();
      if (subs.length > 0) doAttach(subs[0]!.name);
      return;
    }

    The prototype's keymap says Shift+Tab should cycle in reverse, and the previous behavior was navigate(key.shift ? "left" : "right"). Shift+Tab is now indistinguishable from Tab, which is a UX regression and doesn't match the design doc.

  2. Switcher number jump is inconsistent with Tab cycling. session-graph-panel.tsx:226-232
    Number keys index into store.sessions (which includes orchestrator at [0]), so pressing 1 "attaches" to orchestrator and falls through to setViewMode("graph"). But Tab uses getSubagents() which excludes orchestrator. The switcher UI also numbers from 1 over all sessions while getActiveAgentIndex() is 0-based within subagents only — same data, three different orderings. Pick one (likely subagents) and use it consistently in the switcher, its numeric index, and the statusline position (activeAgentIdx + 1/subagentCount).

  3. Only 1–9 reachable via number jump. parseInt(key.sequence, 10) only catches single-digit keys, so workflows with 10+ agents can't use this shortcut. Worth noting in a comment or doc, or handle multi-digit input buffering if intended.

  4. Polling Ctrl+G detection may be redundant with the auto-reset. session-graph-panel.tsx:368-382 polls tmux every 300 ms for the active window index, but session-graph-panel.tsx:242-246 also resets to graph on any key received while attached — and useKeyboard on the orchestrator window can only fire when that window is focused. If both mechanisms exist because one is flaky, pick one; otherwise the poll is wasted tmux IPC every 300 ms per mounted panel. Also, tmuxRun is synchronous subprocess exec — running it on a tight interval isn't free.

  5. source-file is invoked on every createSession. src/sdk/runtime/tmux.ts:179-181
    Every session creation re-sources the config. For a 7-agent workflow that's 7 redundant config reloads. Do this once at server startup (e.g., gate on "did we just start the server?" via has-session/list-sessions).

Design / Risk

  1. Global tmux status bar mutation with hardcoded restore string. session-graph-panel.tsx:391-418
    The restore path hardcodes " #{session_name} | %H:%M " and status-left-length=10. If the user's tmux.conf has a different status format (or if we later change the default in tmux.conf), the restore path silently desyncs. Two options:

    • Capture the current values with show-option -gv status-left before mutating, restore what you captured.
    • Scope changes to the Atomic session with -t <sessionName> instead of -g so the user's global config is never touched.

    Inlining theme colors (#6c7086, #1e1e2e, #cdd6f4, …) in the tmux string duplicates what's already in theme. Reading from the theme object keeps re-theming in one place.

  2. New tmux bindings are unconditional. src/sdk/runtime/tmux.conf:45-50
    bind -n C-g and bind -n C-\\ override common user bindings (C-g is the GNU readline abort, and both are active inside nested shells). This is a socket-isolated server (SOCKET_NAME = \"atomic\"), so the blast radius is contained — but please call this out in user-facing docs so nobody is surprised that C-g no longer cancels a readline prompt inside an Atomic session.

  3. useCallback deps omit store. session-graph-panel.tsx:141, 146, 154, 159 drop store from deps while reading store.setViewMode, store.sessions, store.viewMode, store.activeAgentId. Because store is a stable class instance from context it works, but the React exhaustive-deps lint will complain and future refactors could silently break. Either add store to the deps or add an eslint-disable comment explaining why.

Test coverage

  1. compact-switcher.tsx and session-graph-panel.tsx new code paths are excluded from coverage. bunfig.toml:73-74 adds both to coveragePathIgnorePatterns. Store methods are covered, but the switcher UI + keyboard dispatch (numeric jump, escape, return, wraparound) has nontrivial logic and no component test. At minimum, a test that verifies:

    • Opening the switcher pre-selects the correct index based on viewMode / activeAgentId
    • Pressing Esc closes it without side effects
    • Number key > session count is a no-op

    …would catch the inconsistencies flagged in (2).

  2. Status test weakened. tests/sdk/components/statusline.test.tsx:85-86 replaces an explicit \"running\" string assertion with a single Unicode bullet check. If any icon in the theme renders a bullet, this passes — consider asserting the running status' specific color or the exact focused-node name + icon pair.

Small / nits

  • src/sdk/components/compact-switcher.tsx:28width={44} is a magic number; consider exposing as a theme/layout constant.
  • session-graph-panel.tsx:150currentId can be \"\" when activeAgentId is unset, and findIndex returns -1, so Math.max(0, -1) === 0 silently selects the first row. Probably intentional but worth a one-line comment.
  • research/designs/option3-refined-prototype.html (682 lines) is a large artifact — fine since it's under research/, but flagging in case the intent was to keep research docs lean.

Positives

  • The store API (setViewMode, getSubagents, getActiveAgentIndex) is cleanly separated and well-tested.
  • Extracting CompactSwitcher as a standalone component keeps SessionGraphPanel readable.
  • Statusline simplification (removing the duplicate statusLabel next to the icon) is a nice cleanup.

Happy to discuss any of these — the Tab/Shift+Tab and switcher-indexing inconsistencies feel like the most important to sort out before merging.

Tab attach-to-first-subagent shortcut is no longer needed now that
Ctrl+G and arrow-key navigation cover graph traversal. Update the
getSubagents() docstring to reflect its remaining usage (tmux status
bar) and drop the corresponding tests.

Assistant-model: Claude Code
@claude

claude Bot commented Apr 12, 2026

Copy link
Copy Markdown

Code Review — #588

Thanks for the thorough PR description and the accompanying HTML prototype — both made the review much easier. Overall the decomposition is clean (types → store methods → UI components) and the new store methods are well-tested. A handful of concerns below, grouped by severity.

Potential bugs / correctness

1. Tab handler is missing — PR description mismatch
The PR description states "Tab now attaches to the first available subagent instead of navigating the graph grid" and the HTML prototype wires Tab to cycleAgent(). But session-graph-panel.tsx removes the old Tab case without adding a replacement — a case-insensitive grep for tab in the new file returns nothing. Meanwhile session-graph-panel.test.tsx deletes both Tab tests with no new coverage. Either drop the Tab claim from the PR description, or add the handler and a test.

2. Tmux format-string interpolation is not escaped (session-graph-panel.tsx:387)

const left = `... #[fg=#cdd6f4]${name} #[fg=#7f849c]${activeAgentIdx + 1}/${subagentCount}`;

If an agent name contains # (e.g. a node called build#2), tmux will interpret #[...] / #(...) as format directives and mangle the status bar — possibly even executing shell via #(). Agent names come from workflow definitions today (low risk), but this should escape ### before interpolation. Cheap defensive fix.

3. Pending agents are selectable in the compact switcher
CompactSwitcher lists store.sessions verbatim, but doAttach early-returns for pending sessions (session.status === \"pending\"). That means pressing Enter / a number on a pending entry in the popup silently does nothing. Either filter pending sessions out of the list, or dim + skip them during ↑/↓ navigation with a visual "waiting" affordance.

4. Number shortcuts in the switcher include the orchestrator
Pressing 1 in the switcher maps to store.sessions[0], which is the orchestrator, and ends up in setViewMode(\"graph\") without any visible transition (it's already graph mode when the popup was triggered from the graph). The switcher header says "jump" but 1 is effectively a no-op. Consider indexing number keys against the subagent list (via getSubagents()) so 1..9 always map to navigable entries, matching the numbering shown in the list.

5. useCallback dependency inconsistency (session-graph-panel.tsx:142, 146, 154, 159)
doAttach has [layout.map, tmuxSession] but reads store.sessions; openSwitcher, returnToGraph, closeSwitcher all capture store with [] deps. Works today because PanelStore is a stable reference, but it trips react-hooks/exhaustive-deps and quietly couples correctness to that stability invariant. I'd either include store in deps everywhere (no extra cost — it's stable, callbacks won't re-create) or leave a one-line comment explaining the pattern.

Performance

6. Tmux status-bar sync fires on every subagent count change (session-graph-panel.tsx:384-401)
The effect depends on subagentCount, which ticks every time any session starts or completes. Each tick spawns 4 synchronous tmuxRun subprocesses. In a busy workflow with frequent status transitions this is noticeable subprocess churn. Consider: (a) only updating status-left on activeAgentId change and status-right once per attach/detach transition, or (b) debouncing the write.

7. 300 ms polling of window_index (session-graph-panel.tsx:361-375)
Spawning a tmux display-message subprocess every 300 ms while attached is workable but heavy. A lighter alternative is a tmux hook — set-hook -g after-select-window 'run-shell ...' — or a FIFO written from the binding itself. At minimum, consider backing off to ~1 s; the perceptual threshold for "returned to graph" is well above 300 ms and the keypress fallback already catches foreground interactions.

8. source-file on every createSession (tmux.ts:181)
createSession already passes -f CONFIG_PATH to tmux new-session, which loads the config at server startup. Re-sourcing immediately afterwards is a no-op unless the file was edited during the same server's lifetime. The comment explains the hot-reload rationale, but in the production path this is redundant work; consider gating it on a dev flag or dropping it.

UX / discoverability

9. Ctrl+\\ collides with SIGQUIT
Binding Ctrl+\\ at the tmux level (bind -n C-\\\\) intercepts the traditional SIGQUIT keystroke before it reaches the child process. Users debugging a hung agent may be surprised they can no longer core-dump. Worth calling out in docs or picking a different key (e.g. Ctrl+], M-]).

10. useStore() called in Statusline but unused (statusline.tsx:18-19)
const store = useStore(); useStoreVersion(store); — the store binding isn't referenced afterwards. The subscription itself is intentional (re-render on updates), but dropping the binding or renaming to _ makes the intent obvious.

Tests / coverage

11. New files added to coveragePathIgnorePatterns
compact-switcher.tsx and session-graph-panel.tsx contain the new switcher state machine, the polling loop, and the tmux status-bar mirror — the riskiest logic in this PR. Ignoring them for coverage hides that the only new behavior exercised by tests are the three store methods. At minimum, add focused tests for the useKeyboard branches that handle switcher-open (escape / up / down / return / number) and the / → open flow.

12. `getSubagents` "excludes orchestrator" test is trivially satisfied
```ts
test("excludes orchestrator from subagent list", () => {
store.startSession("planner");
const subs = store.getSubagents();
expect(subs.every((s) => s.name !== "orchestrator")).toBe(true);
});
```
The `beforeEach` workflow doesn't register an orchestrator node, so the assertion passes vacuously. Add an orchestrator to the workflow + `startSession("orchestrator")`, then assert it's filtered out.

Nits

  • `statusline.test.tsx`: in addition to asserting the icon is present, also assert the old text label (`"running"`) is absent, so the test pins the intent rather than just the new symbol.
  • `openSwitcher` pre-selects via `store.sessions.findIndex(...)`; if you apply the fix in (4), align the pre-selection to `getSubagents()` as well.
  • Consider extracting the tmux status-bar restore into a small helper (`resetTmuxStatus()`) — it's duplicated across the sync effect and the unmount cleanup.

Strengths worth keeping

  • Belt-and-suspenders detection of return-to-graph (polling + keypress auto-reset) — nice resilience.
  • `setViewMode` / `getSubagents` / `getActiveAgentIndex` are well-scoped additions with focused tests.
  • The HTML prototype is an excellent design artifact — worth linking from a README or design doc.
  • Clean separation of `CompactSwitcher` as a pure presentational component driven by `selectedIndex`.

Happy to iterate on any of the above — #1, #2, and #11 are the ones I'd treat as blocking before merge.

…cher

Number keys 1-9 conflicted with graph panel input handling;
navigation is handled by the compact switcher UI instead.

Assistant-model: Claude Code
@claude claude Bot changed the title feat(orchestrator): add graph/attached navigation and compact agent switcher feat(orchestrator): add view-mode navigation and compact agent switcher Apr 12, 2026
@claude

claude Bot commented Apr 12, 2026

Copy link
Copy Markdown

Code Review — feat(orchestrator): graph/attached navigation and compact agent switcher

Nice feature. The store refactor (ViewMode, getSubagents, getActiveAgentIndex) is clean and well-tested, and the separation of the CompactSwitcher into its own component is good. A few items worth addressing before merge.

Higher-priority

1. 300 ms polling of tmux display-message is wasteful — src/sdk/components/session-graph-panel.tsx:354-368.
setInterval(check, 300) + Bun.spawnSync forks a tmux process ~3×/sec while attached. tmux already supports event-driven notifications via hooks; consider:

set-hook -g client-session-changed 'run-shell "echo ... >> /tmp/atomic-fifo"'

or set-hook -g after-select-window — then read/watch the FIFO. Even if the polling stays, it should be faster to detect the return (300 ms feels laggy), and you probably want to skip polling when no agent windows exist.

2. Ctrl+\\ collision with SIGQUIT — src/sdk/runtime/tmux.conf:51.
Because of bind -n, tmux intercepts it before the child process sees it, which is fine inside the orchestrator's tmux server. But Ctrl+\\ is a long-standing SIGQUIT binding in shells, and there's no escape hatch documented. Worth mentioning this in the status-bar hints or the docs, and confirming this doesn't break any of the integrated agent CLIs (Claude Code, OpenCode, Copilot CLI) that may use it internally.

3. tmux format-string injection surface — src/sdk/components/session-graph-panel.tsx:380.
status-left contains ${name} where name = store.activeAgentId. tmux will expand any #{...} or #[...] tokens inside that string. Workflow-defined names today are safe (planner, writer, …), but if this ever accepts user-supplied names you'll want to escape ### before interpolating. Cheap fix now:

const safeName = name.replace(/#/g, "##");

4. Coverage exclusion regression — bunfig.toml:73-74.
Adding session-graph-panel.tsx and compact-switcher.tsx to coveragePathIgnorePatterns silently drops ~260 lines of new behavior from measured coverage. There's already a session-graph-panel.test.tsx exercising it; if the tests are running but coverage is flaky due to OpenTUI rendering, a comment explaining why it's excluded would help the next person not undo this.

5. Duplicated default tmux status-bar strings — src/sdk/components/session-graph-panel.tsx:387-392, 397-403 + tmux.conf:24-26.
The defaults live in three places. If tmux.conf changes (e.g. different status-right format), the restore logic silently drifts. Extract into a shared constant (e.g. TMUX_DEFAULT_STATUS_LEFT, TMUX_DEFAULT_STATUS_RIGHT) in tmux.ts and reference it from both the component and — if possible — document that tmux.conf must match.

Lower-priority

6. useCallback deps on store.*session-graph-panel.tsx:141-159.
doAttach, returnToGraph, openSwitcher, closeSwitcher reference store.sessions / store.viewMode / store.activeAgentId but have empty or minimal dep arrays. It works because PanelStore is a stable class instance and properties are read imperatively at call time, but react-hooks/exhaustive-deps will flag it. Add store to deps (or leave a comment explaining the pattern).

7. CompactSwitcher width hardcoded at 44 — compact-switcher.tsx:28.
Long agent names (e.g. frontend-component-reviewer) plus duration will overflow. Consider minWidth={44} + flex growth, or truncate with .

8. Pending agents are silently unselectable — session-graph-panel.tsx:220-224.
User can navigate to a pending agent in the switcher, press Enter, and nothing happens. Either hide pending rows (the switcher mirrors store.sessions, so you could filter to started) or render them dimmed/disabled so the no-op is obvious.

9. Orchestrator in the switcher.
agents = store.sessions includes orchestrator. Selecting it triggers `setViewMode("graph")` rather than attaching — functionally fine, but the UX is slightly surprising. Either label it distinctly ("← back to graph") or move it to a dedicated row.

10. Prototype vs. implementation parity.
The HTML prototype supports number-key direct-jump (1-9) and Tab/Shift+Tab cycling in the switcher; the TSX only has ↑↓/jk/Enter/Esc. If the prototype is the spec, the TSX is missing features. If not, consider trimming the prototype to match.

11. 682-line HTML prototype in the repo — research/designs/option3-refined-prototype.html.
Totally reasonable as design documentation, but worth confirming this is the right home for it and whether it should live outside the npm package (files in package.json) to keep the shipped artifact small.

12. source-file on every createSessiontmux.ts:179-181.
Comment says "tmux only loads -f on first server start" — correct. But this now re-sources the conf every time a new tmux session is created (which may be once per workflow, OK), and silently ignores failure via tmuxRun. If the config ever has a syntax error it'll be a silent no-op; logging the stderr on !result.ok would help debuggability.

Tests

  • orchestrator-panel-store.test.ts additions look thorough — setViewMode, getSubagents, getActiveAgentIndex each cover edge cases (empty, pending-only, orchestrator exclusion, non-existent agent).
  • Removed Tab/Shift+Tab tests are appropriate given the behavior moved to tmux.
  • No tests for the new keyboard paths in session-graph-panel (switcher open/close, / trigger, Ctrl+G polling effect, tmux status-bar sync). Given the coverage exclusion this is understandable, but at minimum the switcher open/close + arrow + Enter flow would be valuable and testable with the existing mockInput harness.

Style / CLAUDE.md compliance

  • Bun-only: no node/npm usage introduced ✅
  • No any/unknown in new code ✅
  • tmuxRun (array-arg, no shell) correctly avoids shell injection ✅

Overall the direction is solid and the store layer is clean — the main asks are reducing the tmux polling cost, tightening the coverage story, and factoring out the duplicated status-bar restore strings.

Extract TMUX_DEFAULT_STATUS_* constants into tmux.ts so the restore
logic in session-graph-panel stays in sync with tmux.conf without
duplicating magic strings.  Add escapeTmuxFormat() to prevent agent IDs
containing '#' from being interpreted as tmux style/command directives.

Assistant-model: Claude Code
…ride

Clarify that Ctrl+G / Ctrl+\ bindings use the root table and only
apply inside Atomic's isolated tmux server.  Note that Ctrl+\ replaces
the default SIGQUIT signal and document the workaround.

Assistant-model: Claude Code
@claude claude Bot changed the title feat(orchestrator): add view-mode navigation and compact agent switcher feat(orchestrator): add view-mode navigation, compact switcher, and tmux keybindings Apr 13, 2026
@claude

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

PR Review: view-mode navigation + compact agent switcher

Nice, well-scoped feature. The three-layer design (tmux bindings → React view-mode state → tmux status-bar mirror) is a sensible way to make navigation discoverable across the boundary. The tests for the new store methods are solid and the prototype HTML is a helpful reference.

Flagging a handful of concerns worth addressing before merge.

Correctness / bugs

1. doAttach uses tmuxSession in callback deps, but the useCallback deps only include [layout.map, tmuxSession] (was [layout.map, tmuxSession, store.sessions] before the change). src/sdk/components/session-graph-panel.tsx:148 — after refactor it drops store.sessions from deps but still reads store.sessions via .find(...) on line 131. In practice the store ref is stable so this works, but it is inconsistent with the rest of the file and will trip the exhaustive-deps lint rule. Same inconsistency in returnToGraph / openSwitcher / closeSwitcher (empty dep arrays while referencing store).

2. CompactSwitcher lists every session incl. orchestrator and pending agents, but doAttach silently rejects pending. compact-switcher.tsx:20 uses store.sessions directly rather than store.getSubagents(). Pressing Enter on a pending row is a no-op with zero visual feedback — user thinks the key is broken. Suggestions:

  • Source the list from getSubagents() (matches the status-line 1/N position), or
  • Disable pending rows and skip them during j/k navigation, or
  • At minimum, flash an "→ agent not started yet" message in attachMsg.

Also: including orchestrator in a list labeled "agents" and having Enter "return to graph" mixes two distinct concepts. Consider omitting orchestrator and having a separate affordance (or just relying on Ctrl+G).

3. Pre-selection fallback lands on index 0 (orchestrator). session-graph-panel.tsx:159setSwitcherSel(Math.max(0, idx)). When findIndex returns -1 (agent not found), selection silently becomes orchestrator rather than the focused/active agent. Either use the subagent list (then -1 becomes 0 = first real agent) or fall back to the currently focused index.

4. Prototype keymap documents Tab / number-shortcuts; the React handler doesn't implement them. The HTML prototype (line 331, 649) supports Tab/1-9 in the switcher, but session-graph-panel.tsx:214-234 only has up/down/j/k/return/escape. Either update the prototype or add the shortcuts.

5. Silent tmux failures in doAttach. tmuxRun([\"switch-client\", ...]) returns {ok: false, stderr} on failure (wrong session name, detached client, etc.) and nothing surfaces. The polling effect will self-heal the viewMode, but the user sees no indication that the attach didn't work. Consider surfacing stderr via attachMsg on failure.

Performance

6. 300ms subprocess polling is heavier than it looks. session-graph-panel.tsx:361-375 spawns a tmux display-message subprocess ~3×/sec while attached. On Windows/psmux subprocess spawn is 20–50ms+; on slower systems this is a visible tax. Consider either:

  • A tmux hook: set-hook -g client-session-changed or session-window-changed firing run-shell \"...\" that pokes a FIFO the React app reads, or
  • Chaining the Ctrl+G binding to do the poke itself: bind -n C-g select-window -t :0 \; run-shell \"...\".

These are more complex but eliminate the polling entirely and cut latency from up-to-300ms to immediate.

7. Status-bar sync fires 4 tmuxRun subprocess calls per viewMode/active-agent change. session-graph-panel.tsx:384-401. Could be collapsed into a single tmuxRun([\"set\", \"-g\", \"status-left\", L, \";\", \"set\", \"-g\", \"status-left-length\", \"50\", ...]) using tmux's ; separator — one subprocess instead of four.

Design / consistency

8. Duplicate restore logic. session-graph-panel.tsx:394-400 and 404-410 run the same 4 tmuxRun calls. Extract a restoreDefaultStatusBar() helper (or a setStatusBar(left, right, ...) helper) and use it in both places.

9. Hardcoded hex colors in left / right strings (#6c7086, #1e1e2e, #cdd6f4, #7f849c). These bypass the theme. If the theme ever varies (per-agent or per-user), the tmux status bar will drift. Pull them from theme.* and build the format string from there.

10. Mode badge literal always says GRAPH. statusline.tsx:21 now hard-codes "GRAPH" with a comment saying it's always graph-mode. True today, but the badge component could be simplified to a non-conditional label or renamed (GraphBadge) to make the invariant explicit.

Tests

11. New state methods are well-tested (setViewMode, getSubagents, getActiveAgentIndex) — good coverage on happy path and edges (-1, pending exclusion, orchestrator exclusion).

12. Missing tests for the switcher keyboard loop and auto-reset behavior. session-graph-panel.tsx is excluded from coverage (bunfig.toml:74), but these are new user-facing behaviors:

  • / opens switcher, j/k navigate, Enter attaches, Esc closes
  • Auto-reset: any key in attached mode returns to graph
  • Pre-selection of current/focused agent

The existing session-graph-panel.test.tsx already renders the panel with a mock input, so extending it should be straightforward even though other tests from it rely on rendering frames.

13. escapeTmuxFormat is exported and used, but no unit tests for it. Easy win — a one-liner test file covering ###, empty string, no-op for safe inputs.

Security / tmux concerns

14. Ctrl+\\ overriding SIGQUIT is documented in the conf (good). Worth calling out that non-integrated tooling a user might spawn in an agent pane (a bash shell, a debugger) loses SIGQUIT. The scope (-L atomic socket) is the right mitigation — just make sure this is surfaced in user docs, not only in the conf comment.

15. No escaping of tmuxSession / node name in switch-client -t \"${tmuxSession}:${n.name}\". session-graph-panel.tsx:146. Tmux args aren't shell-interpreted (args go through Bun.spawnSync), so it's not a shell-injection risk. But a colon or whitespace in a node name would break the target selector. Worth validating session/window names at the ingress point (workflow definition) rather than trusting them here.

Minor / nits

  • String(i + 1).padStart(2) — default fill is space, which is what you want; explicit \" \" would make intent clearer.
  • CompactSwitcher width is fixed at 44 cols — will overflow agent names > ~28 chars. Consider width=\"auto\" with a maxWidth or derive from the longest agent.name.
  • The source-file after createSession silently ignores failures (tmuxRun returns but result is discarded). A malformed tmux.conf would leave stale bindings with zero diagnostic. Consider logging when !result.ok.
  • Comment on session-graph-panel.tsx:209 still says \"gg double-tap tracking (graph mode only)\" — fine, but the "graph mode only" qualifier is no longer enforced since auto-reset transparently flips attached→graph before key dispatch. Either remove the qualifier or early-return when viewMode === \"attached\".

Summary

The core approach is sound and the test additions for store state are high-quality. The main things I'd want resolved before merge: (1) switcher handling of pending/orchestrator rows (UX footgun), (2) the 300ms polling — either document it as a known cost or switch to a hook-driven approach, and (3) some coverage of the switcher keyboard loop in tests. Everything else is quality-of-life.

@flora131
flora131 merged commit 8fb4418 into main Apr 13, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants