Skip to content

feat(kanban): trim chrome to maximize horizontal space at narrow widths - #241

Merged
agent-era-ai merged 5 commits into
mainfrom
maximize-kanban-horizontal-spa
May 25, 2026
Merged

feat(kanban): trim chrome to maximize horizontal space at narrow widths#241
agent-era-ai merged 5 commits into
mainfrom
maximize-kanban-horizontal-spa

Conversation

@agent-era-ai

Copy link
Copy Markdown
Collaborator

Summary

  • Strip kanban column-level and screen-level chrome so narrow terminals stop wasting cols on borders/padding/margins (and stop overflowing): drop column paddingX/marginRight, drop the outer root paddingX, and remove the inter-group separator entirely. Plan vs implementation grouping is now signaled by the existing column accent colors only.
  • Tighten card secondary/approve-hint indent and chip marginLeft from 4 → 2 cols, and lower MIN_COLUMN_WIDTH from 20 → 12 so the default board fits cleanly inside 80-col terminals.
  • Side effect: dropping the inter-group separator's flexGrow={1} also closes the wide-terminal mid-board void.

Test plan

  • npm run typecheck — clean
  • npm test — 815 pass / 78 suites
  • npm run test:terminal — 294 pass / 1 skipped, including a new per-size width-fit assertion at every SIZE_COMBOS entry (10×80 through 40×160)
  • Visual sanity-check at 100×20: 4 columns abut, accent colors distinguish plan/impl, slugs render at the full new width
  • (reviewer) Spot-check at ≥ 200-col terminal — board now ends at the column-cap width with no center void

🤖 Generated with Claude Code

agent-era-ai and others added 3 commits May 25, 2026 02:38
Strip column-level and screen-level chrome down to the minimum needed for
visual grouping so narrow terminals (~80 cols) stop wasting space — and
overflowing — on borders/padding/margins.

Changes:
- Drop column paddingX, marginRight, and the outer root paddingX.
- Remove the inter-group separator entirely; plan vs implementation grouping
  is now signaled by the column accent colors (blue / magenta) alone. Side
  effect: dropping the separator's flexGrow={1} also stops the wide-terminal
  mid-board void.
- Tighten the card secondary-text / approve-hint indent and chip marginLeft
  from 4 to 2.
- Lower MIN_COLUMN_WIDTH from 20 to 12 so narrow terminals shrink instead of
  overflowing the row.
- Recompute the slug-truncation, secondary-text-wrap, pendingNew, and title-
  truncation width budgets against the new chrome (named via
  COLUMN_BORDER_COLS / SLUG_ROW_GUTTER / SECONDARY_ROW_GUTTER).
- Keep paddingX={1} on the title bar and footer only (no border to ground them
  against the terminal edge).

Test:
- Add a per-size width-fit assertion to tracker-board-size.test.mjs so
  rendered rows never exceed termCols at any SIZE_COMBOS entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rims

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — feat(kanban): trim chrome to maximize horizontal space at narrow widths

Overview

This PR removes fixed chrome (column padding, margins, and the inter-group separator) from the kanban board, replaces magic numbers with named constants, tightens card-level gutters from 4→2 spaces, lowers MIN_COLUMN_WIDTH from 20→12, and adds a width-fit regression test. The intent is to stop narrow terminals (80–120 cols) from overflowing with the default 5-column layout.


✅ Strengths

  • Named constants replace magic numbers. COLUMN_BORDER_COLS, SLUG_ROW_GUTTER, and SECONDARY_ROW_GUTTER give every truncation calculation a self-documenting budget. This is a clear maintainability win over the old bare 8 and 4 literals.
  • Width formula simplification is correct. Math.floor(termCols / numColumns) is the right formula once all external chrome is gone. The previous formula Math.floor((termCols - 4 - numColumns) / numColumns) was faithfully modeling chrome that no longer exists.
  • groupWidthFor updated consistently. Old: count * (colWidth + 1), new: count * colWidth — correctly drops the removed marginRight.
  • Partial padding preservation is sensible. Keeping paddingX={1} on the title bar and footer (rather than stripping the root padding and leaving those rows flush against col 0) preserves visual grounding for unbordered rows while still giving the board itself edge-to-edge width.
  • Regression test is a good addition. The width-fit assertion across all SIZE_COMBOS closes a real gap in the existing test suite.
  • All truncation math cross-checks out. Verified independently: slug (colWidth - 2 - 4 = colWidth - 6), secondary (colWidth - 2 - 2 = colWidth - 4), column title, and pendingNew rows all match the new chrome model.

⚠️ Issues / Suggestions

1. Width-fit test uses String.length, which undercounts display width for wide Unicode characters.

const widest = lines.reduce(
  (acc, line, idx) => (line.length > acc.length ? ... : acc),
  ...
);
assert.ok(widest.length <= width, ...);

String.prototype.length counts UTF-16 code units, not terminal display columns. Braille spinner chars (⠋⠙…) and box-drawing chars (╮╭) are all BMP single-width, so this test is accurate for the current rendering. But if ANSI escape sequences are present in the frame output (color codes have string length but zero visual width), the test could yield false negatives — it would see a line as wider than it visually is and flag it incorrectly, or ANSI could make some lines appear within the limit when they are not.

Recommendation: Verify whether frame() in the test harness strips ANSI escape codes before asserting. If it does not, add a strip step (or use a display-width utility like string-width) to make the assertion accurate.


2. SLUG_ROW_GUTTER excludes the border — consider whether the constant split is the right abstraction.

The comment on SLUG_ROW_GUTTER = 4 explains it covers cursor + glyph only, with COLUMN_BORDER_COLS handling the border separately. This is correct. But callers always use them together (colWidth - COLUMN_BORDER_COLS - SLUG_ROW_GUTTER), which means you have to remember they're complementary. An alternative is two totals (SLUG_ROW_OVERHEAD = 6, SECONDARY_ROW_OVERHEAD = 4), though the current split does allow independently reasoning about border vs. gutter costs. Minor call — just make sure future changes update both constants when gutter changes.


3. pendingNew sub-row uses SECONDARY_ROW_GUTTER but the prefix is ' ' (2 spaces) not a 2-space indent after cursor/glyph.

The pendingNew title row renders ' ' (accent, 2 chars) + spinner + space + title. That's a 4-char leading prefix inside the border — consistent with SLUG_ROW_GUTTER. The sub-row is ' ${truncated}' (2 spaces), consistent with SECONDARY_ROW_GUTTER. The math is correct. This is just a note that the pendingNew rendering follows the same gutter model as regular cards, so the shared constants are appropriate.


4. No guard on the visual gap at MAX_COLUMN_WIDTH for very wide terminals.

The PR correctly notes in the tracker implementation doc that at ≥200 cols, cumulative column width hits the cap (5 × 50 = 250, caps at 50) and the board will end before the terminal's right edge. This is acceptable per the stated scope. Consider adding a brief comment in groupWidthFor or the colWidth calc to surface this trade-off in the code for whoever next touches this path.


Minor Style Notes

  • The tracker files (notes.md, requirements.md, implementation.md) are verbose but seem intentional for a self-dogfooding tracker tool. No concern there.
  • Comments in the diff are well-scoped — they explain WHY (the layout accounting model), not WHAT. Consistent with AGENTS.md guidance.
  • CHANGELOG entry is clear and accurate.

Summary

The core logic is correct and the PR delivers what it promises. The primary actionable item is confirming the ANSI-stripping behavior in the test harness so the new width-fit regression is reliable. Everything else is minor. The constant extraction and simplified width formula are genuine improvements to maintainability.

Surface the wide-terminal slack inline so future readers don't mistake the
column cap for a layout bug. Addresses PR #241 review note.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — feat(kanban): trim chrome to maximize horizontal space at narrow widths

Overview

This PR strips fixed chrome from the kanban board — per-column paddingX/marginRight, the outer root paddingX, and the inter-group separator — and tightens the card-internal gutter from 4 to 2 cols. The column-width formula simplifies to floor(termCols / numColumns), MIN_COLUMN_WIDTH drops from 20 → 12, and a width-fit regression is added to the terminal test suite. The result is a 5-column board that fits cleanly inside an 80-col terminal.


Code Quality and Style ✅

Named constants are a clear improvement. Replacing the inlined 8 and 4 literals with COLUMN_BORDER_COLS, SLUG_ROW_GUTTER, and SECONDARY_ROW_GUTTER makes the truncation arithmetic readable and keeps the six call sites consistent. If chrome geometry ever changes again, there's one place to update.

The width formula simplification is correct and cleaner. Moving from (termCols - 4 - numColumns) / numColumns to termCols / numColumns follows directly from making all chrome internal to the border-box width={colWidth}. The accompanying comment explains the invariant clearly.

groupWidthFor updated correctly. count * (colWidth + 1)count * colWidth is the right adjustment after dropping marginRight={1}.

Minor style note: The project convention (AGENTS.md) says to prefer a single-line comment when one is genuinely warranted. The two 3-line blocks near the new constants (MAX_COLUMN_WIDTH trade-off note and COLUMN_BORDER_COLS explanation) contain non-obvious constraints and are borderline justified, but could each compress to one line — e.g.:

// Wide terminals: columns cap at MAX_COLUMN_WIDTH, so the row may not reach the terminal edge — accepted trade-off.
const MAX_COLUMN_WIDTH = 50;
// colWidth is border-box (1 left + 1 right border, no paddingX); card lines add SLUG_ROW_GUTTER or SECONDARY_ROW_GUTTER inside.
const COLUMN_BORDER_COLS = 2;

Potential Bugs / Issues

MIN_COLUMN_WIDTH = 12 can still overflow when column count is high relative to terminal width.
colWidth = Math.max(12, Math.floor(termCols / numColumns)). If numColumns × 12 > termCols, the clamped minimum forces overflow. Example: 9 columns at 80 cols → colWidth = max(12, 8) = 12, total = 108 > 80. In practice the board appears fixed at 5 columns, so the default case is safe, but if column count can grow this is a latent overflow path. The new width-fit test will catch any regression if more SIZE_COMBOS column counts are exercised — worth noting for future column-count changes.

Title bar / footer offset vs. board columns. The title bar and footer retain paddingX={1}, so their text starts at col 1 while the column borders start at col 0. On very narrow terminals this means the title bar text is inset by one col relative to the board's left edge. This is intentional (documented in the implementation notes) and visually acceptable, but reviewers should be aware the left edges no longer align.


Performance ✅

No performance implications — this is purely layout/rendering arithmetic. The removal of the flexGrow={1} separator is a minor win since Ink no longer needs to distribute remaining flex space.


Security ✅

No security concerns — pure UI layout changes with no external I/O.


Test Coverage ✅

The new no rendered row exceeds terminal width assertion at every SIZE_COMBOS entry is exactly the right regression to add and closes the gap the discovery notes called out. The existing vertical-alignment assertion continuing to pass after the separator removal is reassuring.

One suggestion: the PR description flags a manual reviewer action item — "Spot-check at ≥ 200-col terminal — board now ends at the column-cap width with no center void." It would be worth confirming the 40 × 160 SIZE_COMBOS entry (the widest tested) shows no large gap and that the implementation doc's "columns end before the right edge" note is acceptable at production-realistic widths.


Summary

Solid, well-scoped change. The math is correct, the named constants are a genuine improvement over magic numbers, and the new regression test directly covers the fix. The two minor points above (multi-line comments, MIN_COLUMN_WIDTH overflow at high column counts) are low priority — the first is a style nit, the second is guarded by the new test for the tested configurations. Happy to approve pending the wide-terminal visual spot-check.

Match the one-line-when-warranted style from AGENTS.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — feat(kanban): trim chrome to maximize horizontal space at narrow widths

Overview

This PR removes per-column padding/margin, the inter-group separator, and the outer root padding from the kanban board, then re-derives the width budget math from named constants. The result is a tighter layout that fits the default 5-column board in an 80-col terminal without overflow. A new terminal E2E assertion guards against regressions.


Code Quality ✅

  • Named constants are a clear improvement. Replacing the bare 8 / 4 magic numbers with COLUMN_BORDER_COLS, SLUG_ROW_GUTTER, and SECONDARY_ROW_GUTTER makes the arithmetic self-documenting and keeps truncation calculations in sync if the layout changes again.
  • Width formula simplification is correct. Math.floor(termCols / numColumns) is the right result once all per-column and screen-level chrome is moved inside the border-box width={colWidth}.
  • Keeping paddingX={1} on the title bar and footer only is the right call — those rows have no border to ground them against col 0. The comment in the implementation notes explains this well.
  • Removing flexGrow={1} from the inter-group separator also closes the wide-terminal mid-board void as a side-effect. Good catch, and the reviewer spot-check item in the test plan is appropriate.
  • groupWidthFor correctly changes from count * (colWidth + 1) to count * colWidth to match the removed marginRight.

Potential Issue — Test width measurement with ANSI codes ⚠️

The new assertion in tracker-board-size.test.mjs measures row width with:

const widest = lines.reduce(
  (acc, line, idx) => (line.length > acc.length ? {length: line.length, idx, line} : acc),
  {length: 0, idx: -1, line: ''}
);
assert.ok(widest.length <= width, ...);

If the rendered frame contains ANSI escape sequences (color codes, bold, etc.), line.length counts the invisible escape bytes, making the measured width larger than the visible column count. The test could fail spuriously on colored output, or — worse — pass when the actual visible content overflows.

Worth checking whether renderBoard strips ANSI before this assertion is trusted as a true width guard. If it does not, wrapping the line with a stripAnsi() call before measuring would make the assertion accurate.


Minor: Comment on MIN_COLUMN_WIDTH ℹ️

const MIN_COLUMN_WIDTH = 12;
// At wide terminals the row may end before the right edge because columns cap here — accepted trade-off.

The comment belongs on MAX_COLUMN_WIDTH (the cap), not MIN_COLUMN_WIDTH (the floor). Worth moving it one line down for clarity.


Test Coverage ✅

  • New width-fit assertion at every SIZE_COMBOS entry is the right regression net.
  • 294 terminal tests pass (+ 1 skipped), 815 unit/E2E pass, typecheck clean.
  • The existing vertical-alignment test continues to hold — good validation that dropping the separator didn't break header-row alignment.

Summary

The implementation is correct, well-reasoned, and the named constants are a solid improvement over the previous magic numbers. Two items worth addressing before merge:

  1. Verify (or fix) ANSI handling in the new width assertion — if ANSI escape codes are present, the measurement is off.
  2. Move the wide-terminal comment from MIN_COLUMN_WIDTH to MAX_COLUMN_WIDTH.

Everything else looks good to merge.

@agent-era-ai
agent-era-ai merged commit cccfe39 into main May 25, 2026
2 checks passed
@agent-era-ai
agent-era-ai deleted the maximize-kanban-horizontal-spa branch May 25, 2026 02:56
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