fix(cli): avoid cli-table3 wordWrap deadlock with 4+ subgraphs#2620
fix(cli): avoid cli-table3 wordWrap deadlock with 4+ subgraphs#2620kamil-gwozdz wants to merge 1 commit intowundergraph:mainfrom
Conversation
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a text-wrapping utility and replaces cli-table3 wordWrap usage by pre-wrapping error and warning messages before table insertion; also adds unit tests for the wrapper and a test exercising the compose error table rendering path. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/test/wrap-text.test.ts (1)
25-33: Consider tightening the assertion tolerance.The test asserts
line.length <= 35for amaxWidthof 30, which is a 16% overage. WhilewrapTextcan legitimately exceedmaxWidthfor single words longer than the limit, this test's assertion is loose enough that it wouldn't catch certain wrapping bugs.For word-boundary wrapping where words are short, lines should be at or below
maxWidth. Consider splitting the assertion:for (const line of lines) { // Lines should not exceed maxWidth unless they contain a single word longer than maxWidth const words = line.split(' '); if (words.length > 1) { expect(line.length).toBeLessThanOrEqual(30); } }This is a minor suggestion—the current test still validates the core behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/wrap-text.test.ts` around lines 25 - 33, The test's length tolerance is too loose; update the assertion in the wrap-text.test to enforce maxWidth for normal wrapped lines: iterate over lines (the lines array from result) and for each line split into words, and if words.length > 1 assert line.length <= maxWidth (use the same numeric maxWidth variable used when calling wrapText, e.g., 30); allow longer lengths only when a single word exceeds maxWidth. This change targets the test block using input, result, lines and wrapText.cli/src/handle-composition-result.ts (1)
12-12: Apply the samewrapTextfix tohandle-proposal-result.tsfor consistency.
cli/src/handle-proposal-result.tscurrently useswordWrap: truein multiple Table configurations (lines 26, 32, 38, 49, 61). Update these tables to usewrapTextfrom./utils.jsinstead, consistent with the fix applied tohandle-composition-result.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/handle-composition-result.ts` at line 12, handle-proposal-result.ts still uses deprecated/incorrect Table options with wordWrap: true in several Table configurations; replace those usages by importing and using the wrapText helper from ./utils.js (same as in handle-composition-result.ts). Specifically, add import { wrapText } from './utils.js' at the top of handle-proposal-result.ts if missing, then for each Table definition that sets wordWrap: true (the tables around the blocks with wordWrap at the spots corresponding to the earlier lines 26, 32, 38, 49, 61) remove wordWrap and pass wrapText for the cell wrapping (e.g., use wrapText(cellValue, width) or whatever wrapText signature is used elsewhere) so the table rendering matches handle-composition-result.ts behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/src/handle-composition-result.ts`:
- Line 12: handle-proposal-result.ts still uses deprecated/incorrect Table
options with wordWrap: true in several Table configurations; replace those
usages by importing and using the wrapText helper from ./utils.js (same as in
handle-composition-result.ts). Specifically, add import { wrapText } from
'./utils.js' at the top of handle-proposal-result.ts if missing, then for each
Table definition that sets wordWrap: true (the tables around the blocks with
wordWrap at the spots corresponding to the earlier lines 26, 32, 38, 49, 61)
remove wordWrap and pass wrapText for the cell wrapping (e.g., use
wrapText(cellValue, width) or whatever wrapText signature is used elsewhere) so
the table rendering matches handle-composition-result.ts behavior.
In `@cli/test/wrap-text.test.ts`:
- Around line 25-33: The test's length tolerance is too loose; update the
assertion in the wrap-text.test to enforce maxWidth for normal wrapped lines:
iterate over lines (the lines array from result) and for each line split into
words, and if words.length > 1 assert line.length <= maxWidth (use the same
numeric maxWidth variable used when calling wrapText, e.g., 30); allow longer
lengths only when a single word exceeds maxWidth. This change targets the test
block using input, result, lines and wrapText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2743d5-d797-4e83-8bf3-7412675425e2
📒 Files selected for processing (5)
cli/src/commands/router/commands/compose.tscli/src/handle-composition-result.tscli/src/utils.tscli/src/wrap-text.tscli/test/wrap-text.test.ts
1c30315 to
ea579d1
Compare
…rgraph#2619) Replace cli-table3's built-in wordWrap with a lightweight wrapText() utility that wraps text on word boundaries using simple character counting. This avoids the expensive string-width/emoji-regex code path in cli-table3 that causes the Node.js main thread to block indefinitely when rendering large error tables. Fixes: wundergraph#2619 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ea579d1 to
5d92b16
Compare
Summary
Fixes #2619
wgc router composedeadlocks when federation composition produces errors and the config references 4+ subgraphs. The process prints the error header but hangs indefinitely before rendering the error table.Root Cause
cli-table3's built-in
wordWrap: trueoption triggers an expensive code path throughstring-width→emoji-regexfor every word in every table cell. With enough error content (4+ subgraphs), this blocks the Node.js main thread indefinitely, preventing signal handling and event loop progress.Fix
wrapText()utility (cli/src/wrap-text.ts) that wraps text on word boundaries using simple character counting — no heavy regex dependencies.TABLE_CONTENT_WIDTHconstant (116 = 120 column width − 2 padding − 2 borders) to avoid magic numbers.wordWrap: truefrom all affected table configurations.Files Changed
cli/src/wrap-text.tswrapText()andTABLE_CONTENT_WIDTH— zero dependenciescli/src/utils.tswrapTextfor backward compatibilitycli/src/commands/router/commands/compose.tscli/src/handle-composition-result.tscli/test/wrap-text.test.tswrapText, including a regression test verifying 500-word inputs complete in <1scli/test/compose-error-table.test.ts@override, verifying the error table renders without hanging (5s timeout)Testing
wrap-text.test.ts— 10 tests covering word wrapping edge cases, empty inputs, long words, paragraph preservation, and a performance regression test.compose-error-table.test.ts— Integration test using the exact reproduction schemas from the issue (4 subgraphs with circular@overridebetween subgraph-b and subgraph-c). RunsfederateSubgraphs, asserts composition fails, renders the error table through the fixed code path, and verifies output correctness within a 5-second timeout.Summary by CodeRabbit
Bug Fixes
Tests