Skip to content

feat(tui): widen nav sidebar so labels do not wrap#1755

Merged
pedrofuentes merged 2 commits into
mainfrom
feature/tui-nav-width
Jun 26, 2026
Merged

feat(tui): widen nav sidebar so labels do not wrap#1755
pedrofuentes merged 2 commits into
mainfrom
feature/tui-nav-width

Conversation

@pedrofuentes

Copy link
Copy Markdown
Owner

Summary

NAV_WIDTH.expanded 14→24 + navWidth in LayoutPlan; AppShell pins the nav box width + flexShrink:0 so 'Conversations'/'Debates' render on one line; verified no overflow at 120/140 cols; Phase 11 polish — feedback #7

Changes

  • src/tui/lib/breakpoints.ts: NAV_WIDTH.expanded bumped from 14 → 24 (fits ● 💬 Conversations 18 cols + paddingX 2 + border 2 = 22 minimum; 24 adds breathing room). New navWidth: number field added to LayoutPlan interface; computeLayout exposes it as NAV_WIDTH[navState].
  • src/tui/components/layout/AppShell.tsx: nav <Box> now receives width={props.layout.navWidth} and flexShrink={0} so the nav pane is fixed-width and cannot be squeezed by the main pane's flexGrow={1}.

Tests

  • breakpoints.test.ts: new assertions for navWidth (expanded=24, rail=3, hidden=0) and updated mainWidth === columns - navWidth
  • app-shell.test.tsx: new test renders AppShell with ● 💬 Conversations in the nav and asserts the label is on one line (emoji and text not split), frame ≤ rows lines

Full TUI suite: 901 tests pass. Lint zero. Typecheck clean.

pedrofuentes and others added 2 commits June 26, 2026 10:20
- assert navWidth property exists on LayoutPlan (expanded=24, rail=3, hidden=0)
- assert mainWidth === columns - navWidth (updated from hardcoded 140-14 → 140-24)
- assert AppShell pins nav so '● 💬 Conversations' renders on one line

Co-authored-by: Copilot <175574315+pedrofuentes@users.noreply.github.com>
- NAV_WIDTH.expanded 14 → 24: fits '● 💬 Conversations' (18 cols content)
  plus LeftNav paddingX=1 (2 cols) plus round border (2 cols) = 22 minimum;
  24 adds two cols breathing room
- Add navWidth to LayoutPlan interface; computeLayout sets it to NAV_WIDTH[navState]
- AppShell nav <Box> now receives width={layout.navWidth} and flexShrink={0}
  so the pane is fixed-width and never squeezed by the main flexGrow=1 pane
- mainWidth stays consistent: columns - navWidth

Co-authored-by: Copilot <175574315+pedrofuentes@users.noreply.github.com>
@pedrofuentes

Copy link
Copy Markdown
Owner Author

Status: CONDITIONAL

Sentinel Review Report

Ref: feature/tui-nav-width → main
Report ID: SENTINEL-1755-a48224a
Reviewed SHA: a48224a
Sentinel ruleset: v1
Reviewed at: 2026-06-26T10:37:34-07:00
Mode: standard
Review depth: Tier 2 (full)
Required action: FILE_ISSUES_AND_MERGE

Phase 1 — TDD / Test Evidence

  • Tests exist & meaningful: ✅ tests/unit/tui/breakpoints.test.ts:66-75 asserts navWidth for expanded/rail/hidden and mainWidth; tests/unit/tui/app-shell.test.tsx:190-212 renders AppShell and asserts the Conversations label remains with the emoji on one line.
  • Test-first history verified: ✅ git log --oneline origin/main..origin/feature/tui-nav-width4db25c0 test(tui): add failing tests for wider nav sidebar precedes a48224a feat(tui): widen nav sidebar so labels do not wrap; git show --name-status 4db25c0 touched only test files, and git show --name-status a48224a touched only source files.
  • RED verified: ✅ at 4db25c0, npx --yes pnpm@10.30.1 exec vitest run tests/unit/tui/breakpoints.test.ts tests/unit/tui/app-shell.test.tsx --reporter=dot failed as expected: Test Files 2 failed (2) / Tests 3 failed | 16 passed (19), including expected undefined to be 24 at app-shell.test.tsx:193 and breakpoints.test.ts:68.
  • Full suite green on SHA: ✅ at a48224a, npx --yes pnpm@10.30.1 test -- --reporter=dotTest Files 358 passed | 1 skipped (359) / Tests 4519 passed | 1 skipped (4520).
  • Required targeted verification: ✅ npx --yes pnpm@10.30.1 exec vitest run tests/unit/tuiTest Files 95 passed (95) / Tests 901 passed (901); npx --yes pnpm@10.30.1 lint exit 0; npx --yes pnpm@10.30.1 typecheck exit 0.
  • Coverage: N/A (no coverage gate/output required for this PR).
  • Branch base: ✅ git merge-base origin/main origin/feature/tui-nav-width = 4cf1b8c610c6f3f4e83d0cf692624568c4317add (current origin/main).

Phase 1.5 — Fast-path Evaluation

🔴 count: 0 | LOC: 18 non-test/non-lockfile changed (≤150: Y) | Security paths: Y (terminal UI rendering surface) | New deps: N | Commit types qualify: N (feat)
→ Fast-path eligible: NO → Phase 2

Phase 2 — Execution Log

Dim Tool Call Agent ID / Ref Status
A1 task(agent_type="general-purpose", name="sentinel-dim-a1") N/A (Copilot task returned inline result; no stable ID exposed) ✅ No findings
A2 task(agent_type="general-purpose", name="sentinel-dim-a2") N/A (inline result) ✅ No findings
B task(agent_type="general-purpose", name="sentinel-dim-b") N/A (inline result) ✅ No findings
C task(agent_type="general-purpose", name="sentinel-dim-c") N/A (inline result) ✅ No findings
D task(agent_type="general-purpose", name="sentinel-dim-d") N/A (inline result) ✅ One test-quality finding
E N/A N/A ✅ N/A (no dependency/supply-chain surface changed)
F task(agent_type="general-purpose", name="sentinel-dim-f") N/A (inline result) ✅ One minor documentation/comment finding
Persist gh pr comment 1755 --body-file - PR #1755 comment ✅ Full report posted

Additional checks: git diff --check origin/main...origin/feature/tui-nav-width exit 0. Scope confirmed limited to breakpoints.ts, AppShell.tsx, and their tests.

Findings

  • 🔴 CRITICAL: 0
  • 🟡 IMPORTANT: 1 new / 0 known
  • 🟢 MINOR: 1

Details (ordered by severity)

  1. 🟡 IMPORTANT — AppShell regression test can pass without the fixed-width nav Box — packages/cli/tests/unit/tui/app-shell.test.tsx:210-212

    • Evidence: const conversationsLine = lines.find((l) => l.includes("Conversations")); / expect(conversationsLine).toBeDefined(); / expect(conversationsLine).toContain("💬");
    • Why: The source change at packages/cli/src/tui/components/layout/AppShell.tsx:86-87 is width={props.layout.navWidth} and flexShrink={0}, but the test only checks the synthetic label remains on one line; Ink can still size the nav to this short content even if width={layout.navWidth} is removed.
    • Impact: Trigger: a future regression removes or stops honoring fixed nav width. Mechanism: the current test still passes because it does not assert the nav pane width or create enough layout pressure to require flexShrink={0}. Consequence: the original wrapping UX bug can recur while CI remains green.
    • Remediation: Add an assertion that observes the nav pane occupying layout.navWidth columns (or render under pressure where removing width/flexShrink={0} reproduces wrapping) so the test fails when AppShell stops applying the fixed nav width.
  2. 🟢 MINOR — RED-phase comment is stale on the final green commit — packages/cli/tests/unit/tui/app-shell.test.tsx:192

    • Evidence: // RED: navWidth does not yet exist on LayoutPlan
    • Why: On final HEAD, navWidth does exist, so the comment describes historical TDD state rather than current behavior.
    • Impact: Minor maintainer confusion when reading the final test.
    • Remediation: Remove the comment or replace it with a behavior-focused rationale.

Follow-ups & Actions

  • CONDITIONAL → FILE_ISSUES_AND_MERGE: file GitHub issues for the new 🟡 and 🟢 findings (sentinel:important, sentinel:minor), link them in PR feat(tui): widen nav sidebar so labels do not wrap #1755, then merge if policy permits.
  • ⚠️ Do NOT fix 🟡/🟢 findings in this PR — file as issues only.
  • Only 🟡/🟢 findings remain; there are no 🔴 blockers.

Decision rationale

  • Verdict: CONDITIONAL for reviewed SHA a48224a69bc3595b58da49f24940a3de4cbfa216.
  • TDD ordering and commit purity are compliant: test-only commit first, source-only implementation commit second.
  • RED/GREEN evidence exists, targeted TUI suite/lint/typecheck pass, and the full package test passed on final SHA.
  • Width math is internally consistent: expanded=24, rail=3, hidden=0; mainWidth = max(0, columns - navWidth) prevents negative widths at small/override sizes and preserves tooNarrow behavior.
  • The remaining issue is a non-blocking test-quality gap; per Sentinel rules, new 🟡 findings produce CONDITIONAL rather than REJECTED.

@pedrofuentes

Copy link
Copy Markdown
Owner Author

Sentinel SENTINEL-1755-a48224a → CONDITIONAL (0 🔴). Follow-ups filed: #1759 (🟡 test coverage), #1760 (🟢 stale comment). Merging per CONDITIONAL policy.

@pedrofuentes pedrofuentes merged commit baa24aa into main Jun 26, 2026
15 checks passed
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