feat(tui): clarify approval modal (harvest of #3437 by @donglovejava)

[Hmbown]

Harvests the approval-modal clarity improvement from #3437 (by @donglovejava) onto current main.

The approval takeover rendered its four options as a flat list, with selection signalled only by a color change. This change:

• draws a divider between the approve group (once / always-for-session) and the deny/abort group, so they read as two distinct decisions
• adds a leading caret to the highlighted row so the action Enter will fire is unmistakable, not signalled by the background alone

Reapplied in the simplest form: locale-neutral (no new MessageId), reusing the existing approval_selected_style(). The four decisions, footer affordances (v / s / Esc), and the session-vs-persistent approval behavior are all unchanged. cargo test -p codewhale-tui approval passes, including the existing approval_selected_destructive_option_uses_contrasting_highlight regression.

Dropped from the original branch (out of scope): unrelated tool_catalog.rs edits, workflow/SECURITY.md changes, and stray working files.

Credit preserved via Co-authored-by: donglovejava.

🤖 Generated with Claude Code

https://claude.ai/code/session_01991fnUqBbWSgiUFw33L8XX

---

Generated by Claude Code

[claude]

Claude finished @Hmbown's task in 1m 42s —— View job

---

Review: feat(tui): clarify approval modal (harvest of #3437)

• Read diff and file context
• Check rendering pipeline (wrap mode, card sizing)
• Verify glyph/style reuse
• Assess test coverage
• Write review

---

Overall: Clean, minimal harvest. Logic is correct and the two new affordances (group divider + leading caret) use existing patterns consistently. One edge case worth noting; two minor gaps.

---

Findings

1. Divider can wrap on very narrow terminals (cosmetic, minor)

crates/tui/src/tui/widgets/mod.rs:1392-1395

The paragraph is rendered with wrap(Wrap { trim: false }) (line 1473), so every line that exceeds the inner render width wraps rather than clips. The divider is 34 chars (" " + "─".repeat(32)). At APPROVAL_CARD_MIN_WIDTH = 40 the inner width is 40 - 2 (borders) - 2 (padding) = 36, so it fits. But the card is also clamped by .min(area.width), meaning terminals narrower than ~38 columns push the card below min-width and the divider overflows — a stray lone ─ wraps onto the next line. The smallest test fixture is 39 wide, which just barely avoids this.

Not a crash, but the comment "sized to fit the minimum card inner width" is only true for the declared constant, not for pathological terminals. Consider 28 dashes (fits the 36-char inner at min-width with headroom) or simply accepting the cosmetic wrapping as out-of-range. Fix this →

2. Hardcoded index 2 is structurally coupled to option ordering

crates/tui/src/tui/widgets/mod.rs:1390

if i == 2 {

This silently relies on approval_options_for returning approve options at indices 0–1 and deny/abort at 2–3. The [ApprovalOptionRow; 4] return type enforces count at compile time but not ordering. If the options are ever reordered the divider lands in the wrong position with no warning. The existing comment documents the intent clearly, so this is low risk as-is — just worth keeping in mind when touching approval_options_for.

3. No new tests for divider or caret

The existing approval_selected_destructive_option_uses_contrasting_highlight test checks that selected cells use SELECTION_BG, which implicitly exercises the caret path, but doesn't assert on the ▸ glyph or the ─ divider character. Two small assertions (scan buffer_text for ▸ on the selected row, and for ─ between option rows) would pin the new behaviour against regressions without much test code.

---

What's good

• \u{25b8} (▸) matches the glyph used by all other selection markers in this file (lines 894, 944, 1011) — visually coherent.
• Caret styled with approval_selected_style() rather than a one-off Style — avoids duplication.
• Locale-neutral, no new MessageId; appropriate harvest scope.
• No unnecessary clones or allocations added.

[claude]

Claude finished @Hmbown's task in 1m 33s —— View job

---

Review: feat(tui): clarify approval modal (harvest of #3437)

• Read diff and file context
• Analyze divider/caret rendering and edge cases
• Check glyph reuse and style consistency
• Assess test coverage
• Write final review

---

Overall: Clean, minimal harvest. The prior review's two actionable findings have both been addressed: dashes reduced from 32 → 28 (narrowing the wrapping threshold), and a new render test now pins both the ▸ caret and ─ divider glyphs. One low-risk structural note remains.

---

Findings

1. Hardcoded i == 2 is silently coupled to option ordering (low risk)

crates/tui/src/tui/widgets/mod.rs:1391

if i == 2 {

The divider placement relies on approval_options_for returning approve options at 0–1 and deny/abort at 2–3. The [ApprovalOptionRow; 4] return type enforces count at compile time but not ordering. A future reorder of that array would silently misplace the divider with no compiler warning. The existing comment documents the intent clearly, which mitigates the risk — just worth keeping in mind when touching approval_options_for.

No change required; noting it for the next person who edits approval_options_for.

---

What's good

• Divider width corrected from prior review: "─".repeat(28) → 30 chars total (including 2-space indent), fits in the 36-char inner width at APPROVAL_CARD_MIN_WIDTH = 40 (40 − 2 borders − 2 padding per side = 36). Wrapping threshold dropped to ~32-col terminals, well below any practical TUI minimum.
• New test covers both glyphs: approval_card_shows_group_divider_and_selected_caret checks \u{25b8} on the selected row and \u{2500} for the divider — pins regression without excess boilerplate.
• \u{25b8} (▸) matches every other selection caret in this file (lines ~894, 944, 1011) — visually coherent.
• Caret styled with approval_selected_style() rather than a one-off Style — no duplication. Non-selected lead uses Span::raw(" ") — same 2-char width, layout stays stable.
• Locale-neutral: no new MessageId, no translation burden.
• No unnecessary allocations or clones.

---

Verdict: LGTM. The two actionable findings from the prior round are resolved; the remaining structural note is purely cosmetic and well-documented.