fix(cli): replace cli-table3 wordWrap with CLITable wrapper#2715
Open
fix(cli): replace cli-table3 wordWrap with CLITable wrapper#2715
Conversation
…t deadlock on large error text (#2619)
iliya-i
requested review from
JivusAyrus,
StarpTech,
endigma,
jensneuse and
wilsonrivera
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces cli-table3 word-wrapping with a deterministic per-cell wrapper: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2715 +/- ##
===========================================
- Coverage 63.14% 35.93% -27.22%
===========================================
Files 249 131 -118
Lines 26661 11755 -14906
Branches 0 486 +486
===========================================
- Hits 16835 4224 -12611
+ Misses 8449 7529 -920
+ Partials 1377 2 -1375
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cli/src/handle-proposal-result.ts (1)
19-57: Consider addingcolWidthsfor non-composition proposal tables to retain bounded wrapping.
CLITablewraps only when a numeric column width is provided. ForchangesTable,lintIssuesTable, andgraphPruningIssuesTable, long messages can now spill into very wide rows.Suggested adjustment
const changesTable = new CLITable({ head: [ pc.bold(pc.white('SUBGRAPH_NAME')), pc.bold(pc.white('CHANGE')), pc.bold(pc.white('TYPE')), pc.bold(pc.white('DESCRIPTION')), ], + colWidths: [24, 16, 22, 120], }); @@ const lintIssuesTable = new CLITable({ @@ - colAligns: ['left', 'left', 'center'], + colAligns: ['left', 'left', 'left', 'center'], + colWidths: [24, 24, 120, 14], }); @@ const graphPruningIssuesTable = new CLITable({ @@ - colAligns: ['left', 'left', 'left', 'center'], + colAligns: ['left', 'left', 'left', 'left', 'center'], + colWidths: [24, 24, 48, 96, 14], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/handle-proposal-result.ts` around lines 19 - 57, The tables changesTable, lintIssuesTable, and graphPruningIssuesTable lack numeric colWidths so long cells can expand rows; update their CLITable constructors (referencing changesTable, lintIssuesTable, graphPruningIssuesTable) to include a colWidths array with sensible numeric widths for the long text columns (e.g., DESCRIPTION in changesTable; ERROR_MESSAGE and LINE NUMBER in lintIssuesTable; FIELD_PATH, MESSAGE and LINE NUMBER in graphPruningIssuesTable) to enable proper wrapping and keep rows bounded while preserving alignment for shorter columns like SUBGRAPH_NAME and RULE.cli/src/cli-table.ts (1)
26-39: Consider guarding against non-positive effective width.If
columnOverhead >= width,wrapTextreceives a non-positivemaxWidth. WhilewrapTexthandles this gracefully (words are emitted without wrapping), this could cause unexpected table rendering if callers misconfigurecolWidths. A defensive check or warning might be valuable for debuggability.🛡️ Optional: Add a defensive minimum width
if (typeof cell === 'string' && typeof width === 'number') { - return wrapText(cell, width - this.columnOverhead); + const effectiveWidth = Math.max(1, width - this.columnOverhead); + return wrapText(cell, effectiveWidth); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/cli-table.ts` around lines 26 - 39, In push([...]) when computing wrap width for each cell, guard against non-positive effective width by computing const effectiveWidth = (typeof width === 'number') ? width - this.columnOverhead : undefined and if effectiveWidth === undefined or effectiveWidth < 1 then either clamp it to 1 before calling wrapText or emit a short warning (e.g., console.warn) referencing the offending column index; update the push method (referencing push, this.colWidths, this.columnOverhead, wrapText) so wrapText is never called with a non-positive maxWidth and callers get a debuggable warning instead of silent odd rendering.cli/test/compose-error-table.test.ts (1)
61-79: Test may be flaky with such small input size.With only 500 bytes of text, both
buggyMsandfixedMscould be 0ms on fast hardware, making the assertionfixedMs < buggyMsfail (0 < 0 is false). Consider increasing the text size or asserting a minimum threshold to make the test more robust.♻️ Suggested fix to increase input size for reliable timing
test('cli-table3 wordWrap is too slow for large error text (demonstrates the bug)', () => { - const text = generateLargeErrorMessage(500); + const text = generateLargeErrorMessage(10_000); // Measure the buggy path: cli-table3 wordWrap: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/compose-error-table.test.ts` around lines 61 - 79, The timing test is flaky because generateLargeErrorMessage(500) can produce 0ms timings on fast machines; increase the input size (e.g., replace generateLargeErrorMessage(500) with a much larger value like 50000) so buggyMs and fixedMs are measurable, and add a guard assertion such as expect(buggyMs).toBeGreaterThan(0) before asserting expect(fixedMs).toBeLessThan(buggyMs) to avoid 0<0 failures; modify the test that references generateLargeErrorMessage, wrapText, TABLE_CONTENT_WIDTH, buggyMs and fixedMs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/commands/graph/federated-graph/commands/move.ts`:
- Around line 102-109: The CLITable instantiation for compositionWarningsTable
has 3 headers but 4 colWidths entries; update the colWidths array in the
compositionWarningsTable CLITable call to match the three headers (e.g., remove
the extra fourth value so it becomes [30, 30, 120] or adjust widths to desired
three-column widths) inside the move.ts file where compositionWarningsTable is
created to ensure the number of colWidths aligns with the header array.
- Around line 39-46: The CLITable instance compositionErrorsTable defines three
headers (FEDERATED_GRAPH_NAME, NAMESPACE, ERROR_MESSAGE) but colWidths only
contains two entries; update the colWidths array on compositionErrorsTable to
have three numeric widths (or remove colWidths to use automatic sizing) so it
matches the three head columns—look for the CLITable constructor call that
creates compositionErrorsTable and adjust the colWidths value accordingly.
---
Nitpick comments:
In `@cli/src/cli-table.ts`:
- Around line 26-39: In push([...]) when computing wrap width for each cell,
guard against non-positive effective width by computing const effectiveWidth =
(typeof width === 'number') ? width - this.columnOverhead : undefined and if
effectiveWidth === undefined or effectiveWidth < 1 then either clamp it to 1
before calling wrapText or emit a short warning (e.g., console.warn) referencing
the offending column index; update the push method (referencing push,
this.colWidths, this.columnOverhead, wrapText) so wrapText is never called with
a non-positive maxWidth and callers get a debuggable warning instead of silent
odd rendering.
In `@cli/src/handle-proposal-result.ts`:
- Around line 19-57: The tables changesTable, lintIssuesTable, and
graphPruningIssuesTable lack numeric colWidths so long cells can expand rows;
update their CLITable constructors (referencing changesTable, lintIssuesTable,
graphPruningIssuesTable) to include a colWidths array with sensible numeric
widths for the long text columns (e.g., DESCRIPTION in changesTable;
ERROR_MESSAGE and LINE NUMBER in lintIssuesTable; FIELD_PATH, MESSAGE and LINE
NUMBER in graphPruningIssuesTable) to enable proper wrapping and keep rows
bounded while preserving alignment for shorter columns like SUBGRAPH_NAME and
RULE.
In `@cli/test/compose-error-table.test.ts`:
- Around line 61-79: The timing test is flaky because
generateLargeErrorMessage(500) can produce 0ms timings on fast machines;
increase the input size (e.g., replace generateLargeErrorMessage(500) with a
much larger value like 50000) so buggyMs and fixedMs are measurable, and add a
guard assertion such as expect(buggyMs).toBeGreaterThan(0) before asserting
expect(fixedMs).toBeLessThan(buggyMs) to avoid 0<0 failures; modify the test
that references generateLargeErrorMessage, wrapText, TABLE_CONTENT_WIDTH,
buggyMs and fixedMs accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37ab2a55-bd6c-49b9-8021-5ecc036a8cc8
📒 Files selected for processing (34)
cli/src/cli-table.tscli/src/commands/contract/commands/create.tscli/src/commands/contract/commands/update.tscli/src/commands/feature-flag/commands/list.tscli/src/commands/graph/common/version/commands/get.tscli/src/commands/graph/common/version/commands/set.tscli/src/commands/graph/federated-graph/commands/check.tscli/src/commands/graph/federated-graph/commands/create.tscli/src/commands/graph/federated-graph/commands/list.tscli/src/commands/graph/federated-graph/commands/move.tscli/src/commands/graph/federated-graph/commands/update.tscli/src/commands/graph/monograph/commands/list.tscli/src/commands/graph/monograph/commands/move.tscli/src/commands/graph/monograph/commands/publish.tscli/src/commands/grpc-service/commands/delete.tscli/src/commands/grpc-service/commands/publish.tscli/src/commands/namespace/commands/list.tscli/src/commands/router/commands/compatibility-version/commands/list.tscli/src/commands/router/commands/compose.tscli/src/commands/router/commands/plugin/commands/delete.tscli/src/commands/router/commands/plugin/commands/publish.tscli/src/commands/router/commands/token/commands/list.tscli/src/commands/subgraph/commands/delete.tscli/src/commands/subgraph/commands/list.tscli/src/commands/subgraph/commands/move.tscli/src/commands/subgraph/commands/publish.tscli/src/commands/subgraph/commands/update.tscli/src/handle-check-result.tscli/src/handle-composition-result.tscli/src/handle-proposal-result.tscli/src/wrap-text.tscli/test/cli-table.test.tscli/test/compose-error-table.test.tscli/test/wrap-text.test.ts
endigma
reviewed
Mar 31, 2026
Comment on lines
+12
to
+44
| export class CLITable { | ||
| private table: InstanceType<typeof Table>; | ||
| private colWidths: (number | null)[] | undefined; | ||
| private columnOverhead: number; | ||
|
|
||
| constructor(options: CLITableOptions = {}) { | ||
| const { columnOverhead, ...tableOptions } = options; | ||
| this.table = new Table(tableOptions); | ||
| this.colWidths = tableOptions.colWidths; | ||
|
|
||
| const { style } = this.table.options; | ||
| this.columnOverhead = columnOverhead ?? style['padding-left'] + style['padding-right']; | ||
| } | ||
|
|
||
| push(...rows: Cell[][]): void { | ||
| for (const row of rows) { | ||
| const wrapped = this.colWidths | ||
| ? row.map((cell, i) => { | ||
| const width = this.colWidths![i]; | ||
| if (typeof cell === 'string' && typeof width === 'number') { | ||
| return wrapText(cell, width - this.columnOverhead); | ||
| } | ||
| return cell; | ||
| }) | ||
| : row; | ||
| this.table.push(wrapped); | ||
| } | ||
| } | ||
|
|
||
| toString(): string { | ||
| return this.table.toString(); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
If you do export ... as Table instead of changing every import to CLITable this would shrink the diff by a lot
Author
There was a problem hiding this comment.
@endigma I made it different on purpose, since exporting as Table could be confused with the original cli-table3 package, but I can change it feel strongly about reducing the diff?
Member
There was a problem hiding this comment.
more a nit than anything, up to you
This was referenced Mar 31, 2026
comatory
approved these changes
Mar 31, 2026
cli/test/cli-table.test.ts
Outdated
| expect(spy).toHaveBeenCalledTimes(1); | ||
| expect(spy).toHaveBeenCalledWith('wrapped', 30 - DEFAULT_OVERHEAD); | ||
|
|
||
| spy.mockRestore(); |
Contributor
There was a problem hiding this comment.
vite.config.ts has restoreMocks: true so this should not be needed
| * This avoids cli-table3's built-in wordWrap which can deadlock due to | ||
| * expensive string-width/emoji-regex evaluations on large inputs (#2619). | ||
| */ | ||
| export function wrapText(text: string, maxWidth: number): string { |
Contributor
There was a problem hiding this comment.
The function is used internally within just the table class right? Why not make it part of the class itself. It does not have access to this so it could be a static method. We can keep separate tests for it in extra describe block in cli-table.test.ts WDYT?
Author
There was a problem hiding this comment.
this way it's a pure function and it's much easier to test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/test/cli-table.test.ts`:
- Around line 18-23: Remove the extra blank lines before the test block closers
so the padded-blocks ESLint rule passes; in cli/test/cli-table.test.ts collapse
the blank line(s) immediately before the closing "});" after the expect
assertions (the blocks containing expect(spy).toHaveBeenCalledTimes(2) /
expect(spy).toHaveBeenCalledWith(...)) so there is no empty line between the
last statement and the "});" in both affected test cases.
- Around line 40-51: The deadlock regression test "does not deadlock on large
text (`#2619`)" uses too-small payload and a brittle wall-clock assertion; update
the test that constructs CLITable and largeText to use a much larger payload
(e.g., increase the repeat count substantially) and remove the elapsed time
assertion (expect(elapsed).toBeLessThan(500)), relying on the existing test
timeout (5000ms) to detect hangs instead; keep references to CLITable,
largeText, and the test name so the change is applied to the correct test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10a479f0-df4b-45b3-9570-bd74e984fbab
📒 Files selected for processing (1)
cli/test/cli-table.test.ts
Comment on lines
+40
to
+51
| test('does not deadlock on large text (#2619)', () => { | ||
| const table = new CLITable({ | ||
| head: ['ERROR_MESSAGE'], | ||
| colWidths: [120], | ||
| }); | ||
| const largeText = 'error '.repeat(100).trim(); | ||
| const t0 = Date.now(); | ||
| table.push([largeText]); | ||
| table.toString(); | ||
| const elapsed = Date.now() - t0; | ||
| expect(elapsed).toBeLessThan(500); | ||
| }, 5000); |
Contributor
There was a problem hiding this comment.
Strengthen the deadlock regression and remove timing flakiness.
At Line 45, repeat(100) is too small for the large-output scenario, and Line 50’s <500ms wall-clock assertion is brittle across CI runners. Use a much larger payload and rely on the test timeout to catch hangs.
Proposed regression-test adjustment
test('does not deadlock on large text (`#2619`)', () => {
@@
- const largeText = 'error '.repeat(100).trim();
- const t0 = Date.now();
- table.push([largeText]);
- table.toString();
- const elapsed = Date.now() - t0;
- expect(elapsed).toBeLessThan(500);
+ const largeText = 'error '.repeat(200_000).trim();
+ expect(() => {
+ table.push([largeText]);
+ table.toString();
+ }).not.toThrow();
}, 5000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/test/cli-table.test.ts` around lines 40 - 51, The deadlock regression
test "does not deadlock on large text (`#2619`)" uses too-small payload and a
brittle wall-clock assertion; update the test that constructs CLITable and
largeText to use a much larger payload (e.g., increase the repeat count
substantially) and remove the elapsed time assertion
(expect(elapsed).toBeLessThan(500)), relying on the existing test timeout
(5000ms) to detect hangs instead; keep references to CLITable, largeText, and
the test name so the change is applied to the correct test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2619
cli-table3's built-inwordWrapoption usesstring-widthwhich executesemoji-regexon every word in every table cell. Rendering time grows super-linearly with text volume, causing the CLI process to hang indefinitely when composition errors produce large output.This PR introduces a
CLITablewrapper class that replacescli-table3'swordWrapwith a lightweightwrapTextutility that breaks text on word boundaries using simple character counting. The wrapper automatically wraps all string cells based on their column width, so developers never need to think about it.cli/src/wrap-text.ts: text wrapping utilitycli/src/cli-table.ts:CLITableclass that wrapscli-table3, automatically applieswrapTextper column based oncolWidths, omitswordWrapandwrapOnWordBoundaryfrom the type to prevent misusenew Table/wordWrap: trueusagescli-table3instance styleBased on the root cause analysis and
wrapTextutility from @kamil-gwozdz in #2620.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.