FRO-211 / ROB-3946 Truncate over-wide summary table cells instead of wrapping#2123
FRO-211 / ROB-3946 Truncate over-wide summary table cells instead of wrapping#2123alonelish wants to merge 4 commits into
Conversation
The Slack "Alerts Summary" digest rendered a corrupted ASCII table: long values in text columns (e.g. Java class names in `label:site`) were passed to tabulate via `maxcolwidths`, which *wraps* cells onto extra physical lines rather than truncating them, mangling every following row. TableBlock.to_table_string now truncates over-wide cells itself (with a single-char "…" ellipsis) so each row stays on one line, and no longer passes maxcolwidths to tabulate. Dotted, space-free qualified names are trimmed from the left to keep the distinctive class-name suffix (e.g. `…settler.AbstractBetSettler`); other text is trimmed from the right. __calc_max_width now water-fills the reduction across the widest text columns (fair distribution, with a minimum width floor) and never shrinks numeric columns, so the Fired/Resolved counters are always shown in full. Adds regression tests proving rows stay single-line and are truncated (not wrapped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:e80816d
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:e80816d me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:e80816d
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:e80816dPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:e80816d |
Walkthrough
ChangesTable rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/robusta/core/reporting/blocks.py`:
- Around line 410-413: Update the shrinkable-column selection around
__numeric_column_indices so an all-numeric table does not fall back to
list(range(num_columns)). Preserve numeric columns at full width when no
non-numeric column exists, while retaining the existing behavior of shrinking
non-numeric columns when available.
- Around line 399-403: Update the table width calculation around num_columns and
columns_max_widths so it expands to cover every column in rendered_rows,
including when headers is empty or rows contain more values than headers.
Preserve header widths and use the resulting column widths for subsequent
rendering without indexing beyond the list.
🪄 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: 7dc50a4d-7ee1-45bf-9126-dc189ce565f8
📒 Files selected for processing (2)
src/robusta/core/reporting/blocks.pytests/test_blocks.py
- __calc_max_width now sizes columns to the widest row, not just the headers, so headerless or ragged tables no longer raise IndexError. - An all-numeric over-width table is left at full width instead of ellipsizing the numbers. - Shorten code/test comments; add regression tests for both cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_blocks.py (1)
159-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant and ineffective wrap-spillover assertion.
The assertion
not any(line.strip() in ("ServiceImpl", "erviceImpl") for line in lines)is technically ineffective. Because theprestotable format includes column dividers (e.g.,|), a wrapped line would look something likeerviceImpl | |, meaningline.strip()would never exactly match the literal strings in the tuple.Furthermore, the explicit line-count assertion earlier (
len(lines) == 2 + len(rows)) already perfectly guarantees that no extra physical lines were generated. You can safely remove this check.♻️ Proposed fix
- # The class name is truncated, not present in full, and no wrap-spillover fragment. + # The class name is truncated, not present in full. assert LONG_CLASS_NAME not in output assert "…" in output - assert not any(line.strip() in ("ServiceImpl", "erviceImpl") for line in lines)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_blocks.py` around lines 159 - 162, Remove the ineffective any-based wrap-spillover assertion from the test around LONG_CLASS_NAME. Keep the existing LONG_CLASS_NAME, ellipsis, and line-count assertions unchanged, since the line-count check already verifies that no extra physical lines are produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_blocks.py`:
- Around line 159-162: Remove the ineffective any-based wrap-spillover assertion
from the test around LONG_CLASS_NAME. Keep the existing LONG_CLASS_NAME,
ellipsis, and line-count assertions unchanged, since the line-count check
already verifies that no extra physical lines are produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80dc6ee8-bc69-449e-8dbe-2737c81ce6ae
📒 Files selected for processing (2)
src/robusta/core/reporting/blocks.pytests/test_blocks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/core/reporting/blocks.py
alonelish
changed the title
Drop the Slack-digest-specific "Fired/Resolved" example, since blocks.py is core code shared across all sinks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer feedback: the full alert path must stay visible, so revert the cell-level ellipsis truncation and let long values wrap as before. Instead of cutting values, guard the real breakage: - to_markdown now renders within a char budget and drops WHOLE rows, appending "... N more rows not shown". The previous line-based trim cut mid-wrapped-row, leaving a value's continuation behind and presenting a partial name as if it were complete. - The budget always stays under BLOCK_SIZE_LIMIT, so MarkdownBlock can no longer blind-cut the text and lose the closing ``` fence - which is what made the large digest render as unformatted, non-monospace text. - Column widths are computed once from all rows so dropping rows doesn't reflow the table. to_table_string keeps its previous wrapping behaviour; only column width allocation changed (numeric columns are never shrunk, so counters don't wrap, plus the headerless/ragged-row fix). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/robusta/core/reporting/blocks.py (1)
389-418: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccount for presto’s separator and padding width in
__calc_max_width.
prestorenders column contents plus framing: separators, padding, and the separator row. Withtable_max_width=27, the returned widths sum to 27, but the rendered physical lines reach 52 characters. Track the rendered/separator row width instead of only content widths, and add a regression asserting every rendered line fits the requested limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/robusta/core/reporting/blocks.py` around lines 389 - 418, Update __calc_max_width to budget for presto’s full rendered table width, including column padding, separators, and the separator row, rather than comparing only the sum of content widths to table_max_width. Adjust the shrink calculation and stopping condition so every physical line produced by presto remains within the requested limit, while preserving numeric-column and minimum-width behavior. Add a regression test asserting that all rendered lines respect the configured maximum width.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/robusta/core/reporting/blocks.py`:
- Around line 491-499: The table rendering flow around __render_within_budget
must keep the full MarkdownBlock within max_chars and BLOCK_SIZE_LIMIT. Trim or
validate the prefix before calculating the content budget, reserve the suffix
accordingly, and when the remaining content budget is non-positive return no
content rather than adding an overflow marker; preserve the closing fence and
existing normal rendering behavior.
---
Outside diff comments:
In `@src/robusta/core/reporting/blocks.py`:
- Around line 389-418: Update __calc_max_width to budget for presto’s full
rendered table width, including column padding, separators, and the separator
row, rather than comparing only the sum of content widths to table_max_width.
Adjust the shrink calculation and stopping condition so every physical line
produced by presto remains within the requested limit, while preserving
numeric-column and minimum-width behavior. Add a regression test asserting that
all rendered lines respect the configured maximum width.
🪄 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 Plus
Run ID: 0ab78349-2fce-4d5e-8b4d-8f385c983ef0
📒 Files selected for processing (2)
src/robusta/core/reporting/blocks.pytests/test_blocks.py
| # Stay under BLOCK_SIZE_LIMIT even when no caller-supplied limit: MarkdownBlock | ||
| # would otherwise blindly cut the text and lose the closing ``` fence, which | ||
| # makes the whole table render as unformatted (non-monospace) text. | ||
| budget = BLOCK_SIZE_LIMIT - 1 if max_chars is None else min(max_chars, BLOCK_SIZE_LIMIT - 1) | ||
| table_contents = self.__render_within_budget( | ||
| budget - len(prefix) - len(suffix), PRINTED_TABLE_MAX_WIDTH, "presto" | ||
| ) | ||
|
|
||
| return MarkdownBlock(f"{prefix}{table_contents}{suffix}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the prefix inside the requested budget.
Line 494 caps the total budget before reserving table_name and the fences. A long table name—or max_chars smaller than the fence envelope—passes a negative inner budget; __trim_rows() then returns "\n...", and the final Markdown exceeds the limit. With the default limit, this can still trigger the blind truncation and lost closing fence described here. Trim/validate the prefix first, and handle a non-positive content budget without adding an overflow marker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/robusta/core/reporting/blocks.py` around lines 491 - 499, The table
rendering flow around __render_within_budget must keep the full MarkdownBlock
within max_chars and BLOCK_SIZE_LIMIT. Trim or validate the prefix before
calculating the content budget, reserve the suffix accordingly, and when the
remaining content budget is non-positive return no content rather than adding an
overflow marker; preserve the closing fence and existing normal rendering
behavior.
1 participant
What & why
Jira ROB-3946 / Linear FRO-211
The Slack "Alerts Summary - N Notifications" grouping digest rendered a corrupted ASCII table. When the
label:sitecolumn held long Java class names (e.g.ats.betting.betcatcher.settlement.settler.AbstractBetSettler), each value was word-wrapped across multiple physical lines instead of being truncated — mangling every row (ServiceImpl/erviceImplspilling onto the next line).Root cause
TableBlock.to_table_string()passedmaxcolwidths=col_max_widthtotabulate, and tabulate wraps over-wide cells onto extra physical lines rather than truncating them. The digest is built insend_or_update_summary_message()→TableBlock(...)→to_markdown()→to_table_string(). The bug title says "trimmed" but the code wrapped.The fix (
src/robusta/core/reporting/blocks.py)to_table_string()now truncates over-wide cells itself with a single-char…ellipsis and no longer passesmaxcolwidthsto tabulate, so every row stays on one physical line.…settler.AbstractBetSettler), since the class name is at the end; plain text (with spaces) is trimmed from the right.…(1 char) is used over...(3 chars) so width accounting stays exact.__calc_max_widthnow detects columns whose non-empty cells are all numeric (theFired/Resolvedcounters) and leaves them at full width; the overflow is water-filled across the widest text columns (fair distribution + a minimum-width floor) instead of gutting a single column.Tables that already fit render byte-identical to before.
Before / after (real prod-az2 screenshot data)
Before — wrapped:
After — single line, suffix preserved:
Tests
tests/test_blocks.pyproving rows stay single-line and over-wide cells are truncated (not wrapped): line-count invariant,…truncation, dotted-name suffix retention, numeric columns never truncated, and trailing-cut for plain text.test_transformer.pytable/HTML tests still pass (the HTML path usestabulatedirectly and is unaffected).Manual verification in Slack
The change is centralized in
to_table_string, so it affects every sink that renders aTableBlock(Slack, Discord, Google Chat, Zulip, PagerDuty, incident.io, …). To exercise the exact digest path end-to-end, run the manual repro (not part of this PR unless requested):🤖 Generated with Claude Code