Exempt URL columns from width cap in generic table renderer - #309
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a mismatch where selectColumns() capped column widths at 40 characters for column-dropping math, but formatCell() never truncated URLs, causing URL columns to overflow the terminal.
Changes:
- Added
containsURLflag to thecolumnstruct, set during the existing width-measurement loop - Skipped the 40-char width cap for columns containing URLs so budget math matches actual rendered width
- Added two new tests and updated an existing test to validate the URL exemption behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/output/render.go | Added containsURL field and URL-detection logic; exempts URL columns from width cap |
| internal/output/output_test.go | Updated existing URL test, added two new tests for URL column width exemption |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/output/output_test.go">
<violation number="1" location="internal/output/output_test.go:2475">
P3: Typo in test name: `Exemps` → `Exempts`. This misspelling appears in both new test functions and will show up in test output / grep results.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
selectColumns() capped all column widths at 40 for column-dropping arithmetic, but formatCell() never truncates URLs. The mismatch caused the budget to understate URL-bearing columns, keeping too many columns in the layout, and the lipgloss table overflowed — garbling URLs with line wraps. Track a containsURL flag on the column struct during the existing width loop (same formatTableCell render path), and skip the 40-char cap for flagged columns. Shorten the URL in TestStyledRenderTablePreservesURLs so it fits an 80-char terminal now that the budget is accurate.
There was a problem hiding this comment.
Pull request overview
Fixes a table rendering bug where URL columns overflowed the terminal because selectColumns() capped widths at 40 chars but formatCell() never truncated URLs, causing a mismatch in column budget calculations.
Changes:
- Added
containsURLflag to thecolumnstruct and detect URL content during width measurement - Skip the 40-char width cap for columns containing URLs so column-dropping math matches actual rendering
- Added two new tests and updated an existing test URL to fit within 80-char terminal width
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/output/render.go | Added containsURL field and logic to exempt URL columns from width cap |
| internal/output/output_test.go | Added tests for URL column exemption; shortened test URL to fit terminal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e9247c31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* Bump SDK to 81660dc1 and surface BC5 forward-compat fields The bc5-readiness stack (basecamp-sdk #293 + #309) merged to SDK main, adding BC5 forward-compat fields to the Go wrappers. Bump to the merge commit and surface the new fields: - Todo.Steps render as a checklist section on `todos show` via a new `steps` presenter format; a breadcrumb points at the existing `cards step` mutation surface. Empty detail sections (heading included) are now skipped so BC4 todos render unchanged. - Todoset BC5 counts join the `todosets show` summary, with a breadcrumb to `todos list` when the todoset carries loose todos. - Notifications combine Bubble Ups + Scheduled Bubble Ups (BC5) with Memories (BC4) without double-counting BC5's memories compat alias, and `notifications read` resolves IDs from all five sections. - Person.tagline, todoset counts/URLs, and notification bubble-up fields flow through JSON and generic output automatically. * Address PR review: sanitize step titles, fix --json hint, skip empty Markdown sections - Step titles pass through richtext.SanitizeSingleLine before rendering, matching the terminal-injection defense in the other format functions. - The todos show steps breadcrumb now names the real --json flag. - Markdown detail rendering gets the same visible-field prepass as the styled path, so a todo without steps no longer emits an empty "#### Steps" heading. - Tests: formatSteps ordering/markers/sanitization, and Markdown heading presence/absence for the Steps section. * Section-skip from formatted output, shared between styled and Markdown The empty-section prepass counted detail fields as visible before checking whether they format to anything, so an incomplete todo with no due date still rendered a bare Status heading. Both renderers now build the section from a shared filtered list of (field, formatted) pairs where collapse rules pass and formatting yields output; label alignment is computed from that filtered list. The comments-rendering test asserted on the Status heading as its presenter artifact — exactly the empty-heading bug — so it now asserts the schema affordance hint instead, and new tests cover empty-Status omission and due-date presence in both modes.
Summary
selectColumns()capped all column widths at 40 for column-dropping math, butformatCell()never truncates URLs — the mismatch caused URL-bearing columns to overflow the terminal and garble with line wrapscontainsURLflag on thecolumnstruct during the existing width loop (sameformatTableCellrender path), skip the 40-char cap for flagged columnshref,app_href,*_url— any column whose values passisURL()Test plan
TestSelectColumnsExempsURLColumnsFromWidthCap—hrefretains actual width,descriptioncapped at 40TestSelectColumnsExempsURLColumnsForSuffixFields—todolists_urlretains actual width (drift guard)TestStyledRenderTablePreservesURLsupdated to use a URL that fits 80-char terminal now that budget is accuratego vetand full test suite passSummary by cubic
Fixes table overflow by exempting URL columns from the 40‑char width cap so the column budget matches actual rendering. Keeps long URLs intact and prevents line-wrap garbling in terminal tables.
href,app_href, and any*_urlfield detected via URL parsing.hrefand*_url; shortened a test URL to fit an 80‑char terminal now that widths are accurate.Written for commit 76e9247. Summary will update on new commits.