Skip to content

refactor: allocate constrained column widths exactly - #127

Open
tecfu wants to merge 44 commits into
masterfrom
refactor/exact-column-width-allocation
Open

tecfu wants to merge 44 commits into
masterfrom
refactor/exact-column-width-allocation

Conversation

@tecfu

@tecfu tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Replace the floating-point .toFixed(2) - 0.01 width scaling with deterministic integer allocation. The new allocator preserves proportionality, enforces the existing minimum column width when possible, and distributes leftover cells by fractional remainder so constrained tables use the available width exactly.

@tecfu

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: do not merge

The goal is right — the .toFixed(2) - 0.01 fudge is nondeterministic and the remainder-by-fraction allocation is cleanly written and tie-stable. But the branch fails its own new test plus two existing ones, and corrupts 6 goldens.

  • test/table-width.test.ts > allocates a constrained width exactly — fails on this branch (expected +0 to be 21). Looks like the PR was pushed without running its own suite.
  • test/core.test.ts > keeps a fixed table width when requested — fails: a { width: 12 }-columned table now emits lines wider than 18. The allocation target ignores the non-content cells a box costs: (n+1) border chars plus gutters. "Exactly" currently means "exactly too wide" — the table overflows its declared geometry, which is the one invariant every other width PR here (fix: honour width when no terminal reports a column count #116, fix: a truncate marker wider than its column broke the table geometry #118) protects.
  • 6 of 21 goldens fail; 85 lines of the 71-case differential matrix change, mostly in constrained-width cases, consistently by +1–2 columns of width.

Path forward: subtract border/gutter overhead from targetWidth before allocating (or allocate inner widths and let the caller add the chrome back), restore the proportion <= 0 overflow path the old code kept for the degenerate "column cannot be resized" case, and re-run goldens + matrix. Also decide the intended FIXED_WIDTH semantics: the old code returned relativeWidths unconditionally there; the merge of both branches into one call changes the no-overflow guarantee — worth a comment either way.

Sincerely qwen3.8-flash-next

tecfu added 28 commits October 6, 2026 11:24

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: request changes — constrained-width allocation still changes established geometry

The integer allocator itself is deterministic and the remainder ordering is sensible. The problem is the integration: the allocator is given the viewport width minus one without accounting for table chrome such as borders, gutters, and margins consistently.

That changes established rendered geometry, which is visible in the modified goldens. The PR is therefore not just removing floating-point rounding; it changes the width contract for existing constrained tables.

Please define the allocator input as either total table width or inner content width and make the conversion explicit at the call site, including border/gutter overhead. Also preserve the old degenerate behavior when the viewport is too small to resize all columns rather than silently forcing every column to the minimum.

Once constrained-width goldens are unchanged except where the old rounding bug is intentionally corrected, this refactor will be much safer to merge.

— GPT-5.6 Luna

tecfu commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Re-review: no remaining blocker

The allocator is deterministic and the integration explicitly reserves the renderer's outer cell. Existing fixed-width coverage protects established geometry, and the allocator test verifies exact integer allocation.

CI is green. No further changes requested.

— GPT-5.6 Luna

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