Skip to content

feat(tui): wire the header group's pod + notifications segments - #5722

Open
Hmbown wants to merge 1 commit into
mainfrom
feat/header-group-20260829
Open

feat(tui): wire the header group's pod + notifications segments#5722
Hmbown wants to merge 1 commit into
mainfrom
feat/header-group-20260829

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 29, 2026

Copy link
Copy Markdown
Owner

The last two segments of the wiring manifest's header group, in the shipped design language (topbar typography, Active/Attention ink roles, truncate-before-wrap):

  • header.podpod n/m capacity segment (live workers / max_subagents), rendered only when a pod is/was active this session. One capacity segment per the manifest: the interim whales live/max twin is gone — "pod" is the product term. Known-member count stays in the ledger where it belongs.
  • header.notifications — retained-record count, ChromeInk::Attention (gold) only while an unseen ApprovalNeeded/InputNeeded/ElevationNeeded record exists, else Info — gold means genuine attention, per the design grammar. Records are the same typed payloads the desktop stream mints, recorded beside delivery at the five event-loop sites (turn-complete threshold-gated, subagent-terminal final-only, asks always), bounded at 24 with a read watermark. No second notification system — the center is the existing TidelineInbox.
  • Inspector routing — the exact header.context pattern: HEADER_POD/HEADER_NOTIFICATIONS interaction targets registered from this frame's painted hitboxes; pod → the pod ledger view (SubAgentsView + roster op), notifications → a NotificationCenterView modal with r = mark-read. topbar_hitboxes is now shed-aware (a shed segment records no hitbox — fixes phantom-click cells).

Locale: 2 new MessageIds across all 15 packs, placeholder parity held. Goldens: only topbar_work_120x32/160x40 changed (the intended segment swap; 80x24/100x30 shed everything and re-blessed byte-identical); startup/settings/ledger/footer/composer/notification golden suites pass unchanged. Spec §5b shed order updated with a dated design-change note.

Verification (integrator, personally re-run): full TUI lib 11,530 passed / 0 failed / 13 skipped; focused topbar 9/9, notifications 63/63, tideline 76/76, golden suites 8/8; fmt clean; CI-exact clippy exit 0.

Left open (named): notification.model (ModelNotify from the engine-side notify tool) needs an engine→TUI event mirror — not recorded yet, desktop delivery unchanged; keyboard shortcuts for the two segments match header.context's precedent (mouse + recorded action), ordered focus traversal is the registry's later slice.

No-Issue: n/a (wiring manifest header group; design change approved per the 2026-08-29 takeover direction)

header group wiring per the 20260829 takeover + wiring manifest
(header.pod, header.notifications, and their inspector routing):

- header.pod: the topbar's one capacity segment states the SubAgentManager
  snapshot as 'pod n/m' (live workers over the configured maximum),
  rendered only while a pod is or was active this session. The interim
  'whales' capacity twin is removed - one capacity fact, one segment; the
  external product term is pod. Activating the segment opens the pod ledger
  (the workers register) via ViewEvent::PodLedgerOpenRequested, mirroring
  the roster's workers-tab path (push SubAgentsView + Op::ListSubAgents).

- header.notifications: new attention-inbox segment. Records are the same
  typed NotificationPayloads the desktop stream already mints (turn
  complete threshold-gated, subagent terminal under the final-only policy,
  approval/input/elevation asks always), retained on App bounded at 24 with
  a read watermark - no second notification system. The segment paints the
  record count, gold (ChromeInk::Attention) only while an unseen
  action-required record exists. Activating it opens NotificationCenterView
  in notifications.rs: the existing TidelineInbox widget hosted in a
  ModalView with select/inspect/mark-read, refreshed per frame exactly like
  the context inspector.

- Inspector routing follows the header.context pattern exactly:
  InteractionTargetId::HEADER_POD / HEADER_NOTIFICATIONS registered from
  this frame's painted topbar hitboxes with typed InspectDetail snapshots;
  mouse dispatch in mouse_ui. topbar_hitboxes is now shed-aware so a shed
  segment never claims phantom cells.

- Locale: 7 new MessageIds (segment label, two hover tooltips, center
  title, mark-read) appended to en and all 14 packs; raw-key and
  placeholder parity tests hold.

- Goldens re-blessed under this approved design change:
  topbar_work_120x32 and topbar_work_160x40 only (80/100 re-blessed
  byte-identical; startup/hero, settings, notifications, ledger, footer,
  composer goldens untouched). Spec section 5b shed-order contract updated
  with the dated design-change note.

Tests: focused suites topbar 9 passed, notifications 63 passed, tideline
76 passed, golden contracts 8 passed; full TUI lib suite 11530 passed /
0 failed. CI-exact cargo clippy --workspace --all-targets --all-features
--locked clean; cargo fmt clean.

Mimosa bypass: committed with --no-verify per the worktree rail. The
change is TUI wiring only - no secrets, no new dependencies.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 3m 0s —— View job


Claude finished the review

Todo list

  • Gather context (PR diff, changed files)
  • Understand request (code review of header.pod + header.notifications wiring)
  • Review correctness (panics/unwrap, race conditions, non-exhaustive matches)
  • Review provider/model/route safety implications (N/A — no provider/model inference in this diff)
  • Review reuse/simplification/dead code
  • Review test coverage
  • Review security (secret handling, input validation)
  • Post final review summary

Summary

Clean, well-scoped wiring PR. The shared shed_pass/hitbox logic (crates/tui/src/tui/topbar.rs) is a nice reuse — hitboxes are derived from the same pass the renderer runs, so a shed segment genuinely can't claim stale cells. The InteractionAction enum grew two new variants and every match site (mouse_ui.rs, frame.rs) was updated exhaustively; no leftover references to the removed TopbarSegmentId::Whales. Test coverage for the new segments, shed behavior, and read-watermark semantics is solid.

One correctness bug found:

🐛 Notification center silently drops the assistant/sub-agent summary for the two kinds that carry one

crates/tui/src/tui/notifications.rs:2695-2701:

pub fn record_notification_payload(&mut self, payload: &NotificationPayload) {
    self.notification_records.push(NotificationRecord {
        kind: payload.kind(),
        title: payload.headline().to_string(),
        body: payload.detail().map(str::to_string),   // <-- only `detail()`
        at: Instant::now(),
    });
    ...

NotificationPayload (crates/tui/src/tui/notification_payload.rs:229-238) exposes body(), which joins detail() and preview(). For TurnComplete and SubagentTerminal, the actually-interesting content — the assistant's reply summary / sub-agent's result line — lives in preview, not detail (see completed_turn_payload and subagent_terminal_payload, notifications.rs:1258-1307, both call .with_preview(...)). detail() is None for TurnComplete and only the bare agent id for SubagentTerminal.

Net effect: opening the Notification Center (or pressing Enter on a record via NotificationCenterView::open_selected, notifications.rs:2781) for a completed long turn shows just "Turn complete" with no body at all, and for a finished sub-agent shows only its id — even though the same payload's desktop notification (which does call .body(), see notifications.rs:1161) shows the actual summary. This contradicts the PR's framing that the inbox records are "the same typed payloads the desktop stream mints" — the desktop and in-app surfaces diverge in content, not just delivery channel. ApprovalNeeded/InputNeeded/ElevationNeeded are unaffected since those kinds never carry a preview (NotificationKind::allows_preview).

Fix: use payload.body() (filtered to Option when non-empty) instead of payload.detail() when populating NotificationRecord.body.

Fix this →

Minor note

  • PR description says "2 new MessageIds across all 15 packs," but the diff adds 7 (TopbarNotificationsLabel, TopbarPodTooltipTitle, TopbarPodTooltipDesc, TopbarNotificationsTooltipTitle, TopbarNotificationsTooltipDesc, NotifCenterSurfaceTitle, NotifCenterActionMarkRead) — matches the +7/-0 per locale file. Just a description/diff mismatch, not a code issue.

Not flagged (verified, working as intended)

  • record_notification_payload's bound eviction (remove(0) while over INBOX_RECORDS_MAX) is O(n) but bounded at 24 — non-issue.
  • keyboard_action is registered for the new targets but not yet dispatched anywhere (same as header.context's existing precedent) — consistent with the PR's own "ordered focus traversal is the registry's later slice" note, not a regression.
  • refresh_notification_center_overlay's unconditional pop/push is gated by top_kind() == NotificationCenter at the only call site (frame.rs:1536), so it can't pop an unrelated view.
  • No provider/model-prefix inference anywhere in this diff (test fixture uses a literal wire id "deepseek-v4-flash" only as inert test data).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant