feat: Phase 6 — GET /api/agent/tree + SSE resilience#12
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new /api/agent/tree endpoint for a compact session tree view and improves client-side SSE resilience with exponential backoff and status indicators. Feedback highlights the need to remove side effects from the GET endpoint, consolidate redundant loops, extract duplicated project grouping logic into a shared utility, and debounce tree reloads during unstable connections to prevent excessive network traffic.
| function projectGroupFromDirectory(directory: string): string { | ||
| const marker = '/workspace/' | ||
| const index = directory.indexOf(marker) | ||
| const normalized = index >= 0 ? directory.slice(index + marker.length) : directory.replace(/^\/+/, '') | ||
| const parts = normalized.split('/').filter(Boolean) | ||
| if (parts.length === 0) return 'workspace' | ||
| const bucketPrefixes = new Set(['apps', 'research', 'pypi_lib', 'libs', 'infra', 'skills', 'mcps', 'anal-repo']) | ||
| if (parts.length >= 2 && bucketPrefixes.has(parts[0])) { | ||
| return `${parts[0]}/${parts[1]}` | ||
| } | ||
| return parts[0] | ||
| } |
There was a problem hiding this comment.
The projectGroupFromDirectory function is duplicated from the client-side store (src/client/store/agentStore.ts). To improve maintainability and ensure consistency across the application, consider extracting this logic into a shared utility file.
| const projectByDirectoryKey = new Map<string, { id: string; name: string }>() | ||
| for (const session of sessions) { | ||
| const dirKey = projectGroupFromDirectory(session.directory) | ||
| if (!projectByDirectoryKey.has(dirKey)) { | ||
| const proj = findOrCreateProject(dirKey) | ||
| projectByDirectoryKey.set(dirKey, proj) | ||
| } | ||
| } | ||
|
|
||
| const canvasBySession = new Map( | ||
| getAllCanvasNodes().map((node) => [ | ||
| node.session_id, | ||
| { | ||
| label: node.label, | ||
| pinned: Boolean(node.pinned), | ||
| detached: Boolean(node.detached), | ||
| projectId: node.project_id ?? null, | ||
| }, | ||
| ]), | ||
| ) | ||
|
|
||
| for (const session of sessions) { | ||
| const dirKey = projectGroupFromDirectory(session.directory) | ||
| const proj = projectByDirectoryKey.get(dirKey) | ||
| if (!proj) continue | ||
| const canvas = canvasBySession.get(session.id) | ||
| if (!canvas?.projectId) { | ||
| setCanvasNodeProject(session.id, proj.id) | ||
| if (canvas) { | ||
| canvas.projectId = proj.id | ||
| } else { | ||
| canvasBySession.set(session.id, { label: null, pinned: false, detached: false, projectId: proj.id }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block has two issues:
- Side Effects in GET: The endpoint performs database writes (
findOrCreateProjectandsetCanvasNodeProject). RESTful GET requests should be idempotent and side-effect free. Modifications to the database should ideally happen during session creation or through a dedicated POST/PATCH endpoint. - Efficiency: The
sessionsarray is iterated over twice to resolve projects and update canvas nodes. These operations can be consolidated into a single loop to improve performance.
| if (shouldReloadTree) { | ||
| reloadTree().catch(console.error) | ||
| } |
There was a problem hiding this comment.
Triggering reloadTree() immediately upon every SSE reconnection can lead to excessive network requests if the connection is unstable. Consider implementing a debounced reload or a check to see if the tree state has actually changed before performing a full fetch.
Use title → command → description fallback chain matching existing client-side display logic (SessionPanel, ApprovalQueue).
StatPan
changed the title
StatPan
left a comment
StatPan
left a comment
There was a problem hiding this comment.
Independent review — LGTM
검증한 것
src/server/routes/agent.ts:Promise.allSettled로 sessions/statuses 분리 처리 → listSessions 실패 시 502, listStatuses 실패 시 idle fallback 정상src/server/sse/broadcaster.ts: pending maps add/delete 경로, message fallback chain (title → command → description)이 SessionPanel/ApprovalQueue 기존 로직과 일치AgentCanvas.tsxSSE 재연결: 1s→30s 지수 백오프, onopen에서 retryMs 리셋 + 최초 연결 제외 reloadTree, 배너 상태 전환 정상agent.test.ts9 케이스 + 전체 88/88 passing, CI 녹색- relation filtering:
fork제외 + visibleIds 교차 필터, forkedFrom viagetForkRelationMap정상
Minor (non-blocking)
- pending Maps는 opencode가
replied/rejected를 미발행하면 누적 가능 — 로컬 도구 범위 허용 - 재연결 시 reloadTree는 백오프로 폭주 방지됨
- GET 핸들러 내
setCanvasNodeProjectside effect는 기존tree.ts패턴 유지
스펙 부합, 회귀 없음.
StatPan
left a comment
StatPan
left a comment
There was a problem hiding this comment.
Code Review — Phase 6: GET /api/agent/tree + SSE resilience
Overall: Implementation is solid and the test suite covers the critical paths. A few issues worth flagging for follow-up.
✅ What works well
Promise.allSettledfor parallel fetching with independent fallback onlistStatusesfailure — correct patternpendingPermissionskeyed byrequestId(notsessionId) — handles multiple concurrent permission requests correctlyhasConnectedOnceflag avoids spuriousreloadTreeon initial SSE connect- All 8 test cases pass; fork/filter/fallback/status-override paths are covered
🔴 Issues
1. setCanvasNodeProject side-effect in GET handler (agent.ts:71-79)
A GET endpoint is writing to the DB (setCanvasNodeProject) to auto-assign sessions to projects. This is a side-effect that violates the read-only contract of a supervisor API. If the supervisor polls rapidly, it triggers repeated DB writes on every call. The auto-assignment logic belongs in the POST /api/tree or session creation path, not here.
2. Dead branch in SSE banner style (AgentCanvas.tsx:322)
background: sseStatus === 'disconnected' ? '#1c1917' : '#1c1917',Both branches are identical — the condition is a no-op. Likely a copy-paste artifact. Can be collapsed to just background: '#1c1917'.
3. projectGroupFromDirectory is duplicated 3 times
This function now exists identically in agent.ts, agentStore.ts, and presumably tree.ts. Any change to the bucketing logic must be applied in all 3 places. Should be extracted to a shared util.
🟡 Minor / Follow-up
tsfield redundancy: Each session object has bothupdatedAtandts— both equalsession.time.updated. The root-levelts(snapshot timestamp) is useful; the per-sessiontsis redundant withupdatedAt.- Server vs client MAX_RETRY_MS asymmetry:
broadcaster.tsuses 60s cap,AgentCanvas.tsxuses 30s. Not a bug, but worth aligning for consistency. - Missing test:
needs-answerstatus override (question.asked path) is not covered inagent.test.ts— onlyneeds-permissionis tested. - In-memory state loss on restart:
pendingPermissions/pendingQuestionsMaps are cleared on process restart. Supervisor agents polling after a server restart will see no pending items until the next SSE event. Acceptable for now, worth noting in docs.
Verdict
Accept with follow-up. The GET side-effect (#1) and duplicated util (#3) are the items most worth cleaning up in a follow-on PR. The dead style branch (#2) is trivial. Core functionality and resilience logic are correct.
1 participant
Summary
Test plan