Skip to content

Fix #3: Migrate dashboard UI to React 18 + Vite SPA#4

Merged
rekpero merged 95 commits into
mainfrom
fix/issue-3
Mar 15, 2026
Merged

Fix #3: Migrate dashboard UI to React 18 + Vite SPA#4
rekpero merged 95 commits into
mainfrom
fix/issue-3

Conversation

@rekpero
Copy link
Copy Markdown
Owner

@rekpero rekpero commented Mar 14, 2026

Closes #3

Summary

  • React 18 + Vite 5 frontend in frontend/ with Tailwind CSS 3, React Query v5, Lucide React, and date-fns
  • Full component tree preserving the existing dark color palette:
    • Layout: Header, WorkspaceSwitcher, TabNav
    • Metrics: MetricsBar, MetricCard (7-card grid)
    • Agents: ActiveAgents, AgentCard, AgentLogViewer, AgentStatusBadge
    • Issues: IssueQueue, IssueStatusBadge with Retry button for needs_human
    • PRs: PRTracker, ReviewThreads
    • Modals: AddWorkspaceModal, WorkspaceSettingsModal, EnvEditor
    • UI primitives: Card, Badge, Button, Modal, Spinner, EmptyState
  • WorkspaceContext persists selectedWorkspaceId to localStorage
  • SPA catch-all route added to dashboard.py (registered after all /api/* routes and static mount) so React client-side navigation works on direct URL loads
  • build-ui command added to run.sh (cd frontend && npm install && npm run build), integrated into the install flow
  • Vite config sets build.outDir = '../orchestrator/static' and emptyOutDir = true — build artifacts go directly into orchestrator/static/
  • .gitignore updated with frontend/node_modules/ and frontend/dist/

Test plan

  • cd frontend && npm run dev — hot-reload dev server at http://localhost:5173 proxying /api to FastAPI
  • npm run build — outputs orchestrator/static/index.html + assets/
  • Start orchestrator with python -m orchestrator.main → open http://localhost:8420 — React app renders
  • All 7 metric cards show correct values (or when empty)
  • Agent cards expand with log viewer, only fetching logs when expanded
  • Issue queue shows Retry button for needs_human issues
  • Add workspace modal creates a new workspace and selects it
  • Workspace settings modal opens for selected workspace
  • http://localhost:8420/ in a fresh tab loads the React app (SPA catch-all works)

🤖 Generated with Claude Code

- Add React 18 + Vite 5 frontend in frontend/ directory with Tailwind CSS,
  React Query v5, Lucide React icons, and date-fns
- Implement full component tree: layout (Header, WorkspaceSwitcher, TabNav),
  metrics (MetricsBar, MetricCard), agents (AgentCard, AgentLogViewer),
  issues (IssueQueue), PRs (PRTracker), modals (AddWorkspace, WorkspaceSettings,
  EnvEditor), and shared UI primitives (Card, Badge, Button, Modal, Spinner)
- Preserve existing dark color palette via CSS custom properties in tokens.css
- Add SPA catch-all route in dashboard.py (registered after all /api/* routes)
  so React client-side navigation works on direct URL loads
- Add build-ui command to run.sh (cd frontend && npm install && npm run build)
  and integrate it into the install flow
- Update .gitignore to exclude frontend/node_modules/ and frontend/dist/
- Build outputs to orchestrator/static/ via vite.config.js outDir setting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 6 potential bugs in this PR.

high: 3 | medium: 2 | low: 1

The PR introduces a React frontend dashboard with several concrete bugs: a log viewer that ignores its pagination cursor and re-fetches all events on every poll, a PR link pointing to the wrong URL, a settings form that can overwrite workspace data with empty strings, silent error swallowing in the env editor, a potential interval leak on agent status transitions, and a backend SPA fallback that can 500 when the frontend bundle is absent.

Comment thread frontend/src/components/agents/AgentLogViewer.jsx
Comment thread frontend/src/components/prs/PRTracker.jsx
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx Outdated
Comment thread frontend/src/components/agents/AgentCard.jsx Outdated
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
Comment thread orchestrator/dashboard.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 1 | low: 2

All six previously reported bugs have been fixed in this revision. Three new bugs were found: log events are replaced rather than accumulated on each poll (losing history), log polling runs unconditionally even for terminal agents, and workspace update API errors are silently discarded with no user feedback.

Comment thread frontend/src/components/agents/AgentLogViewer.jsx
Comment thread frontend/src/components/agents/AgentLogViewer.jsx
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx Outdated
- AgentLogViewer: accumulate events with ID-based deduplication instead of replacing on each poll
- AgentLogViewer: gate polling on isRunning prop so polling stops when agent is done
- WorkspaceSettingsModal: add onError handler to update mutation to show error feedback to user

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

medium: 2 | low: 2

All three previously reported bugs (log event accumulation, unconditional polling, and silent update errors) are now fixed. Four new bugs were found: completed-agent logs are never fetched due to enabled: isRunning fully disabling the query; the since cursor in the React Query key causes spurious loading states on each poll; the delete-workspace mutation silently ignores errors; and the updateError state is not cleared when switching workspaces.

Comment thread frontend/src/components/agents/AgentLogViewer.jsx Outdated
Comment thread frontend/src/components/agents/AgentLogViewer.jsx Outdated
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx
- AgentLogViewer: allow initial fetch for completed agents by removing
  `enabled: isRunning` gate; polling interval still gated on isRunning
- AgentLogViewer/useAgentLogs: remove `since` from query key to prevent
  spurious loading states on every poll that returns new events
- WorkspaceSettingsModal: add onError handler to delete mutation so
  failures surface to the user instead of being silently swallowed
- WorkspaceSettingsModal: clear updateError when selected workspace
  changes to prevent stale error messages from previous workspace

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

low: 1

All four previously reported bugs are fixed. One new low-severity issue was introduced by the fix for bug PRRT_kwDORZvToM50UOIh: the setUpdateError(null) call inside useEffect([workspace]) can clear an active error message whenever the workspace's server data changes (not just when the user switches to a different workspace).

Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 1 | low: 2

Three bugs found: a stale-closure issue in useAgentLogs where the since cursor is missing from the React Query key (causing the incremental log viewer to never fetch new events past the initial window), a file-upload handler that destructively replaces all env rows instead of merging, and a missing cache invalidation after saving env vars despite useQueryClient being called. The previously reported WorkspaceSettingsModal bug (effect clearing errors on any workspace refetch) is fixed — the dependency is now correctly [workspace?.id].

Comment thread frontend/src/hooks/useAgents.js
Comment thread frontend/src/components/modals/EnvEditor.jsx
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
- Include `since` in useAgentLogs query key so cursor advances trigger fresh fetches
- Merge uploaded .env file rows with existing entries instead of replacing them
- Invalidate env query cache after successful saveEnv call

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

medium: 1 | low: 1

Two new bugs found: PRTracker uses a non-unique React list key (pr_number alone) that can cause incorrect DOM reconciliation in multi-repo or all-workspace views; EnvEditor's post-save cache invalidation targets a query key that no hook is registered under, making it a no-op. All three previously reported bugs (missing since in queryKey, file-upload replacing rows instead of merging, and unused queryClient) have been fixed in this PR.

Comment thread frontend/src/components/prs/PRTracker.jsx Outdated
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
- PRTracker: use composite key `${pr.github_repo}-${pr.pr_number}` to prevent duplicate React keys across repos
- EnvEditor: remove no-op invalidateQueries and use fetchCounter state to trigger re-fetch after save

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 5 potential bugs in this PR.

high: 1 | medium: 1 | low: 3

Five bugs found across four files: a high-severity polling feedback loop caused by including a mutable cursor in a React Query key, a medium-severity fetch race condition from missing effect cleanup, and three low-severity issues (index-based React key, mismatched-quote stripping in env parsing, and a missing preventDefault on a hash link). Both previously reported open bugs are resolved: the PR list key is now a correct composite key, and the EnvEditor has been rewritten without the stale invalidateQueries call.

Comment thread frontend/src/hooks/useAgents.js Outdated
Comment thread frontend/src/components/modals/EnvEditor.jsx
Comment thread frontend/src/components/prs/ReviewThreads.jsx Outdated
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
Comment thread frontend/src/components/agents/AgentCard.jsx Outdated
- Remove `since` from React Query key in useAgentLogs to prevent polling feedback loop
- Add cancellation flag to EnvEditor useEffect to prevent race condition on rapid file switches
- Fix parseEnvText to use balanced quote matching regex instead of independent quote stripping
- Use stable key (thread.id or path-line composite) instead of array index in ReviewThreads
- Add preventDefault to AgentCard issue anchor to prevent page scroll on click

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 1 | low: 2

All five previously-reported bugs have been fixed. Three new issues were found: a stale closure in the log-polling hook means the since cursor is never advanced so every poll re-fetches all logs from the start; env-editor rows use array index as React key risking stale UI after row deletion; and the workspace settings form won't reinitialize if workspace data is updated while the modal is open.

Comment thread frontend/src/hooks/useAgents.js Outdated
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx Outdated
- useAgentLogs: hold cursor in useRef so queryFn always reads the latest
  value; expose cursorRef from the hook so AgentLogViewer can advance it
  without triggering a key change
- AgentLogViewer: remove since state, update cursorRef.current directly
  after each successful fetch
- EnvEditor: assign stable crypto.randomUUID() id to each row at parse/add
  time and use it as the React key instead of array index
- WorkspaceSettingsModal: extend useEffect dependency array to include
  individual workspace fields (name, repo_url, base_branch) so form
  reinitialises when those fields change while the modal is open

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 1 | low: 2

All three previously reported bugs have been fixed in this revision (stale cursor closure via useRef, array-index React keys via crypto.randomUUID(), and incomplete useEffect dependency array). Three new bugs were found: the SPA catch-all route silently swallows unknown API 404s by returning HTML with HTTP 200, a shared mutation isPending flag incorrectly disables all Retry buttons in IssueQueue when any single update is in-flight, and the WorkspaceSettingsModal's confirmDelete state persists across open/close cycles causing the delete confirmation prompt to appear immediately on re-open.

Comment thread orchestrator/dashboard.py
Comment thread frontend/src/components/issues/IssueQueue.jsx Outdated
Comment thread frontend/src/components/modals/WorkspaceSettingsModal.jsx
- dashboard.py: guard spa_fallback against api/* paths, returning 404 instead of index.html
- IssueQueue.jsx: track per-row retry state with retryingIssue to avoid disabling all Retry buttons during a single mutation
- WorkspaceSettingsModal.jsx: reset confirmDelete when modal closes via useEffect on open prop

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 2 | low: 1

The PR fixes the three previously reported bugs (SPA 200-on-API-path, shared Retry button loading state, and confirmDelete state leak) and introduces several new features. Three new bugs were found: a narrow edge case in the SPA guard where /api without a trailing slash bypasses the API-path check and serves HTML instead of a 404, a state accumulation issue where agent log entries from a prior agentId bleed into the newly selected agent's view, and a file input not being cleared after upload which prevents re-uploading the same file.

Comment thread orchestrator/dashboard.py
Comment thread frontend/src/hooks/useAgents.js
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

high: 1

All three previously reported bugs are fixed in this PR. One new high-severity bug is introduced: the SPA catch-all route intercepts browser requests for Vite-built assets at /assets/... (no static mount covers that path) and returns index.html instead of the JS/CSS bundles, breaking the frontend in production.

Comment thread orchestrator/dashboard.py
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

high: 1 | medium: 1

The PR adds an /assets static mount and SPA catch-all to serve a Vite-built React frontend. Two bugs were found: the new /assets StaticFiles mount will crash the server on startup if the frontend hasn't been built, and a race condition in the agent-log cursor reset can cause the wrong cursor to be used on the first fetch when switching between agents. The previously-reported SPA catch-all/Vite asset interception bug has been fixed.

Comment thread orchestrator/dashboard.py
Comment thread frontend/src/hooks/useAgents.js Outdated
- dashboard.py: pass check_dir=False to StaticFiles for /assets mount so
  the server doesn't crash at startup when the frontend hasn't been built yet
- useAgents.js: reset cursorRef synchronously during render when agentId
  changes (using prevAgentIdRef comparison) to prevent TanStack Query from
  firing the first fetch with the previous agent's stale cursor
- AgentLogViewer.jsx: remove redundant cursorRef.current = 0 from effect
  since the hook now handles the reset synchronously before the query fires

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

medium: 2

Two new bugs were found: the root / route in the backend can 500 when the frontend is not built (unlike the new SPA fallback which gracefully returns 404), and the agent list pagination offset is not reset when the user switches workspaces. Both previously reported bugs are now fixed: the /assets StaticFiles mount no longer crashes the server at startup (check_dir=False was added), and the agent-log cursor is now reset synchronously during render (not in a racing useEffect) when agentId changes.

Comment thread orchestrator/dashboard.py Outdated
Comment thread frontend/src/components/agents/ActiveAgents.jsx
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

Both previously reported bugs (missing file-existence check in index() and missing offset reset on workspace switch) have been fixed. One new medium-severity bug was found: synchronous ref mutation during render in useAgents.js violates React's render-purity contract and can cause incorrect log-cursor behavior under React StrictMode, which is active in this application.

Comment thread frontend/src/hooks/useAgents.js
Move ref mutations in useAgentLogs from render phase into a useEffect
to comply with React's render-purity contract. The previous synchronous
mutation during render broke under StrictMode's double-render: the second
pass found prevAgentIdRef already updated so cursorRef was never reset,
causing the log viewer to fetch from a stale cursor after agent switches.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

The StrictMode render-purity bug is fixed, but a new race condition remains: TanStack Query schedules its initial queryFn call as a microtask (before React's useEffect macrotasks run), so the cursor is not guaranteed to be 0 for the first fetch after an agent switch, contrary to the code comment. A related effect-ordering issue in AgentLogViewer can also allow stale cached data to overwrite the cursor reset when revisiting a previously-viewed agent.

Comment thread frontend/src/hooks/useAgents.js Outdated
- dashboard.py: guard ws["github_repo"] for None before passing to
  get_pr_branch in the fix_review restart path; returns 409 with a
  descriptive error when the workspace has no github_repo configured
- agent_pool.py: decode exit_code via os.waitpid in _monitor_pid so the
  log line reflects the actual exit status; falls back to None for
  non-child (reattached) processes that raise ChildProcessError
- EnvEditor.jsx: add lastSavedRef to capture saved vars after a
  successful PUT; add Discard button that reverts to lastSavedRef
  content instead of relying on a re-fetch that may be suppressed by
  the dirty guard

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 2 | low: 1

All three previously reported bugs have been fixed. Three new bugs were found: a wrong-file content restoration on discard after switching files in EnvEditor, an unthrottled polling loop in usePlanning when the API returns null, and misleading agent IDs containing the literal string 'None' in the resume path of agent_pool.

Comment thread frontend/src/components/modals/EnvEditor.jsx
Comment thread frontend/src/hooks/usePlanning.js Outdated
Comment thread orchestrator/agent_pool.py Outdated
- EnvEditor: reset lastSavedRef.current to null on activeFile change so
  Discard after a file switch restores the correct file's server content
- usePlanning: apply exponential backoff for null API responses in poll()
  instead of recursing at a fixed 300 ms interval indefinitely
- agent_pool: use 'unknown' sentinel instead of literal "None" in agent ID
  when issue_number is absent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

medium: 2 | low: 2

All three previously-reported bugs are confirmed fixed in this PR. Four new bugs were identified: a path traversal guard edge case in the dashboard, a potentially blocking os.waitpid call in the agent monitor thread, orphan subprocesses from process-group-unaware SIGTERM, and a race window in the polling re-entry guard.

Comment thread orchestrator/dashboard.py
Comment thread orchestrator/agent_pool.py
Comment thread orchestrator/dashboard.py
Comment thread frontend/src/hooks/usePlanning.js
- dashboard.py: use Path.is_relative_to() for path traversal guard to
  correctly handle exact workspace root match (Thread 1)
- agent_pool.py: use os.WNOHANG in os.waitpid to avoid blocking the
  monitor daemon thread (Thread 2)
- dashboard.py: use os.killpg to send SIGTERM/SIGKILL to the entire
  process group, preventing orphaned child processes (Thread 3)
- usePlanning.js: set pollActiveRef.current=true synchronously before
  setTimeout to close the 300ms re-entry window (Thread 4)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

medium: 2

Two new medium-severity bugs introduced in the reattach/restart infrastructure: _monitor_pid uses os.kill instead of os.killpg when timing out a reattached agent (orphaning child processes), and _monitor_agent incorrectly fires the completion callback for externally-stopped agents. All four previously-reported bugs have been fixed in this PR.

Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
- Replace os.kill with os.killpg in _monitor_pid to signal entire process group,
  preventing child processes from being orphaned when reattached agents time out
- Remove spurious _on_agent_complete callback from externally-stopped branch in
  _monitor_agent to avoid triggering PR creation, issue updates, and worktree
  cleanup for agents deliberately replaced by restart_agent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

Both previously-reported bugs (orphaned child processes on timeout kill, and _on_agent_complete firing for externally-stopped agents) are fixed. One new medium-severity bug remains: the newly-added _monitor_pid method completes its DB writes for reattached agents without ever invoking _on_agent_complete, so callback-driven downstream automation (e.g. dispatching a fix_review agent after a reattached implement agent creates a PR) never fires for agents that survived an orchestrator restart.

Comment thread orchestrator/agent_pool.py
Add _on_reattach_complete callback to AgentPool so that _monitor_pid
fires downstream automation (e.g. PR review dispatch) for reattached
agents, matching the behaviour of _monitor_agent for fresh agents.

- Add _on_reattach_complete attribute with signature
  (agent_id, agent_type, pr_number, workspace)
- Add set_reattach_completion_callback() setter
- Track effective_pr_number through all completion paths in _monitor_pid
- Fire the callback at the end of _monitor_pid and before the early
  return in the unpushed-commits→auto-PR path (line ~1125), covering
  both the implement agent PR-found/auto-created paths and the
  fix_review completion path mentioned in the review

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

The previously reported _monitor_pid/_on_agent_complete bug is fixed via the new _on_reattach_complete callback mechanism. One new medium-severity bug was found: stopPolling() in usePlanning.js fails to reset pollActiveRef.current, which can permanently block poll() from rescheduling after a cancelGeneration() failure, leaving the UI frozen in a 'generating' state.

Comment thread frontend/src/hooks/usePlanning.js
Reset pollActiveRef.current in stopPolling() to prevent permanent stuck
'generating' state when cancelGeneration() is called within the 300ms
setTimeout window before the poll tick's finally block runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

medium: 2

The previously reported stopPolling/pollActiveRef bug is fixed. Two new medium-severity bugs exist in usePlanning.js: poll() silently exits without clearing generating state when its retry budget is exhausted (both in the null-data and catch paths), and resumeSession does not reset generating when session data fails to load, leaving the UI permanently stuck in a generating spinner in both scenarios.

Comment thread frontend/src/hooks/usePlanning.js
Comment thread frontend/src/hooks/usePlanning.js Outdated
- Call setGenerating(false) in poll()'s !data path when MAX_POLL_ERRORS is exceeded
- Call setGenerating(false) in poll()'s catch block when MAX_POLL_ERRORS is exceeded
- Call setGenerating(false) in resumeSession's early-return guard for null/error data

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 2 potential bugs in this PR.

low: 1 | medium: 1

Both previously reported generating-spinner bugs in usePlanning.js are fixed. Two new bugs were found: a race condition in IssueQueue where a single retryingIssue string value causes a second concurrent retry's onSettled to clear the first row's spinner prematurely, and a logic error in agent_pool.py where failed reattached fix_review agents set the issue status to in_progress instead of needs_human/pending, permanently stranding those issues.

Comment thread frontend/src/components/issues/IssueQueue.jsx
Comment thread orchestrator/agent_pool.py
- Use a Set for retryingIssues in IssueQueue.jsx so concurrent retries
  don't clear each other's loading indicators prematurely
- Set failed reattached fix_review agent issue status to 'needs_human'
  instead of 'in_progress' to prevent issues from being permanently stuck

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

low: 3

Both previously reported bugs are fixed: IssueQueue now uses a Set for retryingIssues (concurrent retry spinners no longer interfere), and the reattached fix_review agent failure path now correctly sets status to needs_human. Three new low-severity bugs were found: a race condition in usePlanning.js that can create a duplicate polling loop, an ID mismatch in EnvEditor that unmounts/remounts inputs after save, and a silent scroll listener registration failure in PlannerModal on first open.

Comment thread frontend/src/hooks/usePlanning.js
Comment thread frontend/src/components/modals/EnvEditor.jsx
Comment thread frontend/src/components/planner/PlannerModal.jsx
- usePlanning.js: Add pollGenRef generation counter so stopPolling() can
  bump the generation without risk of the in-flight tick's finally block
  clobbering the flag owned by a newer poll() invocation. Each tick
  captures its generation after stopPolling() and only resets
  pollActiveRef when pollGenRef still matches, eliminating the duplicate
  concurrent polling loop described in the review.

- EnvEditor.jsx: Sync row id with the key name as the user types so that
  after save + reload (where the server rebuilds rows as
  { id: keyName, key: keyName, value }) the React key prop is stable
  across reconciliation, preventing input unmount/remount and focus loss.

- PlannerModal.jsx: Replace useRef chatRef with a useCallback callback
  ref backed by useState(chatEl). Because chatEl is a state value, the
  scroll-listener useEffect (and the auto-scroll effect) include it in
  their dependency arrays and re-run once the portal commits the DOM
  node, ensuring the scroll listener is always attached on first open.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

medium: 2 | low: 1

All three previously reported bugs are fixed. Three new bugs were found: two in usePlanning.js where poll() cancels backoff timers on every recursive tick and sendMessage() lacks a stopPolling() guard, and one in ReviewThreads.jsx where custom sentinel tags can inadvertently match and alter user-authored comment content.

Comment thread frontend/src/hooks/usePlanning.js
Comment thread frontend/src/hooks/usePlanning.js
Comment thread frontend/src/components/prs/ReviewThreads.jsx
- Split poll() into _pollTick() (internal, no stopPolling on entry) and
  poll() (public API, always calls stopPolling first). Recursive calls
  in success and error-backoff paths now use _pollTickRef to avoid
  cancelling in-progress backoff timers and to correctly re-arm the
  poll loop after each tick.
- Add stopPolling() at the start of sendMessage() so any stale in-flight
  tick is always cancelled before a new poll loop begins.
- Replace HTML-like sentinel tags (<code-block>, <inline-code>, <strong>)
  in formatCommentBody with null-byte delimited sentinels that cannot
  appear in GitHub comment text, preventing collisions with user content.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

low: 4

All three previously reported bugs (poll() backoff cancellation, sendMessage() missing stopPolling(), and sentinel tag collisions in formatCommentBody) have been fixed. Four new low-severity issues were found: an over-broad severity-prefix strip in formatCommentBody affecting all comment types, a stale event flush window in createIssue's poll interval, a React concurrent-mode-unsafe ref mutation during render in useAgents, and an unbounded module-level set in planner.py.

Comment thread frontend/src/components/prs/ReviewThreads.jsx
Comment thread frontend/src/hooks/usePlanning.js
Comment thread frontend/src/hooks/useAgents.js
Comment thread orchestrator/planner.py
- ReviewThreads.jsx: restrict formatCommentBody severity-strip regex to known
  keywords (HIGH|CRITICAL|MEDIUM|LOW|INFO) so legitimate bold prefixes like
  **Note**: are not silently removed
- usePlanning.js: introduce `cancelled` flag in createIssue to prevent stale
  interval ticks from appending stream events after creation state is cleared
- useAgents.js: add explicit concurrent-mode safety note documenting the
  intentional render-body ref mutation and why it is safe
- planner.py: replace unbounded _issue_created_keys set with a bounded
  OrderedDict (cap 10 000 entries) with LRU eviction to prevent memory growth
  in long-running deployments; update cleanup_session_issue_keys accordingly

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 5 potential bugs in this PR.

medium: 2 | low: 3

The PR fixes all four previously reported bugs (formatCommentBody regex, createIssue cancelled-flag race, ref-mutation-in-render, and _issue_created_keys unbounded growth). Five new bugs were found: a file-unlink-while-open resource handling issue and a _stopped_agent_ids leak in error paths (both medium), plus three low-severity issues involving waitpid exit-code misreporting, log-tailer thread termination on transient DB errors, and a cursor-reset race in AgentLogViewer.

Comment thread orchestrator/agent_pool.py
Comment thread orchestrator/dashboard.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
Comment thread frontend/src/components/agents/AgentLogViewer.jsx Outdated
- Thread 1 (agent_pool.py:791): Close resume_stdout before os.unlink in
  the except clause so the file is not unlinked while still open. The
  finally guard is also updated to skip close() if already closed.
- Thread 2 (dashboard.py:382): Wrap db.finish_agent/db.update_issue in
  try/except that calls unmark_externally_stopped on failure, preventing
  a permanent leak of the agent_id in _stopped_agent_ids.
- Thread 3 (agent_pool.py:1166): Replace os.waitpid(pid, os.WNOHANG)
  with os.waitpid(pid, 0) so child processes are properly reaped and
  the exit code is accurately reported.
- Thread 4 (agent_pool.py:977): Wrap both db.update_agent(log_offset)
  calls in individual try/except blocks that log a warning and continue,
  preventing a transient DB error from killing the entire tailer thread.
- Thread 5 (AgentLogViewer.jsx:115): Remove cursorRef.current = 0 from
  the post-paint useEffect since useAgentLogs already resets it
  synchronously during render at the correct timing, avoiding a race
  that caused unnecessary re-fetching of already-seen events.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

low: 1

All five previously-reported bugs have been fixed in this PR. One new bug was introduced: the new _tail_log method (for freshly-spawned agents) lacks the try/except guard around db.update_agent that was correctly applied to the parallel _tail_log_file method — a transient DB error will kill the tailer thread.

Comment thread orchestrator/agent_pool.py
Wrap both db.update_agent calls in _tail_log with try/except to prevent
a transient DB error from propagating to the outer handler and killing
the tailer thread. Matches the existing pattern used in _tail_log_file.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

medium: 3 | low: 1

The PR fixes the previously-reported unprotected db.update_agent bug in _tail_log (both calls are now wrapped in try/except), but introduces analogous unprotected db.insert_event calls in the same methods (_tail_log and _tail_log_file) that can still kill the tailer thread; additionally, a misleading WNOHANG comment in _monitor_pid and a race condition leaving old agent records stuck in 'running' state in restart_agent were found.

Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/dashboard.py Outdated
- Wrap db.insert_event calls in _tail_log and _tail_log_file with
  try/except so a transient DB write error does not kill the tailer thread
- Fix os.waitpid(pid, 0) to use os.WNOHANG in _monitor_pid, matching
  the comment's stated intent and handling not-yet-reaped zombies
- In restart_agent, call db.finish_agent before dispatching the new agent
  so the old record is never permanently stuck in 'running' if a DB write
  fails after dispatch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

medium: 1 | low: 3

All four previously reported bugs are fixed in this revision. Four new issues were found: a missing unmark_externally_stopped call in restart_agent when the dispatch call throws (medium), an incorrect unmark call when db.update_issue fails after finish_agent already committed (low), a TOCTOU deduplication race in planner.py between two separate lock acquisitions (low), and accumulating phantom blank rows in the EnvEditor on repeated paste/upload (low).

Comment thread orchestrator/dashboard.py Outdated
Comment thread orchestrator/dashboard.py
Comment thread orchestrator/planner.py
Comment thread frontend/src/components/modals/EnvEditor.jsx Outdated
- dashboard.py: separate update_issue from finish_agent try/except so
  dispatch proceeds even if only the issue status update fails (Thread 2);
  wrap dispatch calls in try/except to always call unmark_externally_stopped
  on exception (Thread 1)
- planner.py: reserve (session_id, message_index) key inside the same lock
  acquisition as the membership check to close the TOCTOU race; clean up
  the reservation in the finally block if issue creation ultimately fails
  (Thread 3)
- EnvEditor.jsx: limit blank rows to at most one after paste/upload to
  prevent phantom row accumulation (Thread 4)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@rekpero rekpero left a comment

Choose a reason for hiding this comment

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

🟢 Claude BugBot Analysis

All four previously reported bugs are fixed in this PR. The restart_agent endpoint now correctly separates finish_agent and update_issue into independent try/except blocks (fixing PRRT_kwDORZvToM50Ytgv), wraps each dispatch_* call in try/except that calls unmark_externally_stopped on exception (fixing PRRT_kwDORZvToM50Ytgu), the planner dedup check and insertion are now in the same lock block (fixing PRRT_kwDORZvToM50Ytgw), and EnvEditor retains at most one blank row during paste/upload (fixing PRRT_kwDORZvToM50Ytgx). No new bugs were identified in the added code.

No bugs were detected in this PR.

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.

Migrate dashboard UI from vanilla HTML to React 18 + Vite SPA

1 participant