fix: resolve next.config.js syntax error and scaffold dashboard UI - #68
fix: resolve next.config.js syntax error and scaffold dashboard UI#68archittmittal wants to merge 2 commits into
Conversation
purvanshjoshi
left a comment
There was a problem hiding this comment.
Summary
This is a solid scaffold that correctly resolves #37 (the next.config.js syntax fix and removing || true from CI are both right — the UI build now genuinely passes) and lays out a clean App Router structure (globals.css, layout.tsx, page.tsx) with a genuinely good-looking dashboard. However, I found three functional bugs that will break the dashboard at runtime, so I can't approve in its current state.
Blocking issues
-
/api/sessionsresponse is not unwrapped (page.tsxfetchSessionsList).
The server returns{ "sessions": [...], "total", "page", "limit" }(routers/sessions.pylist_sessions), but the code doessetSessions(data)and checksdata.length/data[0]. This sets the store'ssessionsto the wrapper object instead of an array, sofilteredSessions = sessions.filter(...)in render throws aTypeErrorand the sidebar crashes whenever the API returns data. It also means the auto-select never fires (data.lengthisundefined).
Fix:setSessions(data.sessions)and usedata.sessions.length/data.sessions[0]. -
/api/sessions/{id}/eventsresponse is not unwrapped (handleSelectSession).
The events endpoint returns{ "events": [...] }(routers/events.pylist_session_events), but the code doessetEvents(eventsData), makingeventsan object —events.map(...)in the feed render then crashes.
Fix:setEvents(eventsData.events). -
WS
session_updatefield mismatch (page.tsxwebsocket handler).
The server broadcasts{ "type": "session_update", "session_id", "session": session_data }(ws_manager.pybroadcast_session_update), but the handler readsmessage.meta. Sincemetaisundefined,updateSessionMetasilently becomes a no-op and live session/token updates never reach the UI.
Fix: usemessage.sessioninstead ofmessage.meta.
Should address
-
Hardcoded API/WS URLs.
page.tsxhardcodeshttp://127.0.0.1:8765and derives the WS host fromwindow.location.hostname. The repo already documentsNEXT_PUBLIC_API_URLandNEXT_PUBLIC_WS_URL(README, set indocker-compose.yml) but they're never read. In any deployment where the browser's origin differs from the API host (remote host, Docker host variance), this breaks silently. Please add a small helper that readsprocess.env.NEXT_PUBLIC_API_URL/NEXT_PUBLIC_WS_URLwithhttp://127.0.0.1:8765as a fallback, and use it consistently for both REST and WS (also note REST and WS currently derive hosts differently). -
Agent Call Hierarchy is a flat list, not a DAG.
graphData.edgesis fetched but never rendered, and thereactflowdependency (plus the README's "Agent DAG Graph" claim) is unused. Fine as an incremental scaffold toward the React Flow graph (tracked in #5/#15), but please either render the edges/parent-child relationships now or explicitly mark the panel as a placeholder so the README claim isn't overstated.
Minor (non-blocking)
- The "Active Agents" KPI shows
statsData?.agents?.length— that's a unique-agent count, not "active agents". - Sidebar session cards and event rows are clickable
<div>s; considerrole="button"/<button>with keyboard support for accessibility. next-env.d.tsis normally auto-generated by Next — committing it is fine, just don't edit it manually.
The scaffold direction is good and the rest of the code is clean — happy to re-review once the three blocking bugs (and ideally #4) are addressed.
|
@archittmittal ❌ Do not merge yet — 3 blocking bugs remain: (1) setSessions(data) should be setSessions(data.sessions), (2) setEvents(eventsData.events) missing unwrap, (3) WS handler reads message.meta but the server sends message.session. |
|
Working on the review feedback. Will push fixes shortly. |
Fixes #37
Applied review feedback: