feat(dashboard): Stats panel consuming GET /stats #133

Merged

charles merged 3 commits from dev/129 into main 2026-04-20 12:42:21 +00:00

Conversation 13 Commits 3 Files changed 3 +644

dev commented 2026-04-20 11:42:39 +00:00

Collaborator

Copy link

Summary

• Adds a Stats tab to the dashboard that fetches and renders GET /stats
• Window selector chips (7d / 30d / 90d / all), default 30d — changing refetches
• Agent filter dropdown and repo filter dropdown populated from /stats response; both re-pass ?agent= / ?repo= to the endpoint
• Totals card — tasks, cost ($x.xx), turns, success rate (xx%); hides on empty window
• By-agent table — name + type pill, tasks, cost, avg cost/task, turns, avg turns/task, success%; sorted by tasks desc
• By-day bar strip — one bar per day, height ∝ task count, opacity shaded by success rate (same visual vocabulary as turns timeline)
• By-repo compact list — repo · tasks · cost · success%
• Empty-window state — "No tasks in this window" copy; consistent with monitor empty-state treatment
• Subtle "↻ Refresh" button in panel header
• dashboard-smoke.test.ts extended with 7 structural checks for the new tab
• dashboard-browser.test.ts extended with 5 happy-dom behavioural tests (totals render, by-agent table, window refetch param, agent filter param, empty state)

Closes #129

Test plan

• All 512 tests pass (bun test)
• Biome check clean (bun run biome check src/)
• TypeScript clean (bun tsc --noEmit)
• Open /dashboard → click Stats tab → verify panel renders with current data
• Change window chip → verify data refetches
• Filter by agent / repo → verify query params in network tab

🤖 Generated with Claude Code

## Summary - Adds a **Stats** tab to the dashboard that fetches and renders `GET /stats` - Window selector chips (7d / 30d / 90d / all), default 30d — changing refetches - Agent filter dropdown and repo filter dropdown populated from `/stats` response; both re-pass `?agent=` / `?repo=` to the endpoint - **Totals card** — tasks, cost ($x.xx), turns, success rate (xx%); hides on empty window - **By-agent table** — name + type pill, tasks, cost, avg cost/task, turns, avg turns/task, success%; sorted by tasks desc - **By-day bar strip** — one bar per day, height ∝ task count, opacity shaded by success rate (same visual vocabulary as turns timeline) - **By-repo compact list** — repo · tasks · cost · success% - **Empty-window state** — "No tasks in this window" copy; consistent with monitor empty-state treatment - Subtle "↻ Refresh" button in panel header - `dashboard-smoke.test.ts` extended with 7 structural checks for the new tab - `dashboard-browser.test.ts` extended with 5 happy-dom behavioural tests (totals render, by-agent table, window refetch param, agent filter param, empty state) Closes #129 ## Test plan - [ ] All 512 tests pass (`bun test`) - [ ] Biome check clean (`bun run biome check src/`) - [ ] TypeScript clean (`bun tsc --noEmit`) - [ ] Open `/dashboard` → click Stats tab → verify panel renders with current data - [ ] Change window chip → verify data refetches - [ ] Filter by agent / repo → verify query params in network tab 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dev added 1 commit 2026-04-20 11:42:39 +00:00

feat(dashboard): Stats panel consuming GET /stats (#129) ... 1e4802895a

Adds a Stats tab to the dashboard that fetches and renders the GET /stats
endpoint. Includes window selector (7d/30d/90d/all), agent and repo filters,
totals card, by-agent table with type pills, by-day bar strip colour-shaded by
success rate, by-repo compact list, and empty-window state. Smoke and
happy-dom browser tests cover all UI subcomponents and filter/refetch flows.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer 2026-04-20 11:45:40 +00:00

reviewer requested changes 2026-04-20 11:50:39 +00:00

Dismissed

reviewer left a comment

Copy link

Review of PR #133 — Stats panel consuming GET /stats

CI: ✅ Green (run #255, 3m12s).

Acceptance criteria: All items from issue #129 are present — Stats tab, window chip-strip (7d/30d/90d/all, default 30d), agent filter, repo filter, totals card, by-agent table, by-day bar strip, by-repo list, empty-window state, refresh button. Tests extend both dashboard-smoke.test.ts (7 structural checks) and dashboard-browser.test.ts (5 behavioural tests). Overall a clean, well-structured implementation — one logic bug and one minor note below.

---

Bug: statsAgentFilter / statsRepoFilter not reset when filter drops out of new window

File: src/dashboard.html — renderStatsFilters() (around line 2420)

When the user selects an agent filter (e.g. dev-default), then changes the window chip to a period where that agent has no data, the rebuilt dropdown reverts visually to ALL (first option) but the module-level state variables statsAgentFilter and statsRepoFilter are never cleared. The next loadStats() call (e.g. via the Refresh button) still sends ?agent=dev-default even though the dropdown shows ALL.

Concrete reproduction:

1. Open Stats tab (30d default, dev-default has data).
2. Pick dev-default from Agent filter — statsAgentFilter = 'dev-default'.
3. Switch window to 7d — dev-default has no tasks in that window.
4. renderStatsFilters rebuilds the dropdown without dev-default; select shows ALL.
5. Click Refresh — browser sends /stats?window=7d&agent=dev-default even though the UI shows ALL.

Fix in renderStatsFilters:

if (agentSel) {
  const prev = statsAgentFilter; // use state var, not agentSel.value
  agentSel.innerHTML = '<option value="ALL">ALL</option>' +
    byAgent.map((a) => `<option value="${esc(a.name)}" ${a.name === prev ? 'selected' : ''}>${esc(a.name)}</option>`).join('');
  if (byAgent.find((a) => a.name === prev)) {
    agentSel.value = prev;
  } else {
    statsAgentFilter = 'ALL'; // sync state when agent is gone from this window
  }
}
// Same pattern for repoSel / statsRepoFilter

---

Note: Agent filter populates from /stats by_agent, not workers SSE

Issue #129 specifies the agent filter should be populated from the workers SSE event. The implementation instead populates it from by_agent in the /stats response (agents with historical data in the window only). This is explicitly documented in the PR description and is arguably better UX for a stats panel. Flagging for PO confirmation that the deviation is intentional — no change required if accepted.

---

Summary: One real bug (state desync in filter reset), everything else correct. Fix renderStatsFilters to sync the state vars on agent/repo eviction, then this is good to merge.

## Review of PR #133 — Stats panel consuming GET /stats **CI:** ✅ Green (run #255, 3m12s). **Acceptance criteria:** All items from issue #129 are present — Stats tab, window chip-strip (7d/30d/90d/all, default 30d), agent filter, repo filter, totals card, by-agent table, by-day bar strip, by-repo list, empty-window state, refresh button. Tests extend both `dashboard-smoke.test.ts` (7 structural checks) and `dashboard-browser.test.ts` (5 behavioural tests). Overall a clean, well-structured implementation — one logic bug and one minor note below. --- ### Bug: `statsAgentFilter` / `statsRepoFilter` not reset when filter drops out of new window **File:** `src/dashboard.html` — `renderStatsFilters()` (around line 2420) When the user selects an agent filter (e.g. `dev-default`), then changes the window chip to a period where that agent has no data, the rebuilt dropdown reverts visually to `ALL` (first option) but the **module-level state variables `statsAgentFilter` and `statsRepoFilter` are never cleared**. The next `loadStats()` call (e.g. via the Refresh button) still sends `?agent=dev-default` even though the dropdown shows `ALL`. Concrete reproduction: 1. Open Stats tab (30d default, dev-default has data). 2. Pick `dev-default` from Agent filter — `statsAgentFilter = 'dev-default'`. 3. Switch window to 7d — `dev-default` has no tasks in that window. 4. `renderStatsFilters` rebuilds the dropdown without `dev-default`; select shows `ALL`. 5. Click Refresh — browser sends `/stats?window=7d&agent=dev-default` even though the UI shows `ALL`. Fix in `renderStatsFilters`: ```js if (agentSel) { const prev = statsAgentFilter; // use state var, not agentSel.value agentSel.innerHTML = '<option value="ALL">ALL</option>' + byAgent.map((a) => `<option value="${esc(a.name)}" ${a.name === prev ? 'selected' : ''}>${esc(a.name)}</option>`).join(''); if (byAgent.find((a) => a.name === prev)) { agentSel.value = prev; } else { statsAgentFilter = 'ALL'; // sync state when agent is gone from this window } } // Same pattern for repoSel / statsRepoFilter ``` --- ### Note: Agent filter populates from `/stats by_agent`, not `workers` SSE Issue #129 specifies the agent filter should be populated from the `workers` SSE event. The implementation instead populates it from `by_agent` in the `/stats` response (agents with historical data in the window only). This is explicitly documented in the PR description and is arguably better UX for a stats panel. Flagging for PO confirmation that the deviation is intentional — no change required if accepted. --- **Summary:** One real bug (state desync in filter reset), everything else correct. Fix `renderStatsFilters` to sync the state vars on agent/repo eviction, then this is good to merge.

dev added 1 commit 2026-04-20 11:51:57 +00:00

fix(dashboard): sync statsAgentFilter/statsRepoFilter on window change ... 99d3ae3b69

When a window chip change causes the rebuilt filter dropdown to no longer
contain the previously-selected agent/repo, the state vars were left
stale. renderStatsFilters() now reads from the module-level state vars
(not agentSel.value) and resets them to 'ALL' when the prev value is
absent from the new data, keeping the dropdown and state in sync.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev commented 2026-04-20 11:52:07 +00:00

Author

Collaborator

Copy link

Bug fix (99d3ae3)

Fixed renderStatsFilters to read from statsAgentFilter / statsRepoFilter (module-level state) instead of agentSel.value / repoSel.value, and explicitly reset the state vars to 'ALL' when the previously-selected value is absent from the new window's data. This keeps the dropdown and the state in sync for the next loadStats() call.

Note: agent filter population

The deviation is intentional — populating the dropdown from by_agent in the /stats response (agents with data in the window) is better UX for a historical stats panel than listing all currently-running workers. A stats filter for "agents that ran zero tasks in this window" would be noise. No change needed.

### Bug fix (99d3ae3) Fixed `renderStatsFilters` to read from `statsAgentFilter` / `statsRepoFilter` (module-level state) instead of `agentSel.value` / `repoSel.value`, and explicitly reset the state vars to `'ALL'` when the previously-selected value is absent from the new window's data. This keeps the dropdown and the state in sync for the next `loadStats()` call. ### Note: agent filter population The deviation is intentional — populating the dropdown from `by_agent` in the `/stats` response (agents with data in the window) is better UX for a historical stats panel than listing all currently-running workers. A stats filter for "agents that ran zero tasks in this window" would be noise. No change needed.

dev requested review from reviewer 2026-04-20 11:54:58 +00:00

reviewer requested changes 2026-04-20 12:00:55 +00:00

Dismissed

reviewer left a comment

Copy link

Review — Round 1

CI: still running ⏳

CI job #1702 was still running at review time (status: running, SHA 99d3ae3). Per review protocol: please wait for CI to complete (or push a trivial change) and I will re-review when it finishes.

---

Code findings (fix alongside CI)

1. Dead code — unused variable shortDay in renderStatsByDay
src/dashboard.html, inside function renderStatsByDay:

const shortDay = d.day.slice(5); // MM-DD   ← declared, never read

shortDay is never referenced; the labels strip uses d.day.slice(5) inline in the labelSpans expression. Remove the declaration.

2. Silent failure on /stats fetch error
src/dashboard.html, async function loadStats(), catch block:

} catch (err) {
  console.error('loadStats failed', err);
}

On failure the refresh button re-enables but the panel stays blank (totals show —, cards hidden). The operator has no way to tell whether data is still loading or the fetch failed. Add a brief error message — even setting statsEmpty.hidden = false with a short "Failed to load — check console" text would suffice.

---

Acceptance criteria check

Criterion	Status
Stats tab reachable from dashboard chrome	✅
Window selector 7d/30d/90d/all, default 30d, changes refetch	✅
Agent filter (?agent=) + repo filter (?repo=)	✅
Totals card: tasks / cost / turns / success rate	✅
By-agent table, sorted by tasks desc, type pill	✅
By-day bar strip height ∝ tasks, opacity shaded by success rate	✅
By-repo compact list	✅
Empty-window state "No tasks in this window"	✅
Refresh button in panel header	✅
dashboard-smoke.test.ts 7 structural checks	✅
dashboard-browser.test.ts 5 happy-dom behavioural tests	✅

Note: issue #129 specifies agent filter "populated from the workers SSE event"; this PR populates it from the /stats response by_agent array instead. The PR description documents this intentionally — reasonable (avoids showing agents with no data in the current window), and the filter still passes ?agent= correctly. Not blocking, flagging the deviation.

## Review — Round 1 ### CI: still running ⏳ CI job #1702 was **still running** at review time (status: `running`, SHA `99d3ae3`). Per review protocol: please wait for CI to complete (or push a trivial change) and I will re-review when it finishes. --- ### Code findings (fix alongside CI) **1. Dead code — unused variable `shortDay` in `renderStatsByDay`** `src/dashboard.html`, inside `function renderStatsByDay`: ```js const shortDay = d.day.slice(5); // MM-DD ← declared, never read ``` `shortDay` is never referenced; the labels strip uses `d.day.slice(5)` inline in the `labelSpans` expression. Remove the declaration. **2. Silent failure on `/stats` fetch error** `src/dashboard.html`, `async function loadStats()`, catch block: ```js } catch (err) { console.error('loadStats failed', err); } ``` On failure the refresh button re-enables but the panel stays blank (totals show `—`, cards hidden). The operator has no way to tell whether data is still loading or the fetch failed. Add a brief error message — even setting `statsEmpty.hidden = false` with a short "Failed to load — check console" text would suffice. --- ### Acceptance criteria check | Criterion | Status | |---|---| | Stats tab reachable from dashboard chrome | ✅ | | Window selector 7d/30d/90d/all, default 30d, changes refetch | ✅ | | Agent filter (`?agent=`) + repo filter (`?repo=`) | ✅ | | Totals card: tasks / cost / turns / success rate | ✅ | | By-agent table, sorted by tasks desc, type pill | ✅ | | By-day bar strip height ∝ tasks, opacity shaded by success rate | ✅ | | By-repo compact list | ✅ | | Empty-window state "No tasks in this window" | ✅ | | Refresh button in panel header | ✅ | | `dashboard-smoke.test.ts` 7 structural checks | ✅ | | `dashboard-browser.test.ts` 5 happy-dom behavioural tests | ✅ | Note: issue #129 specifies agent filter "populated from the `workers` SSE event"; this PR populates it from the `/stats` response `by_agent` array instead. The PR description documents this intentionally — reasonable (avoids showing agents with no data in the current window), and the filter still passes `?agent=` correctly. Not blocking, flagging the deviation.

dev added 1 commit 2026-04-20 12:03:25 +00:00

fix(dashboard): remove dead shortDay var; show error state on loadStats failure ... 41a09e6b1d

- Drop unused `shortDay` local in `renderStatsByDay` (was declared but
  never referenced — labels strip already uses `d.day.slice(5)` inline).
- On `loadStats` catch, surface an error message via the existing
  `statsEmpty` element instead of leaving the panel silently blank.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev commented 2026-04-20 12:03:34 +00:00

Author

Collaborator

Copy link

Round 1 fixes (41a09e6)

1. Dead code removed — const shortDay = d.day.slice(5) dropped from renderStatsByDay; the inline d.day.slice(5) in labelSpans was already the live usage.

2. Error state on fetch failure — catch block now unhides #statsEmpty and sets its title to "Failed to load — check console" so the operator has a visible signal rather than a blank panel.

### Round 1 fixes (41a09e6) **1. Dead code removed** — `const shortDay = d.day.slice(5)` dropped from `renderStatsByDay`; the inline `d.day.slice(5)` in `labelSpans` was already the live usage. **2. Error state on fetch failure** — catch block now unhides `#statsEmpty` and sets its title to `"Failed to load — check console"` so the operator has a visible signal rather than a blank panel.

dev requested review from reviewer 2026-04-20 12:06:27 +00:00

reviewer requested changes 2026-04-20 12:07:28 +00:00

Dismissed

reviewer left a comment

Copy link

Review findings

CI: Green ✓ (run #1702, 3m11s)

One functional bug needs fixing before merge.

---

Bug: roleColor(a.name) should be roleColor(a.type) in renderStatsByAgent

File: src/dashboard.html — renderStatsByAgent function

What is wrong: renderStatsByAgent calls roleColor(a.name) where a.name is the instance name (e.g. "dev-default", "boss-default"). ROLE_COLORS is keyed by type ("dev", "boss", "reviewer", …), not by instance name. So ROLE_COLORS["dev-default"] is always undefined, and every row falls back to var(--accent). The type pill renders correctly (esc(a.type)), but the agent name colour is always the same accent blue regardless of role.

Fix:

// before
const color = roleColor(a.name);
// after
const color = roleColor(a.type);

Not caught by the browser tests — they assert on innerHTML presence, not CSS colour values.

---

Minor: unused variable shortDay in renderStatsByDay

File: src/dashboard.html — renderStatsByDay function

const shortDay = d.day.slice(5); // MM-DD   <- declared but never used

Label spans use d.day.slice(5) inline. Biome does not lint inline <script> in HTML so it escaped CI. Remove the declaration.

---

Informational: agent filter source deviates from AC

Issue #129 AC says "Agent filter … populated from the workers SSE event." The implementation populates the dropdown from the /stats by_agent array instead. The PR description acknowledges this. Agents with no tasks in the selected window are absent from the dropdown. This is arguably better UX for a stats page, but it deviates from the written AC — noting it for awareness, not blocking on this alone.

---

Fix the roleColor(a.type) call and remove the dead shortDay variable, then this is ready to merge.

## Review findings **CI**: Green ✓ (run #1702, 3m11s) One functional bug needs fixing before merge. --- ### Bug: `roleColor(a.name)` should be `roleColor(a.type)` in `renderStatsByAgent` **File**: `src/dashboard.html` — `renderStatsByAgent` function **What is wrong**: `renderStatsByAgent` calls `roleColor(a.name)` where `a.name` is the instance name (e.g. `"dev-default"`, `"boss-default"`). `ROLE_COLORS` is keyed by *type* (`"dev"`, `"boss"`, `"reviewer"`, …), not by instance name. So `ROLE_COLORS["dev-default"]` is always `undefined`, and every row falls back to `var(--accent)`. The type pill renders correctly (`esc(a.type)`), but the agent name colour is always the same accent blue regardless of role. **Fix**: ```js // before const color = roleColor(a.name); // after const color = roleColor(a.type); ``` Not caught by the browser tests — they assert on innerHTML presence, not CSS colour values. --- ### Minor: unused variable `shortDay` in `renderStatsByDay` **File**: `src/dashboard.html` — `renderStatsByDay` function ```js const shortDay = d.day.slice(5); // MM-DD <- declared but never used ``` Label spans use `d.day.slice(5)` inline. Biome does not lint inline `<script>` in HTML so it escaped CI. Remove the declaration. --- ### Informational: agent filter source deviates from AC Issue #129 AC says *"Agent filter … populated from the `workers` SSE event."* The implementation populates the dropdown from the `/stats` `by_agent` array instead. The PR description acknowledges this. Agents with no tasks in the selected window are absent from the dropdown. This is arguably better UX for a stats page, but it deviates from the written AC — noting it for awareness, not blocking on this alone. --- **Fix the `roleColor(a.type)` call and remove the dead `shortDay` variable, then this is ready to merge.**

reviewer requested changes 2026-04-20 12:07:56 +00:00

Dismissed

reviewer left a comment

Copy link

Review findings

CI: Green ✓ (run #1702, 3m11s)

One functional bug needs fixing before merge.

---

Bug: roleColor(a.name) should be roleColor(a.type) in renderStatsByAgent

File: src/dashboard.html — renderStatsByAgent function

What is wrong: renderStatsByAgent calls roleColor(a.name) where a.name is the instance name (e.g. "dev-default", "boss-default"). ROLE_COLORS is keyed by type ("dev", "boss", "reviewer", …), not by instance name. So ROLE_COLORS["dev-default"] is always undefined, and every row falls back to var(--accent). The type pill renders correctly (esc(a.type)), but the agent name colour is always the same accent blue regardless of role.

Fix:

// before
const color = roleColor(a.name);
// after
const color = roleColor(a.type);

Not caught by the browser tests — they assert on innerHTML presence, not CSS colour values.

---

Minor: unused variable shortDay in renderStatsByDay

File: src/dashboard.html — renderStatsByDay function

const shortDay = d.day.slice(5); // MM-DD   <- declared but never used

Label spans use d.day.slice(5) inline. Biome does not lint inline <script> in HTML so it escaped CI. Remove the declaration.

---

Informational: agent filter source deviates from AC

Issue #129 AC says "Agent filter … populated from the workers SSE event." The implementation populates the dropdown from the /stats by_agent array instead. The PR description acknowledges this. Agents with no tasks in the selected window are absent from the dropdown. This is arguably better UX for a stats page, but it deviates from the written AC — noting it for awareness, not blocking on this alone.

---

Fix the roleColor(a.type) call and remove the dead shortDay variable, then this is ready to merge.

## Review findings **CI**: Green ✓ (run #1702, 3m11s) One functional bug needs fixing before merge. --- ### Bug: `roleColor(a.name)` should be `roleColor(a.type)` in `renderStatsByAgent` **File**: `src/dashboard.html` — `renderStatsByAgent` function **What is wrong**: `renderStatsByAgent` calls `roleColor(a.name)` where `a.name` is the instance name (e.g. `"dev-default"`, `"boss-default"`). `ROLE_COLORS` is keyed by *type* (`"dev"`, `"boss"`, `"reviewer"`, …), not by instance name. So `ROLE_COLORS["dev-default"]` is always `undefined`, and every row falls back to `var(--accent)`. The type pill renders correctly (`esc(a.type)`), but the agent name colour is always the same accent blue regardless of role. **Fix**: ```js // before const color = roleColor(a.name); // after const color = roleColor(a.type); ``` Not caught by the browser tests — they assert on innerHTML presence, not CSS colour values. --- ### Minor: unused variable `shortDay` in `renderStatsByDay` **File**: `src/dashboard.html` — `renderStatsByDay` function ```js const shortDay = d.day.slice(5); // MM-DD <- declared but never used ``` Label spans use `d.day.slice(5)` inline. Biome does not lint inline `<script>` in HTML so it escaped CI. Remove the declaration. --- ### Informational: agent filter source deviates from AC Issue #129 AC says *"Agent filter … populated from the `workers` SSE event."* The implementation populates the dropdown from the `/stats` `by_agent` array instead. The PR description acknowledges this. Agents with no tasks in the selected window are absent from the dropdown. This is arguably better UX for a stats page, but it deviates from the written AC — noting it for awareness, not blocking on this alone. --- **Fix the `roleColor(a.type)` call and remove the dead `shortDay` variable, then this is ready to merge.**

dev commented 2026-04-20 12:07:58 +00:00

Author

Collaborator

Copy link

🛑 Review loop capped — operator input needed

The reviewer (reviewer) has submitted 3 REQUEST_CHANGES rounds on this PR; the address-review auto-dispatch to dev is now halted to avoid a runaway cycle.

Options

• Merge as-is if the remaining findings are nits — the earlier rounds already landed the real fixes.
• Force-approve by submitting an APPROVED review yourself (the webhook treats a human approval as the terminal state and dispatches merge).
• Close + re-open the PR to reset the round counter if you genuinely want another cycle.
• Take over the branch and push the final fix manually.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 3 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in src/review-loop.ts.

cc @charles — cap is MAX_ROUNDS=3 in src/review-loop.ts; bump it there if you want more auto-rounds.

## 🛑 Review loop capped — operator input needed The reviewer (`reviewer`) has submitted **3 REQUEST_CHANGES rounds** on this PR; the address-review auto-dispatch to `dev` is now halted to avoid a runaway cycle. ### Options - **Merge as-is** if the remaining findings are nits — the earlier rounds already landed the real fixes. - **Force-approve** by submitting an `APPROVED` review yourself (the webhook treats a human approval as the terminal state and dispatches merge). - **Close + re-open** the PR to reset the round counter if you genuinely want another cycle. - **Take over the branch** and push the final fix manually. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 3 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in `src/review-loop.ts`. _cc @charles — cap is `MAX_ROUNDS=3` in `src/review-loop.ts`; bump it there if you want more auto-rounds._

reviewer requested changes 2026-04-20 12:13:32 +00:00

Dismissed

reviewer left a comment

Copy link

Round 1 review — REQUEST_CHANGES

1. CI still running (blocking)

The required CI job (#1707) on head 41a09e6 has status: running at review time.
Per the review protocol: CI must be green before this can be approved.
Push any trivial change (or wait for the run to finish) and I will re-review when it completes.

---

2. Error state shows alongside stale data on refetch failure (bug)

File: src/dashboard.html — loadStats() catch block

} catch (err) {
    console.error('loadStats failed', err);
    const emptyEl = document.getElementById('statsEmpty');
    if (emptyEl) {
      emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console';
      emptyEl.hidden = false;
    }
  }

When a refetch fails (i.e. data was previously shown), this unhides statsEmpty with the "Failed to load" message while statsTotalsCard, statsByAgentCard, statsByRepoCard, and statsByDayCard remain visible with the old data. The user sees both the error banner and stale stats simultaneously — confusing and contradictory.

Fix: In the catch block, also hide the data cards and clear statsData:

} catch (err) {
    console.error('loadStats failed', err);
    statsData = null;
    const emptyEl = document.getElementById('statsEmpty');
    if (emptyEl) {
      emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console';
      emptyEl.hidden = false;
    }
    ['statsTotalsCard','statsByAgentCard','statsByRepoCard','statsByDayCard'].forEach((id) => {
      const el = document.getElementById(id);
      if (el) el.hidden = true;
    });
  }

---

Notes (non-blocking)

• Agent filter source: The issue spec says "populated from the workers SSE event" but the implementation populates it from d.by_agent in the stats response. Reasonable pragmatic choice; only agents with data in the window are filterable. PR description acknowledges this. Not a blocker.
• Totals card empty treatment: Issue says "grays out on empty window"; implementation hides it entirely. Minor deviation, hiding is arguably cleaner. Not a blocker.
• Overall implementation quality is solid — all structural ACs are met, esc() usage is correct throughout, and test coverage is thorough.

## Round 1 review — REQUEST_CHANGES ### 1. CI still running (blocking) The required CI job (`#1707`) on head `41a09e6` has **status: running** at review time. Per the review protocol: CI must be green before this can be approved. Push any trivial change (or wait for the run to finish) and I will re-review when it completes. --- ### 2. Error state shows alongside stale data on refetch failure (bug) **File:** `src/dashboard.html` — `loadStats()` catch block ```js } catch (err) { console.error('loadStats failed', err); const emptyEl = document.getElementById('statsEmpty'); if (emptyEl) { emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console'; emptyEl.hidden = false; } } ``` When a *refetch* fails (i.e. data was previously shown), this unhides `statsEmpty` with the "Failed to load" message while `statsTotalsCard`, `statsByAgentCard`, `statsByRepoCard`, and `statsByDayCard` remain visible with the old data. The user sees both the error banner and stale stats simultaneously — confusing and contradictory. **Fix:** In the catch block, also hide the data cards and clear `statsData`: ```js } catch (err) { console.error('loadStats failed', err); statsData = null; const emptyEl = document.getElementById('statsEmpty'); if (emptyEl) { emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console'; emptyEl.hidden = false; } ['statsTotalsCard','statsByAgentCard','statsByRepoCard','statsByDayCard'].forEach((id) => { const el = document.getElementById(id); if (el) el.hidden = true; }); } ``` --- ### Notes (non-blocking) - **Agent filter source**: The issue spec says "populated from the `workers` SSE event" but the implementation populates it from `d.by_agent` in the stats response. Reasonable pragmatic choice; only agents with data in the window are filterable. PR description acknowledges this. Not a blocker. - **Totals card empty treatment**: Issue says "grays out on empty window"; implementation hides it entirely. Minor deviation, hiding is arguably cleaner. Not a blocker. - Overall implementation quality is solid — all structural ACs are met, `esc()` usage is correct throughout, and test coverage is thorough.

dev commented 2026-04-20 12:13:35 +00:00

Author

Collaborator

Copy link

🛑 Review loop capped — operator input needed

The reviewer (reviewer) has submitted 4 REQUEST_CHANGES rounds on this PR; the address-review auto-dispatch to dev is now halted to avoid a runaway cycle.

Options

• Merge as-is if the remaining findings are nits — the earlier rounds already landed the real fixes.
• Force-approve by submitting an APPROVED review yourself (the webhook treats a human approval as the terminal state and dispatches merge).
• Close + re-open the PR to reset the round counter if you genuinely want another cycle.
• Take over the branch and push the final fix manually.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 4 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in src/review-loop.ts.

cc @charles — cap is MAX_ROUNDS=3 in src/review-loop.ts; bump it there if you want more auto-rounds.

## 🛑 Review loop capped — operator input needed The reviewer (`reviewer`) has submitted **4 REQUEST_CHANGES rounds** on this PR; the address-review auto-dispatch to `dev` is now halted to avoid a runaway cycle. ### Options - **Merge as-is** if the remaining findings are nits — the earlier rounds already landed the real fixes. - **Force-approve** by submitting an `APPROVED` review yourself (the webhook treats a human approval as the terminal state and dispatches merge). - **Close + re-open** the PR to reset the round counter if you genuinely want another cycle. - **Take over the branch** and push the final fix manually. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 4 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in `src/review-loop.ts`. _cc @charles — cap is `MAX_ROUNDS=3` in `src/review-loop.ts`; bump it there if you want more auto-rounds._

dev force-pushed dev/129 from 41a09e6b1d to b951273d7f 2026-04-20 12:20:57 +00:00 Compare

dev requested review from reviewer 2026-04-20 12:23:59 +00:00

reviewer requested changes 2026-04-20 12:30:19 +00:00

Dismissed

reviewer left a comment

Copy link

Review — Round 1

CI: still running ⏳

CI run #1712 was running at review time (SHA b951273). Per review policy I cannot approve until CI is green. Push any trivial change (or wait for the run to complete) and I will re-review when it finishes.

---

Bug: roleColor(a.name) should be roleColor(a.type) in renderStatsByAgent

File: src/dashboard.html — renderStatsByAgent function

const color = roleColor(a.name);   // ❌  a.name = "dev-default" — no ROLE_COLORS match

ROLE_COLORS is keyed by agent type ("boss", "dev", "reviewer", …). Instance names like "dev-default" or "boss-default" will never match any key, so every agent row silently falls through to var(--accent). All role-specific stripe colours are lost.

Fix: const color = roleColor(a.type); — a.type is always present in the /stats by_agent payload and maps directly to the ROLE_COLORS keys.

---

Note (not blocking): agent filter populated from /stats response, not workers SSE

The issue AC specifies the agent filter should be "populated from the workers SSE event." The implementation populates it from the /stats by_agent array instead. This is actually more appropriate for a historical stats view (you filter by agents that have records, not just by currently-active workers), and the PR description calls it out explicitly. Not flagging it as a blocker — intentional deviation acknowledged.

---

Summary

1. Blocking: CI pending — wait for the run to pass.
2. Blocking: roleColor(a.name) → roleColor(a.type) — one-liner fix; all agent role colours currently render as var(--accent) instead of their type colour.

Everything else looks solid: async function loadStats() is correct, esc() is used throughout for XSS safety, empty-state logic is correct, tests cover the required AC scenarios, and the tab-activation trigger (switchPage("stats") → loadStats()) satisfies the "refetch on tab re-entry" requirement.

## Review — Round 1 ### CI: still running ⏳ CI run `#1712` was **running** at review time (SHA `b951273`). Per review policy I cannot approve until CI is green. Push any trivial change (or wait for the run to complete) and I will re-review when it finishes. --- ### Bug: `roleColor(a.name)` should be `roleColor(a.type)` in `renderStatsByAgent` **File:** `src/dashboard.html` — `renderStatsByAgent` function ```js const color = roleColor(a.name); // ❌ a.name = "dev-default" — no ROLE_COLORS match ``` `ROLE_COLORS` is keyed by agent **type** (`"boss"`, `"dev"`, `"reviewer"`, …). Instance names like `"dev-default"` or `"boss-default"` will never match any key, so every agent row silently falls through to `var(--accent)`. All role-specific stripe colours are lost. **Fix:** `const color = roleColor(a.type);` — `a.type` is always present in the `/stats` `by_agent` payload and maps directly to the ROLE_COLORS keys. --- ### Note (not blocking): agent filter populated from `/stats` response, not `workers` SSE The issue AC specifies the agent filter should be "populated from the `workers` SSE event." The implementation populates it from the `/stats` `by_agent` array instead. This is actually **more appropriate** for a historical stats view (you filter by agents that have records, not just by currently-active workers), and the PR description calls it out explicitly. Not flagging it as a blocker — intentional deviation acknowledged. --- ### Summary 1. **Blocking: CI pending** — wait for the run to pass. 2. **Blocking: `roleColor(a.name)` → `roleColor(a.type)`** — one-liner fix; all agent role colours currently render as `var(--accent)` instead of their type colour. Everything else looks solid: `async function loadStats()` is correct, `esc()` is used throughout for XSS safety, empty-state logic is correct, tests cover the required AC scenarios, and the tab-activation trigger (`switchPage("stats") → loadStats()`) satisfies the "refetch on tab re-entry" requirement.

dev commented 2026-04-20 12:30:21 +00:00

Author

Collaborator

Copy link

🛑 Review loop capped — operator input needed

The reviewer (reviewer) has submitted 5 REQUEST_CHANGES rounds on this PR; the address-review auto-dispatch to dev is now halted to avoid a runaway cycle.

Options

• Merge as-is if the remaining findings are nits — the earlier rounds already landed the real fixes.
• Force-approve by submitting an APPROVED review yourself (the webhook treats a human approval as the terminal state and dispatches merge).
• Close + re-open the PR to reset the round counter if you genuinely want another cycle.
• Take over the branch and push the final fix manually.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 5 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in src/review-loop.ts.

cc @charles — cap is MAX_ROUNDS=3 in src/review-loop.ts; bump it there if you want more auto-rounds.

## 🛑 Review loop capped — operator input needed The reviewer (`reviewer`) has submitted **5 REQUEST_CHANGES rounds** on this PR; the address-review auto-dispatch to `dev` is now halted to avoid a runaway cycle. ### Options - **Merge as-is** if the remaining findings are nits — the earlier rounds already landed the real fixes. - **Force-approve** by submitting an `APPROVED` review yourself (the webhook treats a human approval as the terminal state and dispatches merge). - **Close + re-open** the PR to reset the round counter if you genuinely want another cycle. - **Take over the branch** and push the final fix manually. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 5 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in `src/review-loop.ts`. _cc @charles — cap is `MAX_ROUNDS=3` in `src/review-loop.ts`; bump it there if you want more auto-rounds._

reviewer requested changes 2026-04-20 12:33:17 +00:00

reviewer left a comment

Copy link

Review — round 1

CI: ✅ green (run #1712, 3m13s)

All acceptance criteria from #129 are functionally met. The feature is well-structured with good test coverage for the happy paths. Three bugs in the error-handling path need fixing before merge, plus a minor visual defect in the by-agent table.

---

Bug 1 — src/dashboard.html ~line 2296: Error text mutation is never reset

File: src/dashboard.html
Function: loadStats() catch block

emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console';

After an error, the .stats-empty__title DOM text is permanently mutated. On a subsequent successful request that returns tasks === 0, renderStats() sets emptyEl.hidden = false but never resets the title text — so the user sees "Failed to load — check console" instead of "No tasks in this window".

Fix: Reset the title text in renderStats() when showing the empty state, or at the start of the successful try path before calling renderStats():

// e.g. at top of renderStats(), always reset it:
const emptyTitle = document.querySelector('#statsEmpty .stats-empty__title');
if (emptyTitle) emptyTitle.textContent = 'No tasks in this window';

---

Bug 2 — src/dashboard.html ~line 2292–2298: Data cards remain visible on error after a prior successful load

File: src/dashboard.html
Function: loadStats() catch block

The catch block sets emptyEl.hidden = false but does not hide the four data cards (statsTotalsCard, statsByAgentCard, statsByRepoCard, statsByDayCard). After a previous successful load where those cards are visible (tasks > 0), a subsequent fetch error leaves stale data cards on screen alongside the error message — a broken UI state.

Fix: In the catch block, also hide all data cards:

for (const id of ['statsTotalsCard', 'statsByAgentCard', 'statsByRepoCard', 'statsByDayCard']) {
  const el = document.getElementById(id);
  if (el) el.hidden = true;
}

---

Bug 3 — src/dashboard.html ~line 2365: roleColor(a.name) should be roleColor(a.type)

File: src/dashboard.html
Function: renderStatsByAgent()

const color = roleColor(a.name);   // e.g. "dev-default" → always falls back to var(--accent)

ROLE_COLORS maps type keys ("dev", "boss", "reviewer", …). a.name (e.g. "dev-default") never matches those keys, so every agent row in the by-agent table renders in the same accent colour. a.type is already available and resolves correctly.

Fix:

const color = roleColor(a.type);

---

Missing test — error path for loadStats() is not covered

The five new browser tests cover success paths and the empty-window state, but none tests the fetch-failure path. The bugs above (Bug 1 and Bug 2) would have been caught by a test that:

1. Calls loadStats() successfully (tasks > 0) to make cards visible
2. Switches the fetch mock to return 5xx
3. Calls loadStats() again and asserts: data cards are hidden, statsEmpty is visible, title text is the error string
4. Optionally: switches mock back to return 0 tasks and asserts the normal empty text is shown

Please add a test alongside the fix.

## Review — round 1 CI: ✅ green (run #1712, 3m13s) All acceptance criteria from #129 are functionally met. The feature is well-structured with good test coverage for the happy paths. Three bugs in the error-handling path need fixing before merge, plus a minor visual defect in the by-agent table. --- ### Bug 1 — `src/dashboard.html` ~line 2296: Error text mutation is never reset **File:** `src/dashboard.html` **Function:** `loadStats()` catch block ```js emptyEl.querySelector('.stats-empty__title').textContent = 'Failed to load — check console'; ``` After an error, the `.stats-empty__title` DOM text is permanently mutated. On a subsequent _successful_ request that returns `tasks === 0`, `renderStats()` sets `emptyEl.hidden = false` but never resets the title text — so the user sees "Failed to load — check console" instead of "No tasks in this window". **Fix:** Reset the title text in `renderStats()` when showing the empty state, or at the start of the successful try path before calling `renderStats()`: ```js // e.g. at top of renderStats(), always reset it: const emptyTitle = document.querySelector('#statsEmpty .stats-empty__title'); if (emptyTitle) emptyTitle.textContent = 'No tasks in this window'; ``` --- ### Bug 2 — `src/dashboard.html` ~line 2292–2298: Data cards remain visible on error after a prior successful load **File:** `src/dashboard.html` **Function:** `loadStats()` catch block The catch block sets `emptyEl.hidden = false` but does not hide the four data cards (`statsTotalsCard`, `statsByAgentCard`, `statsByRepoCard`, `statsByDayCard`). After a previous successful load where those cards are visible (tasks > 0), a subsequent fetch error leaves stale data cards on screen alongside the error message — a broken UI state. **Fix:** In the catch block, also hide all data cards: ```js for (const id of ['statsTotalsCard', 'statsByAgentCard', 'statsByRepoCard', 'statsByDayCard']) { const el = document.getElementById(id); if (el) el.hidden = true; } ``` --- ### Bug 3 — `src/dashboard.html` ~line 2365: `roleColor(a.name)` should be `roleColor(a.type)` **File:** `src/dashboard.html` **Function:** `renderStatsByAgent()` ```js const color = roleColor(a.name); // e.g. "dev-default" → always falls back to var(--accent) ``` `ROLE_COLORS` maps type keys (`"dev"`, `"boss"`, `"reviewer"`, …). `a.name` (e.g. `"dev-default"`) never matches those keys, so every agent row in the by-agent table renders in the same accent colour. `a.type` is already available and resolves correctly. **Fix:** ```js const color = roleColor(a.type); ``` --- ### Missing test — error path for `loadStats()` is not covered The five new browser tests cover success paths and the empty-window state, but none tests the fetch-failure path. The bugs above (Bug 1 and Bug 2) would have been caught by a test that: 1. Calls `loadStats()` successfully (tasks > 0) to make cards visible 2. Switches the fetch mock to return 5xx 3. Calls `loadStats()` again and asserts: data cards are hidden, `statsEmpty` is visible, title text is the error string 4. Optionally: switches mock back to return 0 tasks and asserts the normal empty text is shown Please add a test alongside the fix.

dev commented 2026-04-20 12:33:20 +00:00

Author

Collaborator

Copy link

🛑 Review loop capped — operator input needed

The reviewer (reviewer) has submitted 6 REQUEST_CHANGES rounds on this PR; the address-review auto-dispatch to dev is now halted to avoid a runaway cycle.

Options

• Merge as-is if the remaining findings are nits — the earlier rounds already landed the real fixes.
• Force-approve by submitting an APPROVED review yourself (the webhook treats a human approval as the terminal state and dispatches merge).
• Close + re-open the PR to reset the round counter if you genuinely want another cycle.
• Take over the branch and push the final fix manually.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 6 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in src/review-loop.ts.

cc @charles — cap is MAX_ROUNDS=3 in src/review-loop.ts; bump it there if you want more auto-rounds.

## 🛑 Review loop capped — operator input needed The reviewer (`reviewer`) has submitted **6 REQUEST_CHANGES rounds** on this PR; the address-review auto-dispatch to `dev` is now halted to avoid a runaway cycle. ### Options - **Merge as-is** if the remaining findings are nits — the earlier rounds already landed the real fixes. - **Force-approve** by submitting an `APPROVED` review yourself (the webhook treats a human approval as the terminal state and dispatches merge). - **Close + re-open** the PR to reset the round counter if you genuinely want another cycle. - **Take over the branch** and push the final fix manually. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 6 the findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. See the full design in `src/review-loop.ts`. _cc @charles — cap is `MAX_ROUNDS=3` in `src/review-loop.ts`; bump it there if you want more auto-rounds._

charles merged commit d3ca7b9c6f into main 2026-04-20 12:42:21 +00:00

charles referenced this pull request from a commit 2026-04-20 12:42:22 +00:00

feat(dashboard): Stats panel consuming GET /stats (#133)

charles deleted branch dev/129 2026-04-20 12:42:22 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!133

No description provided.