diff --git a/apps/web/__tests__/HeldCountBadge.test.tsx b/apps/web/__tests__/HeldCountBadge.test.tsx index a1f644172..82d5a6621 100644 --- a/apps/web/__tests__/HeldCountBadge.test.tsx +++ b/apps/web/__tests__/HeldCountBadge.test.tsx @@ -1,38 +1,425 @@ -import { describe, it, expect, afterEach } from 'vitest'; -import { render, screen, cleanup } from '@testing-library/react'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor, act } from '@testing-library/react'; +import type { HeldMessage } from '@cluesmith/codev-types'; import { HeldCountBadge } from '../src/components/HeldCountBadge.js'; afterEach(cleanup); +/** A held row with sensible defaults; override only what a test cares about. */ +function row(over: Partial = {}): HeldMessage { + return { + id: 'row-1', + workspacePath: '/ws', + toAgent: 'cost', + fromAgent: 'architect', + reason: 'busy', + escalated: false, + createdAt: Date.now() - 90_000, // 1m ago + notBefore: null, + ...over, + }; +} + +const noMessages = () => Promise.resolve([]); + +/** Open the panel and wait for its first load to settle. */ +async function open(): Promise { + fireEvent.click(screen.getByTestId('held-badge')); + return screen.findByTestId('held-popover'); +} + describe('HeldCountBadge', () => { + // ---------------------------------------------------------------- Spec 1313 contract + // These five predate Issue 1450 and must keep passing unchanged: the badge's + // count-only / zero-state / attention behaviour is not what the popover changed. + it('renders nothing when the count is 0', () => { - const { container } = render(); + const { container } = render(); expect(container.firstChild).toBeNull(); expect(screen.queryByTestId('held-badge')).toBeNull(); }); it('renders nothing for a negative count (defensive)', () => { - const { container } = render(); + const { container } = render(); expect(container.firstChild).toBeNull(); }); it('shows the held count when greater than 0', () => { - render(); + render(); expect(screen.getByTestId('held-badge')).toBeTruthy(); expect(screen.getByText('3 held')).toBeTruthy(); }); it('is not in the attention state when not escalated', () => { - render(); + render(); const badge = screen.getByTestId('held-badge'); expect(badge.className).not.toContain('held-badge--attention'); expect(badge.querySelector('.held-dot--attention')).toBeNull(); }); it('enters the attention state (pulsing dot) when escalated', () => { - render(); + render(); const badge = screen.getByTestId('held-badge'); expect(badge.className).toContain('held-badge--attention'); expect(badge.querySelector('.held-dot--attention')).toBeTruthy(); }); + + // ---------------------------------------------------------------- Issue 1450: affordance + + it('is a button wired as a disclosure, collapsed by default', () => { + render(); + const badge = screen.getByTestId('held-badge'); + // A real + , + ); + + await open(); + fireEvent.mouseDown(screen.getByText('elsewhere')); + + expect(screen.queryByTestId('held-popover')).toBeNull(); + }); + + it('stays open when clicking inside the panel', async () => { + render( Promise.resolve([row()])} />); + const panel = await open(); + + fireEvent.mouseDown(panel); + + expect(screen.getByTestId('held-popover')).toBeTruthy(); + }); + + it('detaches its document listeners on unmount', async () => { + const removeSpy = vi.spyOn(document, 'removeEventListener'); + const { unmount } = render( + Promise.resolve([row()])} />, + ); + await open(); + unmount(); + + const removed = removeSpy.mock.calls.map((c) => c[0]); + expect(removed).toContain('keydown'); + expect(removed).toContain('mousedown'); + removeSpy.mockRestore(); + }); }); diff --git a/apps/web/__tests__/heldMail.test.ts b/apps/web/__tests__/heldMail.test.ts new file mode 100644 index 000000000..13a73fca9 --- /dev/null +++ b/apps/web/__tests__/heldMail.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import { formatHeldAge, formatHeldDuration, isScheduled } from '../src/lib/heldMail.js'; + +describe('formatHeldDuration', () => { + it('renders sub-minute deltas in seconds', () => { + expect(formatHeldDuration(0)).toBe('0s'); + expect(formatHeldDuration(5_000)).toBe('5s'); + expect(formatHeldDuration(59_000)).toBe('59s'); + }); + + it('rolls over to minutes at 60s', () => { + expect(formatHeldDuration(60_000)).toBe('1m'); + expect(formatHeldDuration(59 * 60_000)).toBe('59m'); + }); + + it('rolls over to hours at 60m', () => { + expect(formatHeldDuration(60 * 60_000)).toBe('1h'); + expect(formatHeldDuration(23 * 60 * 60_000)).toBe('23h'); + }); + + it('rolls over to days at 24h', () => { + expect(formatHeldDuration(24 * 60 * 60_000)).toBe('1d'); + expect(formatHeldDuration(3 * 24 * 60 * 60_000)).toBe('3d'); + }); + + it('clamps a negative delta to 0s rather than rendering "-1s"', () => { + // Clock skew between the server's createdAt and the browser's Date.now() is real; + // a negative age must not leak into the UI. + expect(formatHeldDuration(-5_000)).toBe('0s'); + }); +}); + +describe('formatHeldAge', () => { + it('is the delta between now and createdAt', () => { + expect(formatHeldAge(1_000_000, 1_000_000 + 90_000)).toBe('1m'); + }); +}); + +describe('isScheduled', () => { + const now = 1_000_000; + + it('is false for a deliver-ASAP row (null notBefore)', () => { + expect(isScheduled(null, now)).toBe(false); + }); + + it('is false for a row whose due time has passed', () => { + expect(isScheduled(now - 1, now)).toBe(false); + }); + + it('is false exactly at the due time (matches the SQL boundary not_before <= now)', () => { + // The count query uses `not_before <= now`, so a row due exactly now IS eligible and + // IS counted. This predicate must agree, or the Held group would drift from the badge. + expect(isScheduled(now, now)).toBe(false); + }); + + it('is true for a pre-due row', () => { + expect(isScheduled(now + 15_000, now)).toBe(true); + }); +}); diff --git a/apps/web/src/components/App.tsx b/apps/web/src/components/App.tsx index 1f50e8310..e5de5ca4d 100644 --- a/apps/web/src/components/App.tsx +++ b/apps/web/src/components/App.tsx @@ -4,7 +4,7 @@ import { useTabs, type Tab } from '../hooks/useTabs.js'; import { useMediaQuery } from '../hooks/useMediaQuery.js'; import { useOverview } from '../hooks/useOverview.js'; import { MOBILE_BREAKPOINT } from '../lib/constants.js'; -import { getTerminalWsPath, createFileTab, removeArchitect as removeArchitectApi } from '../lib/api.js'; +import { getTerminalWsPath, createFileTab, fetchInbox, removeArchitect as removeArchitectApi } from '../lib/api.js'; import { readActiveArchitect, writeActiveArchitect } from '../lib/architectPersistence.js'; import { SplitPane } from './SplitPane.js'; import { TabBar } from './TabBar.js'; @@ -357,7 +357,11 @@ export function App() { {overviewTitle}
- + {state?.version && v{state.version}}
diff --git a/apps/web/src/components/HeldCountBadge.tsx b/apps/web/src/components/HeldCountBadge.tsx index e107b8fed..ce7e0fe7b 100644 --- a/apps/web/src/components/HeldCountBadge.tsx +++ b/apps/web/src/components/HeldCountBadge.tsx @@ -1,41 +1,252 @@ /** - * Spec 1313 Phase 8: compact held-mail count indicator for the dashboard header. + * Spec 1313 Phase 8 / Issue 1450: the dashboard header's held-mail indicator. * - * Read-only and count-only. It renders the number of currently-*held* (undelivered) - * mailbox rows in the workspace, fed by `OverviewData.heldCount` (which the overview - * refetches live on the `overview-changed` broadcast). When at least one held row has - * crossed the escalation age (`OverviewData.mailboxEscalated`) the badge enters an - * attention state — a pulsing amber dot — and clears back to normal when the row - * resolves. Dismissal stays CLI-only (`afx inbox`); this surface never mutates state - * (spec Decision 8). Renders nothing when the count is zero, so it stays out of the - * way until there is held mail. + * Renders the number of currently-*held* (undelivered) mailbox rows in the workspace, fed by + * `OverviewData.heldCount` (which the overview refetches live on the `overview-changed` + * broadcast). When at least one held row has crossed the escalation age + * (`OverviewData.mailboxEscalated`) the badge enters an attention state — a pulsing amber dot — + * and clears back to normal when the row resolves. Renders nothing when the count is zero, so + * it stays out of the way until there is held mail. * - * Presentational only (takes its data as props) so it unit-tests in isolation, mirroring - * `CloudStatus`. + * Issue 1450 made it a **disclosure button**: clicking it opens a panel listing each held + * message as `from → to` with its age and why-held reason, so "2 held" stops sending the user + * to a terminal to find out who is stuck. Still strictly READ-ONLY — dismissal remains CLI-only + * (`afx inbox dismiss`, spec Decision 8), and the list carries no message bodies (the redaction + * rule; `afx inbox show ` is the body path). + * + * ## Held vs Scheduled — why the panel groups rows + * + * The badge count and the list come from DIFFERENT queries and legitimately disagree. + * `heldSummaryForWorkspace` (the count) requires `not_before IS NULL OR not_before <= now`, so a + * pre-due `--delay` send is "scheduled, not stuck" and does not inflate the attention count. + * `listHeld` (behind `GET /api/inbox`) has no such filter and returns every held row. So + * `count <= messages.length`, always. + * + * Rather than hide that, the panel groups: **Held (N)** — where N is exactly the badge count — + * and a secondary **Scheduled (M)** section for pre-due rows, with their due countdown. Each + * group renders only when non-empty, so the ordinary case (no `--delay` in flight) looks like a + * single plain list. This mirrors `afx inbox`, which lists both and labels the pre-due ones + * `scheduled`. + * + * Consequence worth knowing: with 0 due and 1 scheduled row the badge does not render at all, + * so a scheduled-only state is invisible here. That is the existing contract — the badge is an + * *attention* indicator — and `afx inbox` remains the surface that sees it. + * + * Presentational: it takes a `loadMessages` loader rather than importing `fetchInbox`, so it + * unit-tests in isolation with a fake loader, mirroring `CloudStatus`. */ +import { useCallback, useEffect, useId, useRef, useState } from 'react'; +import type { HeldMessage } from '../lib/api.js'; +import { formatHeldAge, formatHeldDuration, isScheduled } from '../lib/heldMail.js'; + export interface HeldCountBadgeProps { - /** Count of currently-held rows across the workspace (`OverviewData.heldCount`). */ + /** Count of currently-held ELIGIBLE rows across the workspace (`OverviewData.heldCount`). */ count: number; /** True when at least one held row has crossed the escalation age. */ escalated: boolean; + /** + * Fetches the workspace's held rows. Called on open, and again whenever `count` changes + * while open. Injected so the component stays presentational and testable. + */ + loadMessages: () => Promise; +} + +/** What the panel is currently showing. */ +type LoadState = + | { kind: 'loading' } + | { kind: 'error'; message: string } + | { kind: 'ready'; messages: HeldMessage[] }; + +function HeldRow({ message, now }: { message: HeldMessage; now: number }) { + const scheduled = isScheduled(message.notBefore, now); + // `?` for a missing sender matches how `afx inbox` renders the same row. + const fromTo = `${message.fromAgent ?? '?'} → ${message.toAgent}`; + // A scheduled row shows its countdown to due time; a stuck one shows how long it has waited. + const when = scheduled + ? `→${formatHeldDuration(message.notBefore! - now)}` + : formatHeldAge(message.createdAt, now); + const reason = scheduled ? 'scheduled' : (message.reason ?? 'held'); + return ( +
  • + {fromTo} + + {when} · {reason} + {message.escalated && !scheduled ? '!' : ''} + +
    • + ); } -export function HeldCountBadge({ count, escalated }: HeldCountBadgeProps) { - if (count <= 0) { +export function HeldCountBadge({ count, escalated, loadMessages }: HeldCountBadgeProps) { + const [open, setOpen] = useState(false); + const [state, setState] = useState({ kind: 'loading' }); + // In-flight flag, separate from `state`: a refetch keeps the previous rows rendered, so + // "is a request outstanding" can no longer be read off the state union. + const [busy, setBusy] = useState(false); + const buttonRef = useRef(null); + const wrapperRef = useRef(null); + const panelId = useId(); + + // Discards a response whose request is no longer the current one. Without this, a fast + // open → close → open lands the FIRST (slower) response over the second's data; React 19 + // does not warn about setState on an unmounted/stale path, so the bug would be silent. + const generationRef = useRef(0); + + // The loader is read through a ref so `load` (and therefore the fetch effect) does not + // depend on the prop's IDENTITY. App.tsx passes the module-level `fetchInbox`, which is + // stable — but an inline lambda from any future caller would change identity every render + // and turn the effect into a refetch loop. Behaviour should not hinge on a caller + // remembering to memoize. + const loaderRef = useRef(loadMessages); + useEffect(() => { + loaderRef.current = loadMessages; + }, [loadMessages]); + + const load = useCallback(() => { + const generation = ++generationRef.current; + setBusy(true); + // Keep already-loaded rows on screen while refetching rather than blanking to "Loading…". + // A refetch fires whenever `count` changes while the panel is open, and flashing the list + // away is exactly the wrong moment to do it — the user is watching to see what moved. + // `aria-busy` carries the in-flight state instead. Only a cold open shows the spinner. + setState((prev) => (prev.kind === 'ready' ? prev : { kind: 'loading' })); + loaderRef.current().then( + (messages) => { + if (generationRef.current !== generation) return; + setBusy(false); + setState({ kind: 'ready', messages }); + }, + (err: unknown) => { + if (generationRef.current !== generation) return; + setBusy(false); + // An error DOES replace the rows: once a refetch has failed, the previous list is no + // longer known to be current, and showing it as if it were would be a lie. + setState({ kind: 'error', message: err instanceof Error ? err.message : String(err) }); + }, + ); + // No deps: the loader is reached through `loaderRef`, so `load` is stable for the + // lifetime of the component and the fetch effect fires only on open / count change. + }, []); + + // Load on open, and again when `count` changes while open. Refetch rather than snapshot: + // the count is SSE-driven, so a change means the mailbox actually moved — precisely when a + // user staring at the open panel would expect it to follow. + useEffect(() => { + if (!open) return; + load(); + }, [open, count, load]); + + // Escape closes and returns focus to the button (the disclosure pattern's keyboard contract). + // Click-outside closes without moving focus, since the user is already looking elsewhere. + useEffect(() => { + if (!open) return; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape') { + setOpen(false); + buttonRef.current?.focus(); + } + }; + const onPointerDown = (e: MouseEvent) => { + if (!wrapperRef.current?.contains(e.target as Node)) setOpen(false); + }; + document.addEventListener('keydown', onKeyDown); + document.addEventListener('mousedown', onPointerDown); + return () => { + document.removeEventListener('keydown', onKeyDown); + document.removeEventListener('mousedown', onPointerDown); + }; + }, [open]); + + // Normally the badge disappears at zero. But `useOverview` polls every 2.5s, so the last + // held row being delivered WHILE the panel is open would unmount the button mid-interaction + // and drop focus to . So when open, stay mounted and say the mail cleared; the user's + // own close unmounts us. The closed-at-zero contract is unchanged. + if (count <= 0 && !open) { return null; } + const label = `${count} held`; const title = escalated - ? `${count} held message${count === 1 ? '' : 's'} — at least one past the escalation age. Review with: afx inbox` - : `${count} held message${count === 1 ? '' : 's'} awaiting a clear prompt. Review with: afx inbox`; + ? `${count} held message${count === 1 ? '' : 's'} — at least one past the escalation age. Click to list them.` + : `${count} held message${count === 1 ? '' : 's'} awaiting a clear prompt. Click to list them.`; + + // `now` is sampled per render rather than ticked on a timer: the panel is a triage glance, + // and every refetch re-renders anyway. A live-ticking age would be motion for its own sake. + const now = Date.now(); + const messages = state.kind === 'ready' ? state.messages : []; + const heldRows = messages.filter((m) => !isScheduled(m.notBefore, now)); + const scheduledRows = messages.filter((m) => isScheduled(m.notBefore, now)); + return ( - - - {label} - +
    + + {/* aria-live: the rows arrive asynchronously after the panel opens, so without it a + screen reader announces an empty container and never mentions the messages. */} + {open && ( +
    + {state.kind === 'loading' &&

    Loading…

    } + {state.kind === 'error' && ( +

    + Could not load held messages: {state.message} +

    + )} + {/* Keyed on heldRows, not messages: when the held rows drain but a SCHEDULED row + remains, the panel is not empty yet there is still nothing held — and the + Scheduled group's "not counted above" needs something above it to refer to. */} + {state.kind === 'ready' && heldRows.length === 0 && ( +

    + {count <= 0 && messages.length > 0 ? 'No held messages — the rows below are scheduled.' + : count <= 0 ? 'Held mail cleared.' + : 'No held messages.'} +

    + )} + {heldRows.length > 0 && ( +
    +

    Held ({heldRows.length})

    +
      + {heldRows.map((m) => ( + + ))} +
    +
    + )} + {scheduledRows.length > 0 && ( +
    +

    Scheduled ({scheduledRows.length})

    +

    + Waiting for their due time — not counted above. +

    +
      + {scheduledRows.map((m) => ( + + ))} +
    +
    + )} + {/* Points at `afx inbox` rather than `afx inbox dismiss `: dismissal needs a row + id, and this panel deliberately shows none (a full uuid would dominate the row + and this surface never mutates anyway). `afx inbox` is where the ids live. */} +

    Ids and dismissal: afx inbox

    +
    + )} +
    ); } diff --git a/apps/web/src/index.css b/apps/web/src/index.css index ffbcc68b6..27b22b731 100644 --- a/apps/web/src/index.css +++ b/apps/web/src/index.css @@ -894,6 +894,13 @@ body { /* Spec 1313 Phase 8: held-mail count indicator in the dashboard header. Count-only, read-only; enters an attention state (amber pulse, reusing @keyframes cloud-pulse) when a held row has crossed the escalation age. */ +/* Issue 1450: the badge is a disclosure button anchoring a popover. `.header-controls` is a + plain flex row with no positioning context, so the wrapper provides one. */ +.held-badge-wrapper { + position: relative; + display: inline-flex; +} + .held-badge { display: inline-flex; align-items: center; @@ -901,6 +908,26 @@ body { font-size: 12px; color: var(--text-secondary); white-space: nowrap; + /* Button reset — this was a until Issue 1450. */ + background: none; + border: none; + padding: 0; + font-family: inherit; + cursor: pointer; + /* The affordance: dotted underline reads as "expands to reveal", not as a link. */ + text-decoration: underline dotted; + text-underline-offset: 3px; +} + +.held-badge:hover { + color: var(--text-primary); + text-decoration: underline solid; +} + +.held-badge:focus-visible { + outline: 2px solid var(--accent); + outline-offset: 2px; + border-radius: 3px; } .held-badge--attention { @@ -908,6 +935,104 @@ body { font-weight: 600; } +/* The popover sits at the 1000 tier (with the modal backdrop): xterm's WebGL/canvas panes + paint over anything at the lower 20/100 tiers. */ +.held-popover { + position: absolute; + top: calc(100% + 8px); + right: 0; + z-index: 1000; + min-width: 280px; + max-width: 380px; + max-height: 320px; + overflow-y: auto; + padding: 10px 12px; + background: var(--bg-secondary); + border: 1px solid var(--border-color); + border-radius: 6px; + box-shadow: 0 8px 24px rgba(0, 0, 0, 0.35); + font-size: 12px; + text-align: left; + cursor: default; +} + +.held-popover-note { + margin: 0; + color: var(--text-secondary); +} + +.held-popover-note--error { + color: var(--status-error); +} + +.held-group + .held-group { + margin-top: 12px; + padding-top: 10px; + border-top: 1px solid var(--border-color); +} + +.held-group-title { + margin: 0 0 6px; + font-size: 11px; + font-weight: 600; + text-transform: uppercase; + letter-spacing: 0.04em; + color: var(--text-muted); +} + +.held-group-note { + margin: -2px 0 6px; + font-size: 11px; + font-style: italic; + color: var(--text-muted); +} + +.held-list { + margin: 0; + padding: 0; + list-style: none; +} + +.held-row { + display: flex; + flex-direction: column; + gap: 1px; + padding: 4px 0; +} + +.held-row + .held-row { + border-top: 1px solid var(--border-color); +} + +.held-row-addresses { + color: var(--text-primary); + font-family: ui-monospace, monospace; + word-break: break-word; +} + +.held-row-meta { + font-size: 11px; + color: var(--text-muted); +} + +.held-row--attention .held-row-meta { + color: var(--status-waiting); + font-weight: 600; +} + +.held-popover-foot { + margin: 10px 0 0; + padding-top: 8px; + border-top: 1px solid var(--border-color); + font-size: 11px; + color: var(--text-muted); +} + +.held-popover-foot code { + font-family: ui-monospace, monospace; + font-size: 11px; +} + .held-dot { display: inline-block; width: 8px; diff --git a/apps/web/src/lib/api.ts b/apps/web/src/lib/api.ts index 7a2787997..25cd04dbb 100644 --- a/apps/web/src/lib/api.ts +++ b/apps/web/src/lib/api.ts @@ -91,6 +91,7 @@ export type { OverviewBacklogItem, OverviewRecentlyClosed, OverviewData, + HeldMessage, ProtocolStats, AnalyticsResponse, } from '@cluesmith/codev-types'; @@ -100,6 +101,7 @@ import type { AnalyticsResponse, TeamApiResponse, OverviewData, + HeldMessage, DashboardState, } from '@cluesmith/codev-types'; @@ -123,6 +125,23 @@ export async function fetchOverview(): Promise { return res.json(); } +/** + * Issue 1450: the workspace's currently-held mailbox rows — the payload behind the + * held-mail popover, and the same projection `afx inbox` renders. + * + * Hits the workspace-scoped `GET /api/inbox`, which resolves the workspace server-side from + * the `/workspace//` URL prefix (the dashboard has no absolute workspace path of its + * own). Metadata only — never message bodies. + * + * Called lazily, on popover open, rather than folded into the 2.5s overview poll: this is a + * cold path that only matters when the user asks. + */ +export async function fetchInbox(): Promise { + const res = await fetch(apiUrl('api/inbox'), { headers: getAuthHeaders() }); + if (!res.ok) throw new Error(`Failed to fetch held messages: ${res.status}`); + return res.json(); +} + export async function refreshOverview(): Promise { await fetch(apiUrl('api/overview/refresh'), { method: 'POST', diff --git a/apps/web/src/lib/heldMail.ts b/apps/web/src/lib/heldMail.ts new file mode 100644 index 000000000..a06d5e769 --- /dev/null +++ b/apps/web/src/lib/heldMail.ts @@ -0,0 +1,48 @@ +/** + * Issue 1450: age/countdown formatting for the dashboard's held-mail popover. + * + * Ported — deliberately, not imported — from the `afx inbox` CLI renderer + * (`packages/codev/src/agent-farm/commands/inbox.ts:77-90`). The web app must not import + * from `@cluesmith/codev-core` (server/client isolation, #1189), and `@cluesmith/codev-types` + * is a types-only devDependency, the wrong home for a runtime helper. Ten lines duplicated + * beats a boundary violation; keeping the output identical to the CLI's is what lets a + * reviewer check the popover against `afx inbox` row for row. + * + * Named `formatHeldAge` rather than `formatDuration` because + * `apps/web/src/lib/open-files-shells-utils.ts` already exports a `formatDuration` with + * DIFFERENT semantics (minute granularity, `<1m` floor) and existing callers. Two + * same-named formatters with different output in one `lib/` is a trap. + */ + +/** + * Compact human duration ("5s", "3m", "2h", "1d") from a millisecond delta. + * Second-granularity, matching the CLI — held mail is often seconds old, and "<1m" + * would erase the distinction between "just held" and "held for most of a minute". + * Negative deltas clamp to `0s`. + */ +export function formatHeldDuration(ms: number): string { + const secs = Math.max(0, Math.floor(ms / 1000)); + if (secs < 60) return `${secs}s`; + const mins = Math.floor(secs / 60); + if (mins < 60) return `${mins}m`; + const hours = Math.floor(mins / 60); + if (hours < 24) return `${hours}h`; + return `${Math.floor(hours / 24)}d`; +} + +/** Compact human age ("5s", "3m", "2h", "1d") from an epoch-ms timestamp. */ +export function formatHeldAge(createdAt: number, now: number): string { + return formatHeldDuration(now - createdAt); +} + +/** + * Is this row a pre-due `--delay` send — scheduled rather than stuck? + * + * The single predicate that splits the popover's two groups, and the same test the CLI + * applies (`inbox.ts:135`). A scheduled row is excluded from `OverviewData.heldCount` by + * `heldSummaryForWorkspace`, so grouping on exactly this predicate is what makes the + * "Held" group's length equal the badge count. + */ +export function isScheduled(notBefore: number | null, now: number): boolean { + return notBefore != null && notBefore > now; +} diff --git a/codev/plans/1450-dashboard-make-the-held-mail-c.md b/codev/plans/1450-dashboard-make-the-held-mail-c.md new file mode 100644 index 000000000..ee9388e13 --- /dev/null +++ b/codev/plans/1450-dashboard-make-the-held-mail-c.md @@ -0,0 +1,385 @@ +# PIR Plan: Clickable held-mail counter with a held-messages popover + +Issue: [#1450](https://github.com/cluesmith/codev/issues/1450) — *Dashboard: make the held-mail +counter clickable, showing the held messages (from → to)* + +> **Revision 2** — amended after the architect's plan review (claude lane, Medium tier, +> REQUEST_CHANGES). The blocking finding was correct and I verified it myself; §"Held vs +> Scheduled" is the design that replaces the wrong risk entry. All IMPORTANT items and NITs are +> taken. Disposition list at the end. + +## Understanding + +The dashboard header renders a count-only held-mail indicator. `HeldCountBadge` +(`apps/web/src/components/HeldCountBadge.tsx:31-40`) is an inert ``: a dot, the text +`N held`, and a `title` tooltip whose only remedy is *"Review with: afx inbox"*. It is mounted +once, in the desktop header (`apps/web/src/components/App.tsx:360`), fed by +`OverviewData.heldCount` / `mailboxEscalated`. + +So the user sees "2 held" and must drop to a terminal to learn *who is held from whom*. The ask +is two things: (1) an affordance that reads as clickable, and (2) a panel on click listing each +held message with at least `from → to`. + +### Verifying the issue's claims against current `main` + +The issue was written 2026-08-17; I re-checked every claim against the tree at +`origin/main` (`9129ab81c`): + +| Issue claim | Verdict | +|---|---| +| The counter is inert text | **True.** `HeldCountBadge.tsx:31-40` — a `` with a `title`, no handler, no affordance. Last touched by Spec 1313's `0bbea9de4`; nothing since. | +| Finding out *what* is held needs `afx inbox -w ` | **True.** No dashboard surface lists held rows. | +| "Data is already available server-side (the same store `afx inbox` reads)" | **True.** `GET /api/inbox` → `handleInboxList` (`tower-routes.ts:199`, impl `:2134`) projects exactly the CLI's fields: `id`, `workspacePath`, `toAgent`, `fromAgent`, `reason`, `escalated`, `createdAt`, `notBefore`. | +| "a small read endpoint/**reuse of an existing one**" | **Needs one correction.** `GET /api/inbox` is registered only on the *Tower-level* route table. The dashboard is served under `/workspace//` and calls its API with relative `./api/...` (`getApiBase()` returns `'./'`, `apps/web/src/lib/constants.ts`), which lands in the **workspace-scoped** dispatcher (`tower-routes.ts:2484-2723`). That dispatcher has no `inbox` branch, so `./api/inbox` currently 404s. The fix is a three-line branch that reuses `handleInboxList` — the same pattern `overview`, `analytics`, and `architects/:name` already use. No new handler, no new projection. | + +One more fact that shapes the design: **the dashboard does not know its own workspace path.** +It never reads the encoded prefix out of `window.location` for API purposes — the server +resolves the workspace from the URL prefix. That is why calling the Tower-level +`/api/inbox?workspace=` from the browser is not an option, and why the workspace-scoped +branch (which passes `workspacePath` as an override) is the right seam. + +## Held vs Scheduled — the count and the list do not agree, by design + +*(This section replaces the incorrect risk entry in revision 1, which claimed pre-due rows +"already inflate `heldCount`". They do not. Verified directly against `db/mailbox.ts`.)* + +Two different queries back the two surfaces, and they deliberately disagree: + +- **The badge count** comes from `heldSummaryForWorkspace` (`db/mailbox.ts:215-227`), whose SQL + filters `status = 'held' AND (not_before IS NULL OR not_before <= ?)`. Its docstring is + explicit: a pre-due `--delay` send is *"scheduled, not stuck"* and **must NOT inflate the + attention count/indicator**. +- **The list** comes from `listHeld` (`db/mailbox.ts:113-124`), which `handleInboxList` calls and + which has **no `not_before` filter at all** — it returns every `held` row. The same docstring + confirms the intent: *"Pre-due rows are still visible in `afx inbox`, which lists ALL held rows + and labels these 'scheduled' — only the count/alarm surfaces exclude them."* + +So a naive popover would say "2 held" on the badge and list 3 rows. That is not a bug to paper +over — it is an intentional split between an *attention* count and an *inventory* list, and the +UI has to render the split rather than hide it. + +**Design (option (a) from the review — popover groups):** + +- The popover has two sections. **"Held (N)"** lists rows that are due (`notBefore == null || + notBefore <= now`). **N is exactly the badge count**, so the number the user clicked and the + number of rows in the first group always match. +- **"Scheduled (M)"** is a separate, visually-secondary section listing pre-due rows with their + due countdown (`→15s`), carrying one line of copy: *"Scheduled sends — waiting for their due + time, not counted above."* +- Each group renders only when non-empty, so the common case (no `--delay` in flight) is a single + ungrouped-looking list. + +This also keeps the popover a faithful mirror of `afx inbox`, which shows both kinds and labels +the pre-due ones `scheduled` (`commands/inbox.ts:135-137`). + +**Accepted edge case, stated rather than fixed:** with 0 due and 1 scheduled row, `heldCount` is +0, the badge renders nothing, and the scheduled row is not reachable from the dashboard. That is +the existing, deliberate contract — the badge is an *attention* indicator and a scheduled send is +not attention-worthy. Surfacing it would mean rendering a badge whose count is 0, which +contradicts `heldSummaryForWorkspace`'s stated purpose and the badge's own zero-state test. +`afx inbox` remains the surface that sees scheduled-only state. If the architect wants that +reachable from the dashboard, it is a separate change to what the badge *counts*, not to this +popover. + +Pinned by test: seed 1 due + 1 pre-due row, render with `count={1}`, assert the "Held" group has +exactly 1 row and the "Scheduled" group has exactly 1, and that the Held-group length equals the +badge count. + +## Proposed Change + +### Server — reuse `handleInboxList` under the workspace prefix + +Give `handleInboxList` an optional `workspaceOverride` third parameter, mirroring +`handleOverview(res, url, workspaceOverride?, ctx?)` (`tower-routes.ts:1108-1110`) and +`handleAnalytics` (`:1382`). Resolution is `workspaceOverride ?? url.searchParams.get('workspace')` +— **the override wins**, so a workspace-scoped call can never be redirected to another workspace +by an attacker-supplied `?workspace=`. (`??` rather than `||` so an empty-string override is +still an override rather than silently falling through.) The docstring will note that the +override arrives already normalized by the prefix decoder (`tower-routes.ts:2476`), so +`normalizeWorkspacePath` re-running on it is a safe no-op. The Tower-level registration is +unchanged, so `afx inbox` and any direct caller keep their exact current semantics. + +Then add one branch to the workspace-scoped API dispatcher, next to the existing `overview` one: + +```ts +// GET /api/inbox — held mailbox rows for THIS workspace (Issue 1450). Reuses the +// Tower-level handler with the workspace resolved from the /workspace// prefix, +// the same way `overview` and `analytics` do. Metadata-only projection: never bodies. +if (req.method === 'GET' && apiPath === 'inbox') { + return handleInboxList(res, url, workspacePath); +} +``` + +Deliberately **exact-match `'inbox'` only**: `inbox/:id` (show, carries the body) and +`inbox/:id/dismiss` (mutating) do not match and fall through to the dispatcher's 404, so they stay +off the dashboard surface. That preserves two Spec 1313 rules the current badge docstring already +states — the redaction rule (bodies never leave the CLI/terminal path) and decision 8 (dismissal +is CLI-only; this surface never mutates state). `afx inbox show ` remains the deep-dive path, +as the issue itself allows. Authentication needs no new work: the request passes through Tower's +`isRequestAllowed` chokepoint before reaching the dispatcher, so the new branch inherits the +shared-key check. + +### Types — one shared projection type + +Add `HeldMessage` to `packages/types/src/api.ts` describing the `handleInboxList` projection. +Both sides of the server/client boundary may import `codev-types` (arch invariant #1189), so the +web app gets the shape without reaching into `codev-core`. The CLI's private `InboxRow` +(`packages/codev/src/agent-farm/commands/inbox.ts:21-35`) is left alone — retyping it is a +separate cleanup, not this issue. + +### Web — the badge becomes a disclosure button with a popover + +`HeldCountBadge` keeps its presentational character (its docstring calls that out explicitly, and +it is why the component unit-tests in isolation), but gains disclosure state and a **`loadMessages` +loader prop** rather than importing `fetchInbox` directly. Tests inject a fake loader; `App.tsx` +passes the real one. New shape: + +```ts +export interface HeldCountBadgeProps { + count: number; + escalated: boolean; + /** Fetches the workspace's held rows. Called lazily, on open and on count change. */ + loadMessages: () => Promise; +} +``` + +**Affordance.** The `` becomes a `