Skip to content

Commit d41b0bd

Browse files
committed
fix: clean up upgrade output and hide empty table headers
Clean up the ugly `cli upgrade` output and fix a systemic issue with empty header rows in box-drawing tables across the codebase. ## Upgrade output Before: ``` Binary: Installed to /home/byk/.local/bin/sentry ✓ Upgraded to 0.18.0-dev.1773836960 0.18.0-dev.1773794950 → 0.18.0-dev.1773836960 ╭─────────┬─────────╮ │ │ │ ├─────────┼─────────┤ │ Method │ curl │ │ Channel │ nightly │ ╰─────────┴─────────╯ ``` After: ``` ✓ Upgraded to 0.18.0-dev.1773836960 (from 0.18.0-dev.1773794950) Method: curl · Channel: nightly ``` Changes: - Replace metadata table with a compact muted inline line - Remove redundant version arrow; fold "from" version into success line - Add `--quiet` to `cli setup` subprocess to suppress leaked stdout ## Systemic table fix `mdKvTable()` without a heading produces `| | |` empty header cells, which rendered as an ugly empty bordered row in box-drawing tables. This affected 12 call sites: auth status, issue/org/project details, trace/span/log views, and more. Fix: add `hideHeaders` option to `renderTextTable` and auto-detect all-empty headers in `renderTableToken` to skip the header row while still using it for column width measurement.
1 parent b172db5 commit d41b0bd

File tree

8 files changed

+186
-38
lines changed

8 files changed

+186
-38
lines changed

src/commands/cli/upgrade.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@ async function runSetupOnNewBinary(opts: SetupOptions): Promise<void> {
379379
const args = [
380380
"cli",
381381
"setup",
382+
"--quiet",
382383
"--method",
383384
method,
384385
"--channel",

src/lib/formatters/human.ts

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2071,8 +2071,8 @@ const ACTION_DESCRIPTIONS: Record<UpgradeResult["action"], string> = {
20712071
/**
20722072
* Format upgrade result as rendered markdown.
20732073
*
2074-
* Produces a concise summary line with the action taken, version info,
2075-
* and any warnings (e.g., PATH shadowing from old package manager install).
2074+
* Produces a concise summary: action line, compact metadata, and any
2075+
* warnings (e.g., PATH shadowing from old package manager install).
20762076
* Designed as the `human` formatter for the `cli upgrade` command's
20772077
* {@link OutputConfig}.
20782078
*
@@ -2086,13 +2086,17 @@ export function formatUpgradeResult(data: UpgradeResult): string {
20862086
case "upgraded":
20872087
case "downgraded": {
20882088
const verb = ACTION_DESCRIPTIONS[data.action];
2089-
const offlineNote = data.offline ? " (offline, from cache)" : "";
2090-
lines.push(
2091-
`${colorTag("green", "✓")} ${verb} to ${safeCodeSpan(data.targetVersion)}${escapeMarkdownInline(offlineNote)}`
2092-
);
2093-
if (data.currentVersion !== data.targetVersion) {
2089+
if (data.offline) {
2090+
lines.push(
2091+
`${colorTag("green", "✓")} ${verb} to ${safeCodeSpan(data.targetVersion)}${escapeMarkdownInline(" (offline, from cache)")}`
2092+
);
2093+
} else if (data.currentVersion !== data.targetVersion) {
2094+
lines.push(
2095+
`${colorTag("green", "✓")} ${verb} to ${safeCodeSpan(data.targetVersion)} ${escapeMarkdownInline(`(from ${data.currentVersion})`)}`
2096+
);
2097+
} else {
20942098
lines.push(
2095-
`${escapeMarkdownInline(data.currentVersion)} ${escapeMarkdownInline(data.targetVersion)}`
2099+
`${colorTag("green", "✓")} ${verb} to ${safeCodeSpan(data.targetVersion)}`
20962100
);
20972101
}
20982102
break;
@@ -2123,19 +2127,13 @@ export function formatUpgradeResult(data: UpgradeResult): string {
21232127
}
21242128
}
21252129

2126-
// Metadata table — renders method, channel, and version info below the
2127-
// action line so diagnostics go to stdout (via the markdown pipeline)
2128-
// instead of cluttering stderr with repeated log.info() lines.
2129-
const kvRows: [string, string][] = [
2130-
["Method", escapeMarkdownInline(data.method)],
2131-
["Channel", escapeMarkdownInline(data.channel)],
2132-
];
2133-
lines.push("");
2134-
lines.push(mdKvTable(kvRows));
2130+
// Compact metadata line instead of a full table — method and channel
2131+
// are lightweight diagnostics that don't warrant box-drawing borders.
2132+
const meta = `Method: ${data.method} · Channel: ${data.channel}`;
2133+
lines.push(` ${colorTag("muted", escapeMarkdownInline(meta))}`);
21352134

21362135
// Append warnings with ⚠ markers
21372136
if (data.warnings && data.warnings.length > 0) {
2138-
lines.push("");
21392137
for (const warning of data.warnings) {
21402138
lines.push(`${colorTag("yellow", "⚠")} ${escapeMarkdownInline(warning)}`);
21412139
}

src/lib/formatters/markdown.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,10 @@ function renderList(list: Tokens.List, depth = 0): string {
554554
*
555555
* Converts marked's `Tokens.Table` into headers + rows + alignments and
556556
* delegates to `renderTextTable()` for column fitting and box drawing.
557+
*
558+
* When all header cells are empty (e.g. from {@link mdKvTable} without a
559+
* heading), the header row is hidden so the rendered table starts directly
560+
* with data rows — no empty cells or separator line.
557561
*/
558562
function renderTableToken(table: Tokens.Table): string {
559563
const headers = table.header.map((cell) => renderInline(cell.tokens));
@@ -571,7 +575,11 @@ function renderTableToken(table: Tokens.Table): string {
571575
return "left";
572576
});
573577

574-
return renderTextTable(headers, rows, { alignments });
578+
// Hide the header row when all cells are empty — avoids rendering a
579+
// visually empty bordered row above the data (common in mdKvTable output).
580+
const hideHeaders = headers.every((h) => h.trim() === "");
581+
582+
return renderTextTable(headers, rows, { alignments, hideHeaders });
575583
}
576584

577585
// ──────────────────────── Public API ─────────────────────────────────

src/lib/formatters/text-table.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,17 @@ export type TextTableOptions = {
5656
shrinkable?: boolean[];
5757
/** Truncate cells to one line with "\u2026" instead of wrapping. @default false */
5858
truncate?: boolean;
59+
/**
60+
* Hide the header row entirely (omit from rendered output).
61+
*
62+
* Headers are still used for column width measurement, but the rendered
63+
* table starts directly with data rows. Useful for key-value tables where
64+
* the markdown source requires empty header cells (`| | |`) to satisfy
65+
* table syntax, but the visual output shouldn't show them.
66+
*
67+
* @default false
68+
*/
69+
hideHeaders?: boolean;
5970
/**
6071
* Show horizontal separator lines between data rows.
6172
*
@@ -94,6 +105,7 @@ export function renderTextTable(
94105
minWidths = [],
95106
shrinkable = [],
96107
truncate = false,
108+
hideHeaders = false,
97109
rowSeparator = false,
98110
} = options;
99111

@@ -139,6 +151,7 @@ export function renderTextTable(
139151
border,
140152
cellPadding,
141153
headerSeparator,
154+
hideHeaders,
142155
rowSeparator,
143156
});
144157
}
@@ -512,6 +525,8 @@ type GridParams = {
512525
border: BorderCharacters;
513526
cellPadding: number;
514527
headerSeparator: boolean;
528+
/** Skip rendering the header row (allRows[0]) entirely. */
529+
hideHeaders: boolean;
515530
/** Draw separator between data rows. `true` for plain, or ANSI color prefix string. */
516531
rowSeparator: boolean | string;
517532
};
@@ -562,6 +577,7 @@ function renderGrid(params: GridParams): string {
562577
border,
563578
cellPadding,
564579
headerSeparator,
580+
hideHeaders,
565581
rowSeparator,
566582
} = params;
567583
const lines: string[] = [];
@@ -607,7 +623,11 @@ function renderGrid(params: GridParams): string {
607623
vert,
608624
};
609625

610-
for (let r = 0; r < allRows.length; r++) {
626+
// When hideHeaders is set, skip allRows[0] (the header) and its separator.
627+
// The header is still included in allRows for column width measurement.
628+
const startRow = hideHeaders ? 1 : 0;
629+
630+
for (let r = startRow; r < allRows.length; r++) {
611631
const wrappedCells = allRows[r] ?? [];
612632
lines.push(...renderRowLines(wrappedCells, rowCtx));
613633

test/commands/cli/upgrade.test.ts

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,8 @@ function createMockContext(
6666
...overrides.env,
6767
};
6868

69-
// Force plain output so formatUpgradeResult renders raw markdown tables
70-
// (ASCII pipes) instead of Unicode box-drawing characters in TTY mode.
69+
// Force plain output so formatUpgradeResult renders raw markdown
70+
// instead of ANSI-styled output in TTY mode.
7171
const origPlain = process.env.SENTRY_PLAIN_OUTPUT;
7272
process.env.SENTRY_PLAIN_OUTPUT = "1";
7373

@@ -281,7 +281,7 @@ describe("sentry cli upgrade", () => {
281281
);
282282

283283
const combined = getOutput();
284-
expect(combined).toContain("| curl |");
284+
expect(combined).toContain("Method: curl");
285285
expect(combined).toContain(CLI_VERSION);
286286
expect(combined).toContain("You are already on the target version");
287287
});
@@ -375,7 +375,7 @@ describe("sentry cli upgrade", () => {
375375
);
376376

377377
const combined = getOutput();
378-
expect(combined).toContain("| brew |");
378+
expect(combined).toContain("Method: brew");
379379
expect(combined).toContain("99.99.99");
380380
expect(combined).toContain("Run 'sentry cli upgrade' to update.");
381381
});
@@ -512,7 +512,7 @@ describe("sentry cli upgrade — nightly channel", () => {
512512
);
513513

514514
const combined = getOutput();
515-
expect(combined).toContain("| nightly |");
515+
expect(combined).toContain("Channel: nightly");
516516
});
517517

518518
test("'stable' positional sets channel to stable", async () => {
@@ -531,7 +531,7 @@ describe("sentry cli upgrade — nightly channel", () => {
531531
);
532532

533533
const combined = getOutput();
534-
expect(combined).toContain("| stable |");
534+
expect(combined).toContain("Channel: stable");
535535
});
536536

537537
test("without positional, uses persisted channel", async () => {
@@ -550,7 +550,7 @@ describe("sentry cli upgrade — nightly channel", () => {
550550
);
551551

552552
const combined = getOutput();
553-
expect(combined).toContain("| nightly |");
553+
expect(combined).toContain("Channel: nightly");
554554
});
555555
});
556556

@@ -605,7 +605,7 @@ describe("sentry cli upgrade — nightly channel", () => {
605605
);
606606

607607
const combined = getOutput();
608-
expect(combined).toContain("| nightly |");
608+
expect(combined).toContain("Channel: nightly");
609609
expect(combined).toContain(CLI_VERSION);
610610
expect(combined).toContain("You are already on the target version");
611611
});
@@ -625,7 +625,7 @@ describe("sentry cli upgrade — nightly channel", () => {
625625
);
626626

627627
const combined = getOutput();
628-
expect(combined).toContain("| nightly |");
628+
expect(combined).toContain("Channel: nightly");
629629
expect(combined).toContain("0.99.0-dev.9999999999");
630630
expect(combined).toContain("Run 'sentry cli upgrade' to update.");
631631
});
@@ -741,6 +741,7 @@ describe("sentry cli upgrade — curl full upgrade path (Bun.spawn spy)", () =>
741741
expect(setupCall).toBeDefined();
742742
expect(setupCall).toContain("cli");
743743
expect(setupCall).toContain("setup");
744+
expect(setupCall).toContain("--quiet");
744745
expect(setupCall).toContain("--method");
745746
expect(setupCall).toContain("curl");
746747
expect(setupCall).toContain("--install");

test/lib/formatters/human.test.ts

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,7 @@ describe("formatUpgradeResult", () => {
756756
}
757757
}
758758

759-
test("renders metadata table with method and channel", () => {
759+
test("renders compact metadata line with method and channel", () => {
760760
withPlain(() => {
761761
const result = formatUpgradeResult({
762762
action: "upgraded",
@@ -766,12 +766,28 @@ describe("formatUpgradeResult", () => {
766766
method: "curl",
767767
forced: false,
768768
});
769-
expect(result).toContain("| **Method** | curl |");
770-
expect(result).toContain("| **Channel** | stable |");
769+
expect(result).toContain("Method: curl");
770+
expect(result).toContain("Channel: stable");
771771
});
772772
});
773773

774-
test("renders nightly channel in metadata table", () => {
774+
test("includes from-version in upgraded action", () => {
775+
withPlain(() => {
776+
const result = formatUpgradeResult({
777+
action: "upgraded",
778+
currentVersion: "0.5.0",
779+
targetVersion: "0.6.0",
780+
channel: "stable",
781+
method: "curl",
782+
forced: false,
783+
});
784+
expect(result).toContain("Upgraded to");
785+
expect(result).toContain("0.6.0");
786+
expect(result).toContain("(from 0.5.0)");
787+
});
788+
});
789+
790+
test("renders nightly channel in metadata line", () => {
775791
withPlain(() => {
776792
const result = formatUpgradeResult({
777793
action: "checked",
@@ -781,12 +797,12 @@ describe("formatUpgradeResult", () => {
781797
method: "npm",
782798
forced: false,
783799
});
784-
expect(result).toContain("| **Method** | npm |");
785-
expect(result).toContain("| **Channel** | nightly |");
800+
expect(result).toContain("Method: npm");
801+
expect(result).toContain("Channel: nightly");
786802
});
787803
});
788804

789-
test("up-to-date action includes metadata table", () => {
805+
test("up-to-date action includes compact metadata", () => {
790806
withPlain(() => {
791807
const result = formatUpgradeResult({
792808
action: "up-to-date",
@@ -797,8 +813,39 @@ describe("formatUpgradeResult", () => {
797813
forced: false,
798814
});
799815
expect(result).toContain("Already up to date");
800-
expect(result).toContain("| **Method** | brew |");
801-
expect(result).toContain("| **Channel** | stable |");
816+
expect(result).toContain("Method: brew");
817+
expect(result).toContain("Channel: stable");
818+
});
819+
});
820+
821+
test("offline upgrade shows offline note instead of from-version", () => {
822+
withPlain(() => {
823+
const result = formatUpgradeResult({
824+
action: "upgraded",
825+
currentVersion: "0.5.0",
826+
targetVersion: "0.6.0",
827+
channel: "stable",
828+
method: "curl",
829+
forced: false,
830+
offline: true,
831+
});
832+
expect(result).toContain("(offline, from cache)");
833+
expect(result).not.toContain("(from 0.5.0)");
834+
});
835+
});
836+
837+
test("does not include from-version when versions match", () => {
838+
withPlain(() => {
839+
const result = formatUpgradeResult({
840+
action: "upgraded",
841+
currentVersion: "0.5.0",
842+
targetVersion: "0.5.0",
843+
channel: "stable",
844+
method: "curl",
845+
forced: true,
846+
});
847+
expect(result).toContain("Upgraded to");
848+
expect(result).not.toContain("(from");
802849
});
803850
});
804851
});

test/lib/formatters/markdown.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,40 @@ describe("renderMarkdown blocks (rendered mode)", () => {
703703
expect(stripAnsi(result)).toContain("1");
704704
expect(stripAnsi(result)).toContain("2");
705705
});
706+
707+
test("hides empty header row in rendered KV tables", () => {
708+
// mdKvTable without heading produces `| | |` empty headers
709+
const md = mdKvTable([
710+
["Method", "curl"],
711+
["Channel", "stable"],
712+
]);
713+
const result = rendered(md);
714+
const plain = stripAnsi(result);
715+
716+
// Data rows should be present
717+
expect(plain).toContain("Method");
718+
expect(plain).toContain("curl");
719+
expect(plain).toContain("Channel");
720+
expect(plain).toContain("stable");
721+
722+
// Count content rows (lines containing │ vertical border)
723+
const contentLines = plain.split("\n").filter((l) => l.includes("\u2502"));
724+
// Should have exactly 2 data rows, no empty header row
725+
expect(contentLines).toHaveLength(2);
726+
});
727+
728+
test("shows header row when headers are non-empty", () => {
729+
const md = "| Key | Value |\n|---|---|\n| a | b |";
730+
const result = rendered(md);
731+
const plain = stripAnsi(result);
732+
733+
expect(plain).toContain("Key");
734+
expect(plain).toContain("Value");
735+
736+
// Content rows: header + data = 3 minimum
737+
const contentLines = plain.split("\n").filter((l) => l.includes("\u2502"));
738+
expect(contentLines.length).toBeGreaterThanOrEqual(2);
739+
});
706740
});
707741

708742
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)