Dashboard: make the held-mail counter clickable, showing the held messages (from → to)

[mohidmakhdoomi]

PIR Review: Clickable held-mail counter with a held-messages popover

Fixes #1450

Summary

The dashboard header's held-mail counter ("2 held") was inert text, so learning what was held
meant dropping to afx inbox in a terminal. It is now a disclosure button — dotted underline,
aria-expanded/aria-controls — that opens a panel listing each held message as from → to
with its age and why-held reason. Server-side this reuses the existing handleInboxList behind a
new workspace-scoped GET /api/inbox branch; no new handler and no new projection, so the
metadata-only redaction rule and CLI-only dismissal (Spec 1313 decision 8) are untouched.

The one genuinely subtle part is that the badge count and the list disagree by design — see
Things to Look At.

Files Changed

• apps/web/src/components/HeldCountBadge.tsx (+219 / -…) — span → disclosure button, grouped popover, generation-guarded lazy fetch
• apps/web/src/index.css (+125 / -0) — underline affordance, popover panel, z-index tier
• apps/web/src/lib/heldMail.ts (+48 / -0) — new; formatHeldAge / formatHeldDuration / isScheduled
• apps/web/src/lib/api.ts (+19 / -0) — fetchInbox
• apps/web/src/components/App.tsx (+8 / -3) — wire loadMessages
• packages/codev/src/agent-farm/servers/tower-routes.ts (+30 / -3) — workspaceOverride param + workspace-scoped branch
• packages/types/src/api.ts (+46 / -0) — HeldMessage
• packages/types/src/index.ts (+1 / -0) — export it
• apps/web/__tests__/HeldCountBadge.test.tsx — 32 cases total: 5 originals kept unchanged, 27 new
• apps/web/__tests__/heldMail.test.ts — new, 10 cases
• packages/codev/src/agent-farm/__tests__/inbox-routes.test.ts — 12 new route cases (14 → 26 in the file)
• packages/codev/scripts/issue-1450-dashboard-evidence.mts (+370 / -0) — new; real-browser evidence harness
• codev/plans/1450-dashboard-make-the-held-mail-c.md, codev/state/pir-1450_thread.md, codev/resources/arch.md, codev/resources/lessons-learned.md — artifacts and governance

Commits

• b6c15774e [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] Plan draft
• 934e9354e [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] Plan revised: Held/Scheduled split, a11y disclosure pattern, isolated-Tower Playwright
• 5a782c0b7 [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] Plan: use AF_TEST_DB isolation seam for the evidence Tower (not a HOME redirect)
• d70e3503f [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] feat: clickable held-mail counter with a held-messages popover
• 7483bcce9 [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] Review + retrospective
• 2cb2b3623 [PIR Dashboard: make the held-mail counter clickable, showing the held messages (from → to) #1450] Address 3-way consult + architect integration review

Test Results

• pnpm build: ✓ pass
• pnpm test (@cluesmith/codev): ✓ 4917 passed, 48 skipped, 0 failures
• apps/web (pnpm --filter @cluesmith/codev-web test): ✓ 33 files — 37 new web tests (27 component + 10 formatter)
  • Note: the root test script runs only the codev package; web tests need the filtered command.
• 49 new tests total across the three files (27 + 10 + 12).
• Manual, real browser: packages/codev/scripts/issue-1450-dashboard-evidence.mts — 23/23 checks
  in headless Chromium against an isolated Tower (worktree build, port 14700, NODE_ENV=test +
  AF_TEST_DB). Real workspace, real shellper PTYs painted with an occupied composer, real
  POST /api/send held by the render gate, real built SPA. Asserts the underline affordance,
  aria-expanded toggling, from → to rows, the Held/Scheduled grouping,
  GET .../api/inbox returning 200, Held-group length === badge count, popover stacking over
  a live terminal, and Escape-closes-with-focus-return.
• Human review at the dev-approval gate: the reviewer exercised an interactive instance of
  that same environment personally (2 held + 1 scheduled fixture) and approved.

Architecture Updates

Routed COLD only (codev/resources/arch.md). Neither hot file was touched: both are at their
10-entry cap, and nothing here is a cross-cutting invariant worth displacing an existing entry —
these are subsystem facts about Tower's routing and the mailbox, which is what the cold archive is
for.

Two additions:

1. Agent Farm Internals → "Two route tables, and why a 'working' endpoint can still 404 for the
   dashboard." tower-routes.ts dispatches through two independent tables — the Tower-level
   ROUTES map (/api/<thing>, used by the CLI) and handleWorkspaceRoutes
   (/workspace/<b64>/api/<thing>, used by the dashboard, whose getApiBase() returns './').
   Registering in one does not register in the other, which is exactly why this issue existed:
   GET /api/inbox had backed afx inbox since Spec 1313 while ./api/inbox 404'd for the
   dashboard. Also records why workspace-scoped handlers take a workspaceOverride that wins
   over ?workspace=, and that the exact-vs-prefix match is a security boundary.
2. Mailbox section — the count/list asymmetry, written as "do not 'fix' it" (details below),
   including the corollary that a scheduled-only state is invisible to the badge by design.

Lessons Learned Updates

Routed COLD only (codev/resources/lessons-learned.md → Testing). Both entries are about
browser-test technique — useful, but not the always-injected kind, and the hot file is at cap.

1. A browser assertion can pass while proving nothing — assert its precondition in the same
   test. My z-index check confirmed the popover was on top at its centre point and was green
   and worthless: querySelector('.xterm') had returned the left pane's terminal, which can
   never overlap a top-right panel, so the actual hazard (xterm's WebGL canvas painting over an
   unlayered element) was never exercised. The fix asserts the setup — "a terminal genuinely
   overlaps this rect" — before asserting the property. Same shape as artifact-canvas: remote command channel for review navigation (Tower relay + sdk route) #1401's guard lesson.
2. Two Playwright mechanics specific to this dashboard: waitUntil: 'networkidle' can never
   fire (the SSE stream at /api/events stays open for the page's lifetime), and a lazily-fetched
   panel must be waited on by its loaded content or assertions read the "Loading…" state. Plus:
   assert the status code of the endpoint under test, because an auth failure renders as a
   tidy error state that looks like a working UI.

Things to Look At During PR Review

1. The Held/Scheduled split — the part that took two attempts to get right.
My first plan asserted that pre-due --delay rows "already inflate heldCount". That was
backwards, and the architect's review caught it. Verified against db/mailbox.ts:

• heldSummaryForWorkspace (:215-227) — the badge count — filters
  not_before IS NULL OR not_before <= now.
• listHeld (:113-124) — behind GET /api/inbox — has no such filter.

So heldCount <= inbox.length, always, deliberately: a scheduled send is "scheduled, not stuck"
and must not raise an attention indicator. A naive popover would say "2 held" and list 3 rows. The
panel therefore groups: Held (N) where N is exactly the badge count, above a secondary
Scheduled (M) with a countdown and explanatory copy. Groups render only when non-empty, so the
ordinary case looks like one plain list. Pinned by a unit test (1 due + 1 pre-due at count={1})
and re-verified live in the browser run, which reproduced exactly that fixture.

2. Accepted edge case, not an oversight. With 0 due and 1 scheduled row, heldCount is 0, the
badge does not render, and that row is unreachable from the dashboard. That is the existing
contract — the badge is an attention indicator — and afx inbox remains the surface that sees
it. Surfacing it would mean rendering a badge whose count is 0. Documented on the component, on
the HeldMessage type, and in arch.md; changing it is a change to what the badge counts.

3. The exact-match on 'inbox' is load-bearing. inbox/:id returns the message body and
inbox/:id/dismiss mutates. Both must stay unreachable under the workspace prefix for the
"metadata-only, read-only" claim to hold. Three tests pin that (both 404, and the row survives a
POST) rather than leaving it to inspection.

4. formatHeldAge, not formatDuration. apps/web/src/lib/open-files-shells-utils.ts:2-10
already exports a formatDuration with different semantics (minute granularity, <1m floor) and
existing callers. The new helper is second-granularity to match afx inbox, so it is separately
named rather than merged — two same-named formatters with different output in one lib/ is a
trap. It is a deliberate ~10-line port, not an import: the web app must not cross the
server/client isolation boundary (#1189), and codev-types is a types-only devDependency.

5. Fetch lifecycle. Loads on open and on count change while open (the count is SSE-driven, so
a change means the mailbox moved). Each load carries a generation counter; stale responses are
discarded — React 19 would not warn about the open→close→open race. And while the panel is open
the component stays mounted even at count <= 0, because useOverview polls every 2.5s and the
last row being delivered would otherwise unmount the button and drop focus to <body>. The
closed-at-zero contract is unchanged, so the original zero-state tests pass untouched.

6. Not a dialog. Deliberately the WAI-ARIA disclosure pattern (aria-expanded +
aria-controls + a real <ul>), not role="dialog" — a dialog role without moving focus in is
announced inconsistently, and this panel should not steal focus from the terminals.

How to Test Locally

• View diff: VSCode sidebar → right-click builder pir-1450 → Review Diff
• Run the browser evidence (needs an out-of-tree Playwright; see the script header):

  pnpm build
  npm install playwright-core --prefix /tmp/pw
  PW_CORE=/tmp/pw/node_modules/playwright-core \
  PW_CHROMIUM=~/.cache/ms-playwright/chromium-*/chrome-linux64/chrome \
  node --experimental-strip-types packages/codev/scripts/issue-1450-dashboard-evidence.mts

  It stands up and tears down its own isolated Tower on port 14700; the live Tower on 4100 and the
  real global.db are never touched.
• What to verify:
  • The counter is underlined and reads as interactive; clicking opens the panel.
  • Rows show from → to (a null sender renders ?, matching afx inbox), with age and reason.
  • With a --delay send in flight, Held (N) matches the badge and Scheduled (M) is separate.
  • Escape closes and returns focus to the counter; click-outside closes; a terminal behind the
    panel does not paint over it.
  • On a normally-running dashboard, the union of both groups matches afx inbox -w <workspace>
    row for row, and the Held group alone matches the badge count.

Post-Consultation Fixes

The 3-way consult and the architect's integration review both landed as non-blocking
(APPROVE / COMMENT / APPROVE). Every finding was verified against the files before acting; all
were real and all are fixed in 2cb2b3623 (see the PR's commit list).

Finding	Source	Disposition
handleInboxList's docstring claimed an empty workspaceOverride stays scoped, but rawWorkspace ? … : undefined widened it to all workspaces	Codex + architect (3)	Real. Unreachable today (the dispatcher 400s a missing/relative prefix first), but the comment promised a guarantee the code did not make. Made the code true rather than weakening the comment: a scoped call with a blank override now scopes to '', which matches no rows. The safe failure for a scoped call is zero rows, never every row.
Review file's test counts were wrong	Codex	Real. Recounted from the merge-base: 27 new component + 10 formatter + 12 route = 49, not the 37 originally claimed. Corrected above.
count → 0 with scheduled rows left the panel with no "cleared" notice, and Scheduled's "not counted above" had nothing above it	Codex	Real. The notice now keys on heldRows.length === 0 rather than messages.length === 0, with copy that distinguishes "cleared" from "nothing held, rows below are scheduled".
arch.md edit swallowed the pre-existing pruning/cron sentences into the new count-vs-list paragraph	Claude + architect (2)	Real — content survived but read as part of the Held/Scheduled discussion. Paragraph break restored. (Second splice of this kind in this project; the first was caught in packages/types/src/api.ts before commit.)
loadMessages prop identity drove the refetch effect — an inline lambda from a future caller would loop	Claude + architect (4)	Real footgun. Latched in a ref; load now has empty deps, so behaviour no longer depends on a caller remembering to memoize.
Popover lacked aria-live, so asynchronously-loaded rows were never announced	Claude	Real. Added aria-live="polite" + aria-busy.
Footer said afx inbox dismiss <id> but no id is rendered anywhere	Claude	Real. Reworded to Ids and dismissal: afx inbox. Rendering a full uuid per row would dominate the row, and this surface never mutates.
Server projection was untyped — HeldMessage was client-side decoration only	architect (1)	Real. Annotated const projected: HeldMessage[], so a drifting projection (dropped field, or a body slipping in) fails the server build.
Keep prior rows during refetch instead of blanking to "Loading…"	architect (5), optional	Taken. A refetch fires on every count change while open; flashing the list away is the worst moment to do it. aria-busy carries the in-flight state; only a cold open shows the spinner. An error still replaces the rows — once a refetch fails, the old list is no longer known to be current.
HeldMessage jsdoc typo /api/inbox:id	architect (6)	Not reproduced. The jsdoc already reads GET /api/inbox/:id (packages/types/src/api.ts:597). No change made.

Re-verified after the fixes: pnpm build ✓, web suite ✓ (373 passed), route suite ✓ (26 passed),
and the browser evidence re-run ✓ 23/23.

Flaky Tests

None. No pre-existing failures were encountered in the full suite, so nothing was
skipped or quarantined.

Scope Note

Out of scope and unchanged, per the plan: VSCode's mailbox-indicators.ts (the issue is titled
Dashboard); mobile (MobileLayout does not render the badge today); dismiss/show-body from the
UI (Spec 1313 decision 8 and the redaction rule); and codev-skeleton/ (this is product code, not
framework files, so there is no skeleton twin to mirror).

[mohidmakhdoomi]

Architect integration review (claude lane, Medium tier)

Architectural Fit — strong

The server change is the right seam and follows the established pattern exactly. handleWorkspaceRoutes gets an apiPath === 'inbox' branch that passes workspacePath as workspaceOverride, mirroring handleOverview (:2712) and handleAnalytics (:2736) verbatim. No new handler, no second projection, no new dependency. Auth is inherited from the single pre-dispatch choke point (isRequestAllowed, :256), so this adds no unauthenticated surface.

The exact-match on 'inbox' really is load-bearing, and the three non-reachability tests (:455, :464, :473) are the right way to pin it — the body-bearing inbox/:id and mutating inbox/:id/dismiss regexes anchor on ^/api/inbox/, so the workspace prefix can't reach them, and the tests prove the 404 rather than leaving it to inspection.

Client side, HeldCountBadge stays presentational with an injected loader (mirrors CloudStatus), and the rendering matches afx inbox row-for-row — I checked inbox.ts:130-142: ? for a null sender, →Ns countdown for pre-due, scheduled reason, ! on escalation. The Held/Scheduled split is correct and the route test at :412 proves the asymmetry directly (listHeld returns 2, heldSummaryForWorkspace(...).total is 1).

Findings

1. handleInboxList's projection isn't typed as HeldMessage — this repo's convention says it should be. The PR adds HeldMessage to packages/types but the producer at tower-routes.ts:2166 is a bare const projected = rows.map((r) => ({...})). The established pattern is to annotate: const state: DashboardState = {…} (:2798), const result: OverviewData = {…} (overview.ts:1065). As written the new type is client-side decoration only — rename a field on the server and nothing fails until a human notices. One-word fix: const projected: HeldMessage[] = …. Since this PR is what introduces the type, wiring it into the producer is in scope.

2. codev/resources/arch.md — the new paragraph swallowed two unrelated sentences. The Escalation-&-visibility edit appended "Terminal rows (delivered/superseded/dismissed) are pruned after mailbox.retentionDays… Cron delivers through the same gate via deliverCronMessage…" onto the end of the new bolded count-vs-list paragraph. Those sentences are about pruning and cron and now read as part of the asymmetry argument. Move them back to their own paragraph.

3. The workspaceOverride comment overstates a security guarantee. tower-routes.ts:2137-2140 says the override "wins… so even an empty-string override is honored rather than falling through." But :2151 then does rawWorkspace ? normalize(...) : undefined, so an empty-string override yields undefined → lists every workspace, i.e. the widest scope, not the narrowest. It's unreachable today (handleWorkspaceRoutes:2484 rejects a decoded path that doesn't start with / or a drive letter), so no live bug — but either drop the claim or make it true (if (workspaceOverride !== undefined) { workspace = normalize(workspaceOverride) }).

4. loadMessages prop identity is load-bearing and undocumented as such. load is useCallback([loadMessages]) and the fetch effect depends on it. App passes the module-level fetchInbox, so it's stable today. A future caller passing an inline arrow — a mobile layout, a VSCode webview — would refetch on every parent render, i.e. every 2.5s useOverview poll, for as long as the panel is open. Either latch the loader in a ref or add "must be referentially stable" to the prop's doc comment. It's an exported public props interface now.

5. Minor UX: refetch blanks the list. On a count change while open, load() sets {kind:'loading'}, so the rows the user is reading vanish for the round-trip. Showing "Loading…" only on first load (keeping prior rows during refresh) is a few lines and avoids a flicker at exactly the moment the mailbox moved.

6. Doc typo: HeldMessage jsdoc says GET /api/inbox:id — missing slash.

7. Evidence-script accretion. packages/codev/scripts/issue-1450-dashboard-evidence.mts (370 lines) is committed, not in CI, needs out-of-tree playwright-core and a hardcoded port. It doesn't ship (package.json files covers only scripts/forge + postinstall.mjs) and there's precedent (measure-prompt-behavior.ts), so it isn't wrong. But the isolated-Tower dance (NODE_ENV=test + AF_TEST_DB + private port) has now been written three times per the script's own header (send-integration e2e, pir-1365, this) — that's the reusable part, and nothing will tell you when a per-issue copy rots.

---

VERDICT: COMMENT
SUMMARY: Reuses the existing handler behind the correct workspace-scoped seam with real security tests; two cheap fixes (type the server projection, un-splice the arch.md paragraph) and some doc/robustness nits, none merge-blocking.
CONFIDENCE: HIGH

KEY_ISSUES:

• handleInboxList's projection (tower-routes.ts:2166) is not annotated HeldMessage[], breaking the repo's producer-typed-against-shared-type convention (DashboardState at :2798, OverviewData at overview.ts:1065) — the new type is currently client-only, so server/client can drift silently
• arch.md edit spliced the pre-existing pruning/cron sentences onto the end of the new count-vs-list paragraph
• The workspaceOverride comment (:2137-2140) claims an empty-string override is honored; :2151's truthiness check would actually widen scope to all workspaces. Unreachable today, but the comment asserts a guarantee the code doesn't make
• loadMessages must be referentially stable or the open panel refetches every 2.5s; undocumented on a now-public props interface

INTEGRATION_NOTES:

• Auth, redaction, and CLI-only dismissal are genuinely preserved — the tests prove inbox/:id and inbox/:id/dismiss 404 under the workspace prefix and that the row survives a POST, rather than asserting it in prose
• Follow-up candidate: VSCode's mailbox-indicators.ts shows the same N held in the status bar with no path to the list. The server route it needs now exists; the same Held/Scheduled grouping rule would apply
• Follow-up candidate: MobileLayout renders no badge at all, so mobile has neither the count nor the list
• If a second consumer ever needs this rendering, formatHeldAge/isScheduled belong in @cluesmith/codev-sdk (already an apps/web runtime dep, environment-agnostic) rather than a third copy. The CLI↔web duplication is unavoidable under Introduce packages/codev-sdk: client SDK for Tower (server/client dependency isolation) #1189 and the current placement is right for one consumer
• Follow-up candidate: extract the isolated-Tower fixture (AF_TEST_DB + private port + teardown) into a shared helper so evidence scripts stop reimplementing it — that, not the per-issue assertions, is the part worth keeping