feat(desktop): browser pane の Connect 導線と session バインディング UI (Phase 1)#345
Merged
feat(desktop): browser pane の Connect 導線と session バインディング UI (Phase 1)#345
Conversation
…1 UI) browser pane toolbar に Connect ボタン・session 選択モーダル・一覧ビューを追加。 paneId -> sessionId のバインディングは renderer 側 zustand store で管理し、 ダミーデータで UX を一通り動作させる。Phase 2 以降で実セッション/MCP 連携に 置き換える想定。 - browser-automation store: sessions/bindings/connectModal/listView の状態管理 - ConnectButton: toolbar に配置、接続済みは "Session X · Provider" を表示 - SessionConnectModal: 2カラム（セッション一覧 + 詳細/MCP セットアップ案内） - BrowserAutomationList: workspace の全 browser pane とバインディング状況 - 再割当トースト / 切断導線 / MCP missing の inline ガイドを実装 - mock.html / todo.md を参考資料として追加 (biome では除外)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughブラウザペインと実行中LLMセッションを結び付けるUIとバックエンドを追加。Zustandストア、TRPCルーターと永続BindingStore、データフック、Connectボタン、セッション接続モーダル、一覧ダイアログ、DBマイグレーション、モックと仕様書が含まれます。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Renderer UI
participant Store as BrowserAutomation Store
participant Hook as useBrowserAutomationData
participant TRPC as Main TRPC Router
participant DB as BindingStore / Local DB
UI->>Store: openConnectModal(paneId, preselectSession?)
Store-->>UI: connectModal state (isOpen, paneId, selectedSessionId)
UI->>Hook: fetch sessions & bindings (enabled)
Hook->>TRPC: query todo/terminal sessions, mcp status, listBindings
TRPC->>DB: read/write bindings, detect MCP, list sessions
DB-->>TRPC: bindings / sessions / mcpStatus
TRPC-->>Hook: return sessions, bindingsByPane, mcpStatus
UI->>TRPC: setBinding(paneId, sessionId) / removeBinding(paneId)
TRPC->>DB: set/remove binding (emit onBindingsChanged)
DB-->>TRPC: emit change
TRPC->>Hook: subscription onBindingsChanged -> invalidate caches
Hook-->>UI: refreshed data
UI->>Store: close modal / setSelectedSession / setListViewOpen
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50分 Possibly related PRs
詩
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22a516763
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
biome.jsonc (1)
15-15:!mock.htmlはリポジトリ直下のファイルにのみマッチします。現状
mock.htmlはリポジトリ直下にあるので意図通り動きますが、将来的にサブディレクトリにもモックHTMLを置く可能性があるなら!**/mock.htmlの方が安全です。現時点ではこのままでも問題ありません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@biome.jsonc` at line 15, 現在のパターン "!mock.html" はリポジトリ直下の mock.html にのみマッチするため、将来サブディレクトリにも mock.html を置く可能性がある場合はパターンを "!**/mock.html" に変更してください；具体的には biome.jsonc のエントリ "!mock.html" を見つけて、リポジトリ内の任意のサブディレクトリにある mock.html も除外できるように "!**/mock.html" に置き換えます。apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx (1)
80-85:SessionConnectModalの配置をContentView側に引き上げることを検討してください (recommended)。
SessionConnectModalは概念的にシングルトン (connectModal.paneIdが単数) ですが、現状BrowserPane内にレンダリングしているため、ワークスペースに開いている browser pane の数だけ Radix Dialog ツリーがマウントされます。加えて Line 80-85 で全BrowserPaneインスタンスがconnectModalを購読するため、モーダル開閉のたびに全 pane が再レンダリングされます。同じ PR で
BrowserAutomationListをContentView直下に単一マウントしているのと整合させ、SessionConnectModalもContentViewでマウントしてconnectModal.paneIdから pane context を解決する方が、マウント数・再レンダー範囲の両面で健全です。Phase 1 のモック段階ではブロッカーではないので、Phase 2 以降の実データ配線と合わせて整理する形で問題ありません。Also applies to: 437-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx` around lines 80 - 85, Move the SessionConnectModal out of BrowserPane and mount it once under ContentView: stop rendering SessionConnectModal inside BrowserPane and instead add a single SessionConnectModal in ContentView (aligned with BrowserAutomationList being single-mounted). Update BrowserPane to stop subscribing to connectModal via useBrowserAutomationStore for local render gating (remove the isConnectOpenForThisPane logic that uses connectModal and paneId), and resolve the active pane context inside ContentView from connectModal.paneId when opening the modal so the modal can derive the target pane. Ensure references to SessionConnectModal, ContentView, BrowserPane, connectModal, useBrowserAutomationStore, isConnectOpenForThisPane and paneId are updated accordingly so only one Radix Dialog tree is mounted and panes no longer re-render on modal state changes.apps/desktop/src/renderer/stores/browser-automation.ts (1)
5-13:"Literal" | stringはstringに畳まれてしまい、リテラル補完が効きません
provider: "Claude" | "Codex" | stringおよびkind: "Terminal" | "Chat" | stringは、| stringによって union がstringに吸収されるため、せっかく列挙したリテラル型が IDE 補完・型ガードのどちらにも寄与しません。Phase 2 以降で provider を文字列比較（例:SessionConnectModal内のsession.provider === "Codex"）している箇所の網羅性チェックにも支障が出ます。既知値を型で担保したい場合は
stringを落として union のみにし、拡張したい場合はstring & {}などの「literal union を保つ」イディオムを使うのが安全です。♻️ 提案
- provider: "Claude" | "Codex" | string; - kind: "Terminal" | "Chat" | string; + provider: "Claude" | "Codex" | (string & {}); + kind: "Terminal" | "Chat" | (string & {});将来 provider を厳格に絞る方針なら
string & {}も外して純粋な union にしてください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/browser-automation.ts` around lines 5 - 13, The AutomationSession interface's provider and kind fields use `"Claude" | "Codex" | string` and `"Terminal" | "Chat" | string`, which collapses the union to plain string and disables literal completion and exhaustiveness checks; update the fields in AutomationSession (provider and kind) to preserve literal unions either by removing the plain string if you want strict enums or by replacing `string` with the "preserve literals" idiom `string & {}` so code that does `session.provider === "Codex"` (e.g., SessionConnectModal) gets proper completion and TS exhaustiveness support.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (1)
135-153:assignedElsewherePaneNameの算出が 1 カードあたり 3 回Object.entries(bindings).find(...)を走らせていて、可読性・計算量ともに悪いですセッション件数 × pane 件数 × 3 回の線形走査が毎レンダリング発生します。さらに上部 (Lines 52-59) で
assignedPaneIdForSelectedとして同等の検索を済ませているのに、ここでは再利用せず 3 重に書いており DRY 違反です。sessionId → paneIdの逆引きマップをuseMemoで一度だけ作れば、ロジックも読みやすく、同じ値を footer /ReadyPanelの分岐でも使い回せます。♻️ 提案
const sessionList = useMemo(() => Object.values(sessions), [sessions]); const session = selectedSessionId ? sessions[selectedSessionId] : null; const currentBinding = paneId ? bindings[paneId] : null; const currentSession = currentBinding ? sessions[currentBinding] : null; + // sessionId -> paneId (他の pane に割当済み) の逆引き + const sessionToOtherPaneId = useMemo(() => { + const map: Record<string, string> = {}; + for (const [pid, sid] of Object.entries(bindings)) { + if (pid !== paneId) map[sid] = pid; + } + return map; + }, [bindings, paneId]); + const assignedPaneIdForSelected = useMemo(() => { - if (!selectedSessionId) return null; - return ( - Object.entries(bindings).find( - ([pid, sid]) => sid === selectedSessionId && pid !== paneId, - )?.[0] ?? null - ); - }, [bindings, paneId, selectedSessionId]); + if (!selectedSessionId) return null; + return sessionToOtherPaneId[selectedSessionId] ?? null; + }, [sessionToOtherPaneId, selectedSessionId]); @@ - {sessionList.map((s) => ( - <SessionCard - key={s.id} - session={s} - isSelected={s.id === selectedSessionId} - assignedElsewherePaneName={ - Object.entries(bindings).find( - ([pid, sid]) => sid === s.id && pid !== paneId, - ) - ? (panes[ - Object.entries(bindings).find( - ([pid, sid]) => sid === s.id && pid !== paneId, - )?.[0] ?? "" - ]?.name ?? null) - : null - } - onSelect={() => setSelectedSession(s.id)} - /> - ))} + {sessionList.map((s) => { + const otherPaneId = sessionToOtherPaneId[s.id]; + return ( + <SessionCard + key={s.id} + session={s} + isSelected={s.id === selectedSessionId} + assignedElsewherePaneName={ + otherPaneId ? (panes[otherPaneId]?.name ?? null) : null + } + onSelect={() => setSelectedSession(s.id)} + /> + ); + })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 135 - 153, The assignedElsewherePaneName calculation inside the SessionCard map is inefficient and duplicated: it calls Object.entries(bindings).find(...) three times per session and redoes work already performed for assignedPaneIdForSelected; create a memoized reverse map (e.g., sessionIdToPaneId) using useMemo from bindings once, then for each SessionCard compute assignedElsewherePaneName by looking up sessionIdToPaneId[s.id] and, if found and not equal to paneId, retrieve panes[thatPaneId]?.name or null; update references in the SessionCard props and reuse this memo for footer / ReadyPanel branching to remove the repeated find calls and DRY up the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/stores/browser-automation.ts`:
- Around line 115-121: The markSessionReady updater currently spreads
s.sessions[sessionId] even when it is undefined, creating a broken session
object; update markSessionReady to check whether s.sessions[sessionId] exists
and if not return the state unchanged (early return) so you only call set to
update mcpStatus when the session is present; reference the markSessionReady
function and the s.sessions map to locate and implement this guard.
In `@mock.html`:
- Around line 1798-1909: The Guide click handler sets selectedSessionId but
openModal() immediately overwrites it; update openModal to accept an optional
sessionId parameter (openModal(sessionId?)) and, inside openModal, only set
selectedSessionId from pane.connectedSessionId || pane.suggestedSessionId ||
"session-14" when sessionId is undefined, otherwise use the passed sessionId;
then change the caller in renderAutomationCards (the Guide button click) to call
openModal(button.dataset.sessionId) so the clicked session remains selected.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx`:
- Around line 80-85: Move the SessionConnectModal out of BrowserPane and mount
it once under ContentView: stop rendering SessionConnectModal inside BrowserPane
and instead add a single SessionConnectModal in ContentView (aligned with
BrowserAutomationList being single-mounted). Update BrowserPane to stop
subscribing to connectModal via useBrowserAutomationStore for local render
gating (remove the isConnectOpenForThisPane logic that uses connectModal and
paneId), and resolve the active pane context inside ContentView from
connectModal.paneId when opening the modal so the modal can derive the target
pane. Ensure references to SessionConnectModal, ContentView, BrowserPane,
connectModal, useBrowserAutomationStore, isConnectOpenForThisPane and paneId are
updated accordingly so only one Radix Dialog tree is mounted and panes no longer
re-render on modal state changes.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 135-153: The assignedElsewherePaneName calculation inside the
SessionCard map is inefficient and duplicated: it calls
Object.entries(bindings).find(...) three times per session and redoes work
already performed for assignedPaneIdForSelected; create a memoized reverse map
(e.g., sessionIdToPaneId) using useMemo from bindings once, then for each
SessionCard compute assignedElsewherePaneName by looking up
sessionIdToPaneId[s.id] and, if found and not equal to paneId, retrieve
panes[thatPaneId]?.name or null; update references in the SessionCard props and
reuse this memo for footer / ReadyPanel branching to remove the repeated find
calls and DRY up the logic.
In `@apps/desktop/src/renderer/stores/browser-automation.ts`:
- Around line 5-13: The AutomationSession interface's provider and kind fields
use `"Claude" | "Codex" | string` and `"Terminal" | "Chat" | string`, which
collapses the union to plain string and disables literal completion and
exhaustiveness checks; update the fields in AutomationSession (provider and
kind) to preserve literal unions either by removing the plain string if you want
strict enums or by replacing `string` with the "preserve literals" idiom `string
& {}` so code that does `session.provider === "Codex"` (e.g.,
SessionConnectModal) gets proper completion and TS exhaustiveness support.
In `@biome.jsonc`:
- Line 15: 現在のパターン "!mock.html" はリポジトリ直下の mock.html にのみマッチするため、将来サブディレクトリにも
mock.html を置く可能性がある場合はパターンを "!**/mock.html" に変更してください；具体的には biome.jsonc のエントリ
"!mock.html" を見つけて、リポジトリ内の任意のサブディレクトリにある mock.html も除外できるように "!**/mock.html"
に置き換えます。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 459140a5-ef70-4524-ac45-6ed3e7b5e082
📒 Files selected for processing (12)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/ConnectButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/stores/browser-automation.tsbiome.jsoncmock.htmltodo.md
ダミーデータを削除し、実データを参照するよう配線:
- main-process に browserAutomation tRPC router を追加
- listBindings / setBinding / removeBinding (paneId -> sessionId 管理を in-memory で保持)
- getMcpStatus (~/.claude/settings.json と ~/.codex/config.toml を走査し
superset-browser MCP entry の有無を検出)
- onBindingsChanged subscription で renderer を即時同期
- renderer の store は UI-only state (connectModal, listView) のみを保持し、
session / binding / MCP status は useBrowserAutomationData フックから取得
- session 一覧は todoAgent.listAll (動いている Claude Code の TODO-Agent セッション)
を変換して表示
- SessionConnectModal の Connect / Disconnect / 再割当は trpc mutation 経由で永続化
- 空状態 (セッション無し) と MCP 未設定 (実 configPath を案内) の文言を追加
- lint:fix による TodoModal.tsx 等の format 追従も含む
- workspace フィルタ: tabs.workspaceId で現在の workspace に属する browser pane だけを表示 - Focus ボタン: setFocusedPane の前に setActiveTab で pane が属する tab をアクティブ化し、 別 tab 上の pane を選んだ時もユーザーが遷移を体感できるようにする Refs: - apps/desktop/src/renderer/.../BrowserAutomationList.tsx:31 (codex P2) - apps/desktop/src/renderer/.../BrowserAutomationList.tsx:108 (codex P2)
Owner
Author
|
@codex review 実セッション連携と codex レビュー指摘を反映しました:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b636498a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- useBrowserAutomationData: exclude aborted/escalated sessions too (P1) - SessionConnectModal: allow disconnect when pane has a binding even if the bound session is no longer in the running list (P2)
Owner
Author
|
@codex review 前回指摘を反映:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b75711bae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- useBrowserAutomationData: only include sessions with live status (running / preparing / verifying / waiting). queued / paused / terminal are now filtered out so users cannot bind a pane to a dormant worker. (P2) - onBindingsChanged: invalidate listBindings so other windows see updates without waiting for the next poll. (P3)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/desktop/src/renderer/stores/browser-automation.ts (2)
82-95:formatRelativeTimeで未来時刻（clock skew など）に負の値が出ます。ホスト／todo-daemon 側の時計がずれたり、直前に書き込まれたタイムスタンプが
Date.now()を僅かに上回ると"-1s ago"のような表記になります。Math.max(0, ...)でクランプするか、< 0は"just now"に丸めるのが無難です。♻️ 提案
- const diffSec = Math.round((Date.now() - ts) / 1000); - if (diffSec < 5) return "just now"; + const diffSec = Math.max(0, Math.round((Date.now() - ts) / 1000)); + if (diffSec < 5) return "just now";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/browser-automation.ts` around lines 82 - 95, The function formatRelativeTime can return negative relative strings when ts is slightly in the future; update formatRelativeTime to clamp negative differences (e.g., compute diffSec = Math.max(0, Math.round((Date.now() - ts) / 1000))) or treat any diffSec < 0 as 0/“just now” so you never produce "-1s ago"; modify the logic in formatRelativeTime accordingly and keep the existing branch ordering (just now, seconds, minutes, hours, days).
5-13:provider/kindをstringとの union にするとリテラル型の恩恵が消えます。
"Claude" | "Codex" | stringは TypeScript 上では実質stringに縮退し、switch/絞り込みで網羅チェックが効きません。Phase 1 段階でも確定しているリテラル（"Claude" | "Codex","Terminal" | "Chat"）に留め、未知値が必要になったらprovider: Provider | { custom: string }のように明示的に分岐させるほうが将来安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/browser-automation.ts` around lines 5 - 13, The AutomationSession interface currently defines provider and kind as `"Claude" | "Codex" | string` and `"Terminal" | "Chat" | string`, which collapses to plain string and loses literal type benefits; update AutomationSession so provider is a strict union of known literals (e.g., "Claude" | "Codex") and kind is a strict union of known literals (e.g., "Terminal" | "Chat"), and if you need to represent unknown/third‑party values add an explicit discriminated variant (for example Provider = "Claude" | "Codex" | { custom: string } and likewise for kind) so switch/case and type narrowing on provider and kind remain exhaustive and type-safe while still supporting extensibility.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx (1)
42-47:Needs setupメトリクスのスコープについてご確認ください。
connectedCountは当該 workspace の browser pane に絞って数えていますが、needsSetupCountは全sessions（他 workspace で稼働中のものも含む）の MCP 未整備数を数えています。本モーダルは「この workspace の browser panes と結合状況」を提示する UI なので、メトリクスも workspace スコープに揃えたほうが一貫性があります（例: この workspace 内の pane に現在バインドされている／バインド候補のセッションに限定）。意図的にグローバル集計しているならコメントを一行添えておくと後続レビューで迷いません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx` around lines 42 - 47, The needsSetupCount currently counts MCP-missing across all sessions but should be scoped to this workspace like connectedCount; update the calculation for needsSetupCount to only consider sessions related to this workspace's browserPanes (e.g., sessions whose pane or candidate matches an id in browserPanes) and still check s.mcpStatus === "missing" using the existing symbols (browserPanes, sessions, bindingsByPane, needsSetupCount, connectedCount); if the global aggregation was intended instead, add a one-line comment above the needsSetupCount declaration explaining that it's intentionally global.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (2)
111-123: Disconnect 後にモーダルを閉じない挙動は意図通りかご確認ください。
handleConnectは成功時にonOpenChange(false)でモーダルを閉じますが、handleDisconnectは閉じません。Disconnect 後にそのまま別セッションを選び直せるよう意図的に開いたままにしているなら問題ありませんが、単発で切断だけしたいユーザーは余計にキャンセル操作が必要になります。意図を明確にする意味でも、例えばcurrentBindingが切れたら footer 文言を "Disconnected — pick another session or close" に変える／あるいは閉じる、どちらかのハンドリングを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 111 - 123, handleDisconnect currently disconnects but does not close the modal, while handleConnect calls onOpenChange(false); decide and implement consistent behavior: either close the modal on successful disconnect by calling onOpenChange(false) inside handleDisconnect after removeBinding.mutateAsync succeeds, or update the modal footer when currentBinding becomes null (e.g., set a local footer state or derive footer text from currentBinding and render "Disconnected — pick another session or close"); update the Footer rendering logic to read that state/derived text so UI reflects the new disconnected state. Ensure to reference and use onOpenChange, handleDisconnect, handleConnect, and currentBinding when making the change.
284-480: 同一ファイル内に多数のサブコンポーネント（SessionCard/Tag/ReadyPanel/DetailItem/SetupPanel）が定義されており、肥大化しています。本ファイルは 480 行超で 5 つのローカルコンポーネントを抱えています。コーディングガイドラインの「1 コンポーネント 1 フォルダ」原則は外部公開コンポーネント向けの規約ですが、再利用性・可読性の観点でもこの規模だと
SessionConnectModal/フォルダ配下にSessionCard.tsx/SetupPanel.tsx/ReadyPanel.tsxを分割しておくのが将来のメンテ上有利です（Phase 2 で Codex 固有のSetupPanel分岐や、ReadyPanel のリアルタイム情報追加が予定されているなら特に）。As per coding guidelines:
Use one folder per component with structure ComponentName/ComponentName.tsx plus index.ts for barrel export.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 284 - 480, Split the large SessionConnectModal file by extracting the local subcomponents SessionCard, Tag, ReadyPanel, DetailItem, and SetupPanel into their own component folders (e.g. SessionCard/SessionCard.tsx, SetupPanel/SetupPanel.tsx, ReadyPanel/ReadyPanel.tsx) with a barrel index.ts export for each, move any related helper usage (e.g. getSnippetForSession) or prop types they need into shared exports or pass them as props, update the main SessionConnectModal to import these components instead of defining them inline, and ensure all props and callbacks (like onSelect, onCopy, mcpConfigPath, session, paneName, reassigning, previousPaneName) are preserved and typed the same so behavior remains identical.apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts (2)
48-75:providerを常に"Claude"に固定している点は Phase 1 の割り切りとして問題ありませんが、mcpStatusのマッピングにご注意を。
todoSessionsは現状すべて Claude Code ワーカーというコメント前提でprovider = "Claude"/mcpStatus.claudeReadyのみを見ていますが、SessionConnectModalではsession.provider === "Codex"分岐でcodexConfigPathを参照する UI があり（スニペット/セットアップガイド）、Codex ワーカーが本フックから生えてくるまで到達不能コードになっています。Phase 2 以降で Codex 対応を入れる際は、ここで provider を判定しcodexReadyを参照するように合わせて更新する必要があります。現段階では// TODO(phase2): codex worker -> provider="Codex" & codexReadyといったマーカーを残しておくと後で迷子になりにくいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts` around lines 48 - 75, The mapping for todoSessions assumes every session is Claude by hardcoding provider = "Claude" and only reading mcpStatus.claudeReady; add a TODO marker and update instructions so future Phase 2 Codex support is obvious: leave the provider assignment but add a comment like "// TODO(phase2): detect Codex workers -> provider = 'Codex' and use mcpStatus.codexReady" near the provider/mcpStatus logic (referencing provider, mcpStatus, todoSessions), and ensure SessionConnectModal/codexConfigPath consumers will be able to rely on provider === "Codex" and mcpStatus.codexReady once implemented.
27-36:onBindingsChangedサブスクリプションとrefetchInterval: 1000のポーリングが冗長です。サブスクリプション経由で
listBindingsを invalidate しているため、追加で 1 秒間隔のポーリングを掛ける必要はありません。ポーリングを外すか、サブスクリプション未接続時のフォールバックとしてもう少し長い間隔（例: 30 秒）に緩めるのが望ましいです。さらに
useBrowserAutomationDataはConnectButton（pane ごと）、BrowserAutomationList、SessionConnectModalで個別に呼ばれるため、pane 数が増えると同じクエリに対して複数本のサブスクリプションがサーバ側に張られます。bindingStoreの EventEmitter はsetMaxListeners()の設定がないため、既定の 10 リスナー警告を超える可能性があります。（React Query はクエリを重複排除しますが、subscriptions は共有されません）提案：ポーリング間隔の緩和
const { data: bindings = [] } = electronTrpc.browserAutomation.listBindings.useQuery(undefined, { - refetchInterval: 1000, + // Subscription invalidates on change; keep a long fallback only for + // resilience when the subscription is (re)connecting. + refetchInterval: 30_000, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts` around lines 27 - 36, The current useBrowserAutomationData hook both polls via electronTrpc.browserAutomation.listBindings.useQuery (refetchInterval: 1000) and opens a subscription with electronTrpc.browserAutomation.onBindingsChanged.useSubscription, which is redundant and causes multiple server subscriptions when used from ConnectButton, BrowserAutomationList and SessionConnectModal; fix by removing the aggressive 1s polling (delete refetchInterval) or relax it to a long fallback (e.g. 30000) and, more importantly, centralize the onBindingsChanged subscription so only one subscription is created app-wide (move the electronTrpc.browserAutomation.onBindingsChanged.useSubscription call out of useBrowserAutomationData into a single top-level component/provider) to avoid creating many server subscriptions and hitting bindingStore EventEmitter listener limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 27-79: The EventEmitter in BindingStore can hit the 10-listener
default limit; inside the BindingStore class (where emitter is declared) call
this.emitter.setMaxListeners(0) (or setMaxListeners(n) with a sensible higher
limit) right after creating the emitter to avoid MaxListenersExceededWarning;
update both places where an EventEmitter is created for browser automation (the
emitter field in class BindingStore and the other emitter instance referenced
around the other occurrence) so each uses setMaxListeners(...) and keep
onChange/remove/emitChange logic unchanged.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 51-72: The selectedSessionId can remain stale when its session is
removed from sessions; update the useEffect that runs on modal open to also
detect when selectedSessionId is truthy but no longer present (i.e., session ===
null) and in that case call setSelectedSession with the computed fallback
(currentBinding ?? sessions[0]?.id ?? null); reference selectedSessionId,
sessions, session, currentBinding, paneId, useEffect and setSelectedSession so
the change is done inside the existing effect branch that guards for open and
paneId and uses fallback selection.
---
Nitpick comments:
In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts`:
- Around line 48-75: The mapping for todoSessions assumes every session is
Claude by hardcoding provider = "Claude" and only reading mcpStatus.claudeReady;
add a TODO marker and update instructions so future Phase 2 Codex support is
obvious: leave the provider assignment but add a comment like "// TODO(phase2):
detect Codex workers -> provider = 'Codex' and use mcpStatus.codexReady" near
the provider/mcpStatus logic (referencing provider, mcpStatus, todoSessions),
and ensure SessionConnectModal/codexConfigPath consumers will be able to rely on
provider === "Codex" and mcpStatus.codexReady once implemented.
- Around line 27-36: The current useBrowserAutomationData hook both polls via
electronTrpc.browserAutomation.listBindings.useQuery (refetchInterval: 1000) and
opens a subscription with
electronTrpc.browserAutomation.onBindingsChanged.useSubscription, which is
redundant and causes multiple server subscriptions when used from ConnectButton,
BrowserAutomationList and SessionConnectModal; fix by removing the aggressive 1s
polling (delete refetchInterval) or relax it to a long fallback (e.g. 30000)
and, more importantly, centralize the onBindingsChanged subscription so only one
subscription is created app-wide (move the
electronTrpc.browserAutomation.onBindingsChanged.useSubscription call out of
useBrowserAutomationData into a single top-level component/provider) to avoid
creating many server subscriptions and hitting bindingStore EventEmitter
listener limits.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx`:
- Around line 42-47: The needsSetupCount currently counts MCP-missing across all
sessions but should be scoped to this workspace like connectedCount; update the
calculation for needsSetupCount to only consider sessions related to this
workspace's browserPanes (e.g., sessions whose pane or candidate matches an id
in browserPanes) and still check s.mcpStatus === "missing" using the existing
symbols (browserPanes, sessions, bindingsByPane, needsSetupCount,
connectedCount); if the global aggregation was intended instead, add a one-line
comment above the needsSetupCount declaration explaining that it's intentionally
global.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 111-123: handleDisconnect currently disconnects but does not close
the modal, while handleConnect calls onOpenChange(false); decide and implement
consistent behavior: either close the modal on successful disconnect by calling
onOpenChange(false) inside handleDisconnect after removeBinding.mutateAsync
succeeds, or update the modal footer when currentBinding becomes null (e.g., set
a local footer state or derive footer text from currentBinding and render
"Disconnected — pick another session or close"); update the Footer rendering
logic to read that state/derived text so UI reflects the new disconnected state.
Ensure to reference and use onOpenChange, handleDisconnect, handleConnect, and
currentBinding when making the change.
- Around line 284-480: Split the large SessionConnectModal file by extracting
the local subcomponents SessionCard, Tag, ReadyPanel, DetailItem, and SetupPanel
into their own component folders (e.g. SessionCard/SessionCard.tsx,
SetupPanel/SetupPanel.tsx, ReadyPanel/ReadyPanel.tsx) with a barrel index.ts
export for each, move any related helper usage (e.g. getSnippetForSession) or
prop types they need into shared exports or pass them as props, update the main
SessionConnectModal to import these components instead of defining them inline,
and ensure all props and callbacks (like onSelect, onCopy, mcpConfigPath,
session, paneName, reassigning, previousPaneName) are preserved and typed the
same so behavior remains identical.
In `@apps/desktop/src/renderer/stores/browser-automation.ts`:
- Around line 82-95: The function formatRelativeTime can return negative
relative strings when ts is slightly in the future; update formatRelativeTime to
clamp negative differences (e.g., compute diffSec = Math.max(0,
Math.round((Date.now() - ts) / 1000))) or treat any diffSec < 0 as 0/“just now”
so you never produce "-1s ago"; modify the logic in formatRelativeTime
accordingly and keep the existing branch ordering (just now, seconds, minutes,
hours, days).
- Around line 5-13: The AutomationSession interface currently defines provider
and kind as `"Claude" | "Codex" | string` and `"Terminal" | "Chat" | string`,
which collapses to plain string and loses literal type benefits; update
AutomationSession so provider is a strict union of known literals (e.g.,
"Claude" | "Codex") and kind is a strict union of known literals (e.g.,
"Terminal" | "Chat"), and if you need to represent unknown/third‑party values
add an explicit discriminated variant (for example Provider = "Claude" | "Codex"
| { custom: string } and likewise for kind) so switch/case and type narrowing on
provider and kind remain exhaustive and type-safe while still supporting
extensibility.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c223f114-91cb-4ea6-89e7-0039a9d8ff2e
📒 Files selected for processing (14)
apps/desktop/src/lib/trpc/routers/browser-automation/index.tsapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/main/lib/todo-daemon/client.tsapps/desktop/src/main/todo-agent/daemon-bridge.tsapps/desktop/src/main/todo-agent/debug.tsapps/desktop/src/main/todo-agent/runtime-config.tsapps/desktop/src/main/todo-agent/trpc-router.tsapps/desktop/src/renderer/features/todo-agent/TodoModal/TodoModal.tsxapps/desktop/src/renderer/hooks/useBrowserAutomationData.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/ConnectButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxapps/desktop/src/renderer/stores/browser-automation.ts
✅ Files skipped from review due to trivial changes (6)
- apps/desktop/src/main/todo-agent/runtime-config.ts
- apps/desktop/src/main/todo-agent/trpc-router.ts
- apps/desktop/src/main/lib/todo-daemon/client.ts
- apps/desktop/src/main/todo-agent/daemon-bridge.ts
- apps/desktop/src/main/todo-agent/debug.ts
- apps/desktop/src/renderer/features/todo-agent/TodoModal/TodoModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx
Owner
Author
|
@codex review
|
|
To use Codex here, create an environment for this repo. |
- BindingStore: call setMaxListeners(0) so many browser panes don't trip Node's default 10-listener warning. (coderabbit Minor) - SessionConnectModal: clear stale selectedSessionId when the session drops out of the live list so the right-column detail view is never empty with a truthy selection. (coderabbit Minor)
Owner
Author
|
@codex review
|
|
To use Codex here, create an environment for this repo. |
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2605a118
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- useBrowserAutomationData takes an `enabled` flag. Sessions and MCP status only poll while the Connect dialog / list view is open; the binding query relies on the push subscription. Poll intervals relaxed to 15s / 30s from 5s / 10s. (codex P2) - SessionConnectModal: fallback selection only keeps currentBinding when it still points at a live session, otherwise picks sessions[0]. Stops the modal from opening with a truthy-but-dead selection. (codex P3) - ConnectButton now only consumes the binding data and renders a static "Connected" label instead of the full session name + provider.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 95-99: detectSupersetBrowserMcp currently uses a simple substring
check (contents.includes("superset-browser")) which yields false positives from
comments or inactive settings; update detectSupersetBrowserMcp(filePath: string)
to parse the file and verify an active configuration entry instead: if the file
is JSON, JSON.parse the contents and return Boolean(parsed.mcpServers &&
parsed.mcpServers["superset-browser"]); if the file is TOML (or non-JSON),
detect an uncommented section/header or key for superset-browser (e.g., a TOML
table like [mcpServers."superset-browser"] or an uncommented key) using a regex
that ignores lines starting with # or // and only matches active lines; keep
readFileSync(filePath, "utf8") to load contents, but replace the naive includes
check with these structured validations inside detectSupersetBrowserMcp.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 184-188: The empty-state message in SessionConnectModal is
misleading because useBrowserAutomationData converts only todoAgent.listAll
(Claude terminal) sessions into sessions; update the displayed copy in the
sessions.length === 0 branch to instruct users to start a TODO-Agent (Claude)
session only (remove the "open a chat pane" suggestion) so the message reflects
the actual connectable session sources (change the string used in the <div>
inside SessionConnectModal where sessions is checked).
- Around line 329-338: The button in SessionConnectModal (in the
SessionConnectModal component where the <button> uses onSelect and the
isSelected variable to style selection) only signals selection visually; add an
accessibility state by adding aria-pressed set to the boolean isSelected on that
button (i.e., aria-pressed={isSelected}) so screen readers can announce which
session is selected while keeping the existing onSelect handler and className
logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dbf6785-08c7-41b5-8ba9-f72c39441601
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/browser-automation/index.tsapps/desktop/src/renderer/hooks/useBrowserAutomationData.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/ConnectButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/stores/browser-automation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/ConnectButton.tsx
…e/codex
- packages/local-db: add browser_automation_bindings table + migration
- browserAutomation router: use localDb for bindings so they survive restart
(terminal daemon re-attaches panes, binding must survive too)
- Add listTerminalAgentSessions that walks each live PTY's process tree and
returns the ones running claude or codex. Hook merges them into the session
list keyed by `terminal:<paneId>` so bindings self-heal across shell
re-spawns in the same pane.
- setBinding takes sessionKind ("todo-agent" | "terminal") for future
dispatch logic on the consumer side.
Addresses:
- Persist across restarts (user report)
- Bind to claude/codex started in an ordinary terminal tab (user report)
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b85c59e173
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SessionConnectModal and BrowserAutomationList are mounted at parent level
so they were polling sessions / MCP status / terminal process trees even
while closed. Pass { enabled: open } so background polling only runs while
the dialog is actually visible. (codex P1 / P2)
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0765ed9887
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sync - Add browserAutomation.listBindingLiveness: single main-process query that joins bindings with live status (todo-agent whitelist + terminal process tree). ConnectButton uses it instead of the heavy session list, and the badge now shows "Session ended" with an amber state when the bound worker has exited. (codex P2) - Move the onBindingsChanged subscription into a dedicated `useBrowserBindingsSync` hook mounted once in ContentView, removing the per-pane fan-out. (codex P2)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/desktop/src/lib/trpc/routers/browser-automation/index.ts (1)
143-147:⚠️ Potential issue | 🟡 MinorMCP 設定は文字列検索ではなく有効な entry として判定してください。
コメントアウト済み TOML や JSON 内のメモ文字列だけでも ready 扱いになります。Claude は JSON の
mcpServers["superset-browser"]、Codex は未コメントの TOML section/key を見る形に寄せてください。🛡️ 修正案
-function detectSupersetBrowserMcp(filePath: string): boolean { +function readConfig(filePath: string): string | null { try { - const contents = readFileSync(filePath, "utf8"); - return contents.includes("superset-browser"); + return readFileSync(filePath, "utf8"); } catch { - return false; + return null; } } + +function detectClaudeSupersetBrowserMcp(filePath: string): boolean { + const contents = readConfig(filePath); + if (!contents) return false; + try { + const parsed = JSON.parse(contents) as { + mcpServers?: Record<string, unknown>; + }; + return Boolean(parsed.mcpServers?.["superset-browser"]); + } catch { + return false; + } +} + +function detectCodexSupersetBrowserMcp(filePath: string): boolean { + const contents = readConfig(filePath); + if (!contents) return false; + return /^\s*\[(?:mcpServers|mcp_servers)\.(?:"superset-browser"|superset-browser)\]\s*$/m.test( + contents, + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts` around lines 143 - 147, detectSupersetBrowserMcp currently uses a raw substring search which falsely matches commented text; change it to parse the file and detect a real MCP entry. In detectSupersetBrowserMcp, read the file, detect whether it’s JSON or TOML, then: for JSON parse and check for a real mcpServers["superset-browser"] key; for TOML parse and check for an un-commented section/key corresponding to superset-browser (e.g. mcpServers.superset-browser or equivalent), ensuring commented lines are ignored; return true only when the parsed structure contains a valid entry. Use the function name detectSupersetBrowserMcp to locate the code and add robust error handling for parse failures.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (2)
334-342:⚠️ Potential issue | 🟡 Minor選択状態を
aria-pressedでも伝えてください。選択状態が現在は色だけなので、支援技術ではどの session が選択中か分かりにくいです。過去コメントと同じ指摘です。
修正案
<button type="button" + aria-pressed={isSelected} onClick={onSelect} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 334 - 342, The button that renders the session selector currently conveys selection only via color; update the element in SessionConnectModal (the button with onClick and className using isSelected) to include an accessible state by adding aria-pressed={isSelected} (or aria-pressed={Boolean(isSelected)}) so assistive tech can detect which session is selected; ensure the prop uses the same isSelected variable used for className so the visual and ARIA states remain synchronized.
64-77:⚠️ Potential issue | 🟠 Major
setSelectedSessionの無操作更新を防いでください。Line 77 では
fallbackが現在のselectedSessionIdと同じ値であっても Zustand state を更新しています。sessions依存が再評価されると、モーダル表示中に不要な再レンダリングループが発生します。修正案
const fallback = (bindingIsLive ? currentBinding : null) ?? sessions[0]?.id ?? null; - setSelectedSession(fallback); + if (fallback !== selectedSessionId) { + setSelectedSession(fallback); + }加えて、
SessionCardコンポーネント（line 334）の<button>にaria-pressed={isSelected}属性を追加し、アクセシビリティを改善してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 64 - 77, The effect updates Zustand state even when fallback equals the current selectedSessionId causing unnecessary re-renders; in the useEffect that computes fallback (references: selectedSessionId, sessions, currentBinding, setSelectedSession) add an identity check and only call setSelectedSession(fallback) if fallback !== selectedSessionId (taking care of null/undefined equality), so no-op updates are avoided. Also update the SessionCard button (reference: SessionCard component) to include aria-pressed={isSelected} to improve accessibility.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx (1)
143-152:Metricprops は interface に切り出してください。object shape の型定義は project 方針に合わせて
interfaceに寄せると一貫します。♻️ 修正案
-function Metric({ label, value }: { label: string; value: number }) { +interface MetricProps { + label: string; + value: number; +} + +function Metric({ label, value }: MetricProps) {As per coding guidelines,
Prefer interface for defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx` around lines 143 - 152, The Metric component currently types its props inline; extract the props into a named interface (e.g., interface MetricProps { label: string; value: number }) and update the Metric signature to use MetricProps (function Metric({ label, value }: MetricProps) { ... }) to comply with the project guideline of preferring interface for object shapes; keep the interface local to the file (or export it if used elsewhere) and ensure any references or tests use the new MetricProps type.apps/desktop/src/renderer/hooks/useBrowserBindingsSync.ts (1)
11-15:invalidate()の Promise を await してください。キャッシュ無効化の失敗が未処理になります。
onData内でまとめて await しておくと、Promise handling の方針にも沿います。♻️ 修正案
electronTrpc.browserAutomation.onBindingsChanged.useSubscription(undefined, { - onData: () => { - utils.browserAutomation.listBindings.invalidate(); - utils.browserAutomation.listBindingLiveness.invalidate(); + onData: async () => { + await Promise.all([ + utils.browserAutomation.listBindings.invalidate(), + utils.browserAutomation.listBindingLiveness.invalidate(), + ]); }, });As per coding guidelines,
Use async/await for all promise handling in JavaScript/TypeScript code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserBindingsSync.ts` around lines 11 - 15, The onData handler passed to electronTrpc.browserAutomation.onBindingsChanged.useSubscription should await the invalidate() calls so their revalidation failures are handled; make the onData callback async and await Promise.all([utils.browserAutomation.listBindings.invalidate(), utils.browserAutomation.listBindingLiveness.invalidate()]) (or await each) inside that async function to ensure both invalidations are awaited and errors surface; update the onData declaration in useBrowserBindingsSync.ts accordingly.packages/local-db/src/schema/browser-automation-bindings.ts (1)
20-23:sessionKindも TypeScript 側で union に寄せてください。現状は select 結果上
stringになり、router 側の"terminal"/"todo-agent"分岐と型が噛み合いません。♻️ 修正案
+export type BrowserAutomationSessionKind = "todo-agent" | "terminal"; + export const browserAutomationBindings = sqliteTable( "browser_automation_bindings", { paneId: text("pane_id").primaryKey(), sessionId: text("session_id").notNull(), - sessionKind: text("session_kind").notNull().default("todo-agent"), + sessionKind: text("session_kind") + .notNull() + .$type<BrowserAutomationSessionKind>() + .default("todo-agent"), connectedAt: integer("connected_at").notNull(), }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/src/schema/browser-automation-bindings.ts` around lines 20 - 23, The sessionKind column is currently declared as text(...) so select results are typed as string; create a proper enum/union so TypeScript infers "terminal" | "todo-agent". Define a TS union type (e.g. type SessionKind = "terminal" | "todo-agent") and replace the column declaration sessionKind: text("session_kind").notNull().default("todo-agent") with the schema enum helper your ORM provides (e.g. sqliteEnum/textEnum/sqlEnum(...) or enums(...)) listing ["terminal","todo-agent"] so the resulting column and selects are typed as SessionKind and match the router branches.apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts (2)
25-29: Hook options は namedinterfaceにしてください。この options object は公開 hook の引数なので、inline type より
UseBrowserAutomationDataOptionsのような interface に切り出す方が規約に沿います。修正案
+interface UseBrowserAutomationDataOptions { + enabled?: boolean; +} + export function useBrowserAutomationData({ enabled = true, -}: { - enabled?: boolean; -} = {}) { +}: UseBrowserAutomationDataOptions = {}) {As per coding guidelines,
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts` around lines 25 - 29, The hook useBrowserAutomationData should accept a named interface rather than an inline type for its options; create an interface named UseBrowserAutomationDataOptions describing { enabled?: boolean } and replace the inline parameter type in the useBrowserAutomationData signature with that interface (and update the default param destructuring to use the interface as the function parameter type). Ensure any exports or JSDoc reflect the new UseBrowserAutomationDataOptions name so callers and tests import/consume the typed options consistently.
32-60: 空配列のデフォルト値を安定化してください。
data: todoSessions = []のような destructuring default は、未取得時に render ごとに新しい配列を作ります。useMemoの依存が毎回変わり、sessions/bindingsByPaneを見る downstream effect が不要に再実行されます。修正案
import { useTabsStore } from "renderer/stores/tabs/store"; +const EMPTY_TODO_SESSIONS = []; +const EMPTY_TERMINAL_AGENTS = []; +const EMPTY_BINDINGS = []; + /** * Aggregates browser-automation session and binding data from the @@ - const { data: todoSessions = [], refetch: refetchSessions } = + const { data: todoSessions = EMPTY_TODO_SESSIONS, refetch: refetchSessions } = electronTrpc.todoAgent.listAll.useQuery(undefined, { @@ - const { data: terminalAgents = [] } = + const { data: terminalAgents = EMPTY_TERMINAL_AGENTS } = electronTrpc.browserAutomation.listTerminalAgentSessions.useQuery( @@ - const { data: bindings = [] } = + const { data: bindings = EMPTY_BINDINGS } = electronTrpc.browserAutomation.listBindings.useQuery(undefined, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts` around lines 32 - 60, Destructuring defaults like "data: todoSessions = []", "data: terminalAgents = []", and "data: bindings = []" create new arrays on each render causing downstream useMemo/useEffect deps to change; instead remove those inline defaults and coalesce the query results against stable frozen/memoized empty arrays (e.g., create a single frozen EMPTY_ARRAY or use useMemo(() => [], []) and use that for fallback) when assigning todoSessions, terminalAgents, bindings (and similarly handle mcpStatus if needed), so the variables used by sessions / bindingsByPane remain referentially stable across renders.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx (1)
300-310: Props の object shape はinterfaceに切り出してください。
SessionCard/Tag/ReadyPanel/DetailItem/SetupPanelの props が inline object type になっています。再利用・差分追跡しやすいように named interface に寄せるのがこのリポジトリの規約に合います。修正例
+interface SessionCardProps { + session: AutomationSession; + isSelected: boolean; + assignedElsewherePaneName: string | null; + onSelect: () => void; +} + function SessionCard({ session, isSelected, assignedElsewherePaneName, onSelect, -}: { - session: AutomationSession; - isSelected: boolean; - assignedElsewherePaneName: string | null; - onSelect: () => void; -}) { +}: SessionCardProps) {As per coding guidelines,
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.Also applies to: 374-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx` around lines 300 - 310, Several components (SessionCard, Tag, ReadyPanel, DetailItem, SetupPanel) use inline object-type props; extract each inline type into a named interface (e.g., interface SessionCardProps { ... }) and replace the inline annotations with the interface names in the function/component signatures; ensure exported or locally-scoped interfaces match existing property names and types (AutomationSession, boolean, string | null, callbacks) and update all occurrences within the file (including the other ranges mentioned) to use the new interfaces to follow the repository convention of using interfaces for object shapes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 174-198: The Promise.all over sessions can reject if
getProcessTree or getProcessName throws for a stale PID; wrap each session's
work in its own try/catch (inside the sessions.map async callback) or use
Promise.allSettled so one failing session doesn't reject the whole batch; catch
errors around getProcessTree/getProcessName and skip that session while
optionally logging the error; specifically modify the async callback that calls
getProcessTree(s.pid) and the inner Promise.all of getProcessName calls (and the
names.find logic) to handle/recover from per-session failures so
listTerminalAgentSessions returns other successful sessions.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx`:
- Around line 44-49: connectedCount currently counts any pane with an existing
binding (bindingsByPane[p.id]) which includes stale bindings; change the logic
to only count a pane as connected when it has both a binding and a corresponding
active/live session (use sessions.some(...) to find a session linked to the pane
id and that is not in a terminal/missing state). Update the connectedCount
computation (and the analogous logic between lines 73-110) to require
bindingsByPane[p.id] && sessions.some(s => s.paneId === p.id && /* check not
terminal: e.g. s.mcpStatus !== 'missing' or s.status !== 'ended' */) so
stale/ended sessions are excluded from the connected metric.
In `@packages/local-db/drizzle/0067_add_browser_automation_bindings.sql`:
- Around line 1-6: The session_kind column in browser_automation_bindings allows
arbitrary values; add a DB-level constraint to enforce the allowed domain (e.g.,
restrict session_kind to the expected enum values such as 'todo-agent' and
'terminal') by altering the CREATE TABLE to include a CHECK(session_kind IN
(...)) (or an equivalent ENUM type) and update the migration and regenerate the
snapshot so the constraint is persisted; target the browser_automation_bindings
table and the session_kind column when making these changes.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/browser-automation/index.ts`:
- Around line 143-147: detectSupersetBrowserMcp currently uses a raw substring
search which falsely matches commented text; change it to parse the file and
detect a real MCP entry. In detectSupersetBrowserMcp, read the file, detect
whether it’s JSON or TOML, then: for JSON parse and check for a real
mcpServers["superset-browser"] key; for TOML parse and check for an un-commented
section/key corresponding to superset-browser (e.g. mcpServers.superset-browser
or equivalent), ensuring commented lines are ignored; return true only when the
parsed structure contains a valid entry. Use the function name
detectSupersetBrowserMcp to locate the code and add robust error handling for
parse failures.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 334-342: The button that renders the session selector currently
conveys selection only via color; update the element in SessionConnectModal (the
button with onClick and className using isSelected) to include an accessible
state by adding aria-pressed={isSelected} (or
aria-pressed={Boolean(isSelected)}) so assistive tech can detect which session
is selected; ensure the prop uses the same isSelected variable used for
className so the visual and ARIA states remain synchronized.
- Around line 64-77: The effect updates Zustand state even when fallback equals
the current selectedSessionId causing unnecessary re-renders; in the useEffect
that computes fallback (references: selectedSessionId, sessions, currentBinding,
setSelectedSession) add an identity check and only call
setSelectedSession(fallback) if fallback !== selectedSessionId (taking care of
null/undefined equality), so no-op updates are avoided. Also update the
SessionCard button (reference: SessionCard component) to include
aria-pressed={isSelected} to improve accessibility.
---
Nitpick comments:
In `@apps/desktop/src/renderer/hooks/useBrowserAutomationData.ts`:
- Around line 25-29: The hook useBrowserAutomationData should accept a named
interface rather than an inline type for its options; create an interface named
UseBrowserAutomationDataOptions describing { enabled?: boolean } and replace the
inline parameter type in the useBrowserAutomationData signature with that
interface (and update the default param destructuring to use the interface as
the function parameter type). Ensure any exports or JSDoc reflect the new
UseBrowserAutomationDataOptions name so callers and tests import/consume the
typed options consistently.
- Around line 32-60: Destructuring defaults like "data: todoSessions = []",
"data: terminalAgents = []", and "data: bindings = []" create new arrays on each
render causing downstream useMemo/useEffect deps to change; instead remove those
inline defaults and coalesce the query results against stable frozen/memoized
empty arrays (e.g., create a single frozen EMPTY_ARRAY or use useMemo(() => [],
[]) and use that for fallback) when assigning todoSessions, terminalAgents,
bindings (and similarly handle mcpStatus if needed), so the variables used by
sessions / bindingsByPane remain referentially stable across renders.
In `@apps/desktop/src/renderer/hooks/useBrowserBindingsSync.ts`:
- Around line 11-15: The onData handler passed to
electronTrpc.browserAutomation.onBindingsChanged.useSubscription should await
the invalidate() calls so their revalidation failures are handled; make the
onData callback async and await
Promise.all([utils.browserAutomation.listBindings.invalidate(),
utils.browserAutomation.listBindingLiveness.invalidate()]) (or await each)
inside that async function to ensure both invalidations are awaited and errors
surface; update the onData declaration in useBrowserBindingsSync.ts accordingly.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsx`:
- Around line 143-152: The Metric component currently types its props inline;
extract the props into a named interface (e.g., interface MetricProps { label:
string; value: number }) and update the Metric signature to use MetricProps
(function Metric({ label, value }: MetricProps) { ... }) to comply with the
project guideline of preferring interface for object shapes; keep the interface
local to the file (or export it if used elsewhere) and ensure any references or
tests use the new MetricProps type.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsx`:
- Around line 300-310: Several components (SessionCard, Tag, ReadyPanel,
DetailItem, SetupPanel) use inline object-type props; extract each inline type
into a named interface (e.g., interface SessionCardProps { ... }) and replace
the inline annotations with the interface names in the function/component
signatures; ensure exported or locally-scoped interfaces match existing property
names and types (AutomationSession, boolean, string | null, callbacks) and
update all occurrences within the file (including the other ranges mentioned) to
use the new interfaces to follow the repository convention of using interfaces
for object shapes.
In `@packages/local-db/src/schema/browser-automation-bindings.ts`:
- Around line 20-23: The sessionKind column is currently declared as text(...)
so select results are typed as string; create a proper enum/union so TypeScript
infers "terminal" | "todo-agent". Define a TS union type (e.g. type SessionKind
= "terminal" | "todo-agent") and replace the column declaration sessionKind:
text("session_kind").notNull().default("todo-agent") with the schema enum helper
your ORM provides (e.g. sqliteEnum/textEnum/sqlEnum(...) or enums(...)) listing
["terminal","todo-agent"] so the resulting column and selects are typed as
SessionKind and match the router branches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a31a6497-cb67-4a5c-95c2-0d83178fc0f7
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/browser-automation/index.tsapps/desktop/src/renderer/hooks/useBrowserAutomationData.tsapps/desktop/src/renderer/hooks/useBrowserBindingsSync.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserAutomationList/BrowserAutomationList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/ConnectButton/ConnectButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SessionConnectModal/SessionConnectModal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsxpackages/local-db/drizzle/0067_add_browser_automation_bindings.sqlpackages/local-db/drizzle/meta/0067_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/browser-automation-bindings.tspackages/local-db/src/schema/index.tspackages/local-db/src/schema/schema.ts
✅ Files skipped from review due to trivial changes (3)
- packages/local-db/src/schema/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx
- packages/local-db/drizzle/meta/_journal.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be0f062b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
…lures - detectClaudeMcp: JSON.parse + check mcpServers["superset-browser"] is an object with a command/url/args and not disabled. (codex P2) - detectCodexMcp: isolate [mcp_servers.superset-browser] section, skip if disabled=true, require command/url/args. Avoid adding a TOML parser dep just for this single lookup. - detectTerminalAgentSessions: catch terminal-host IPC errors and degrade to an empty result so listBindingLiveness can still serve non-terminal bindings when the daemon is unavailable. (codex P1)
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c967c80ec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
detectClaudeMcp previously only inspected ~/.claude/settings.json, but `claude mcp add` writes to ~/.claude.json. Check both paths (plus project-scope later) so a normally-configured Claude session is correctly reported as MCP-ready. Expose the primary user path in claudeConfigPath so the setup guide points users at the right file. (codex P1)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7c41b1cd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941983c0c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- collectProjectMcpJsonPaths: enumerate every workspace's worktree / mainRepoPath and include <path>/.mcp.json in the Claude MCP detection paths so `claude mcp add -s project` configurations are recognized. (codex P1) - detectCodexMcp: accept quoted TOML header forms `[mcp_servers."superset-browser"]` and `[mcp_servers.'superset-browser']` alongside the bare form. (codex P2)
…connect # Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx
Owner
Author
|
@codex review |
1 similar comment
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6db35639c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
getMcpStatus previously OR'd every workspace's .mcp.json into a single claudeReady boolean, so one configured project made all Claude sessions look ready across projects. The router now returns: - claudeHomeReady: home-scope only - claudeReadyByWorkspaceId: per-workspace .mcp.json probe useBrowserAutomationData resolves each session against its own workspace (home || project) so Connect stays blocked for a session whose workspace has no MCP entry even if a sibling project does. (codex P1)
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0257cf0b94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`claude mcp add` (default local scope) stores MCP server definitions at projects[<cwd>].mcpServers inside ~/.claude.json, not at the top level. detectClaudeMcpInFile now inspects both locations, keyed by each workspace's resolved path. getMcpStatus passes per-workspace paths so a workspace that only has local-scope MCP still reports ready. (codex P1)
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef6bd025c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Snippet / command name: the real bin shipped by packages/desktop-mcp is `desktop-mcp`, not `superset-browser-mcp`. Update both the TOML and the Claude CLI snippet so copy-pasting actually works. (codex P1) - Claude setup: recommend `claude mcp add superset-browser -s local -- desktop-mcp` and explain /mcp-reload, rather than telling users to append a complete JSON document into ~/.claude.json (which would produce invalid JSON). Codex still gets the TOML section-append path since that is mergeable as-is. (codex P2)
Owner
Author
|
@codex review |
Owner
Author
|
@codex review |
3 tasks
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
browser pane から任意の LLM セッションを選んで接続できる UX を v1 workspace の BrowserPane に組み込みました。session 列挙と MCP readiness 判定は本番データで動作します。実 automation bridge (binding を使って外部 MCP から webview を駆動する経路) は本 PR の対象外です。
実装済み
todoAgent.listAllから live session (running / preparing / verifying / waiting) だけを抽出、browserAutomation.getMcpStatusを突き合わせてmcpStatusを解決paneId → sessionIdbinding を in-memory で保持 (listBindings/setBinding/removeBinding/onBindingsChangedsubscription)、~/.claude/settings.jsonと~/.codex/config.tomlを走査して `superset-browser` MCP entry の有無を判定するgetMcpStatustabs.workspaceIdで現在の workspace に属する browser pane のみ表示、Focus は setActiveTab + setFocusedPane で別 tab からもナビゲート本 PR の対象外 (別 PR で対応)
packages/desktop-mcp) がその binding を参照してelectronTrpc.browser.navigate/evaluateJS/screenshotを呼ぶ経路は未配線。MCP 側に `resolvePaneForSession` 相当のツールを足す必要があるapps/desktop/src/renderer/routes/**/v2-workspace/**には手を入れていない (v1 の `WorkspaceView` 配下のみ)Review 対応履歴
すべて resolve 済み。
Test plan
Summary by CodeRabbit
New Features
Chores
Documentation