Repository navigation
Conversation
@loongphy/codex-auth-darwin-arm64
@loongphy/codex-auth-darwin-x64
@loongphy/codex-auth-linux-arm64
@loongphy/codex-auth-linux-x64
@loongphy/codex-auth-win32-arm64
@loongphy/codex-auth-win32-x64
@loongphy/codex-auth
commit: |
Loongphy
left a comment
There was a problem hiding this comment.
Thanks for the PR — the problem is real, and your direction (compact headers, keeping 100% intact) is right. I verified the patch end-to-end with a pty harness across terminal widths 30–130 (synthetic account) and the full test suite passes on zig 0.16.
However, I'd prefer not to extend the legacy min-clamp layout in src/tui/table.zig, because it cannot handle the extremes well, no matter which minimums we pick:
- Hard floor = sum of minimums. This patch raises the floor to 47 columns, so on 40–46 column terminals the table now hard-wraps where it used to fit. Any min-clamp design has this property; there is no in-design fix.
- Variable-length values still collapse. Error-name overrides render as
Fil.(4-char floor) andunlimitedasu.— a floor only guarantees fixed-width values like100%. - Untestable in CI.
adjustListWidths()callsterminalWidth(), which reads the real stdout TTY inside the renderer, so the narrow layout can never be exercised by unit tests.
The interactive --live table already solves all three with budget allocation in src/cli/table_layout.zig::boundWidths:
// Instead of per-column minimums, distribute the available width by priority,
// then grow columns in a second pass:
growBoundedWidth(&remaining, &bounded.email, ...); // 1. account identity
growBoundedWidth(&remaining, &bounded.rate_5h,
@min(widths.rate_5h, @max(4, "5H".len))); // 2. 5H target = 4 → "100%" always fits
growBoundedWidth(&remaining, &bounded.rate_week, ...); // 3. WEEKLY
// ...
growBoundedWidth(&remaining, &bounded.rate_5h, widths.rate_5h); // second pass: grow to content widthIt never renders wider than the terminal (columns that receive no budget are hidden, so no hard-wrap), never produces dot-only cells, and is unit-tested in tests/table_layout_test.zig.
Could you rework this PR along those lines? Concretely:
- Add a list-layout allocator in
src/cli/table_layout.zig, e.g.
boundListWidths(widths: [7]usize, prefix_len: usize, max_cols: ?usize) [7]usize,
reusinggrowBoundedWidth. Suggested first-pass priority for the 7 columns:
ACCOUNT → 5H (4) → WEEKLY (6) → CREDITS (2) → PLAN (4) → RESET CREDITS (2) → LAST (4),
followed by a second pass that grows each column toward its content width while budget remains. - Make the renderer width-injectable:
writeAccountsTableWithUsageOverridesForCols(out, reg, use_color, usage_overrides, term_cols: ?usize),
keeping the current signature as a thin wrapper that passesterminalWidth(). Columns that receive
width 0 are skipped entirely (header + cells + separators), not rendered empty. - Keep your compact-header idea (
CREDITS→CR,RESET CREDITS→RESET/RC) — it fits naturally as the
below-target fallback when a column ends up narrower than its full label. - While you're in there, please cap raw error-name overrides in
setForegroundUsageOverrideForOutcome(src/workflows/usage.zig:686) to the same
max_usage_override_display_widththatformatStatusOverrideAllocuses for status overrides,
soFileNotFounddoesn't depend on column luck to stay readable. - Add scenario tests for the new allocator following the style of
tests/table_layout_test.zig
(given width W:100%complete, no line longer than W, compact headers, no dot-only cells).
With that: wide terminals stay byte-identical, narrow terminals never wrap or dot-collapse, and the layout becomes unit-testable. Happy to review a follow-up push — and if you'd rather not take on the refactor, no problem, just say so and the maintainer can continue from your branch.
Summary
100%, instead of truncating them with a dot.Validation
CODEX_HOME=<empty temp directory> zig build run -- list --skip-apizig build -Doptimize=ReleaseFastThe full test suite was not run.