FRO-211 / ROB-3946 Truncate over-wide summary table cells instead of wrapping#2123
Open
alonelish wants to merge 3 commits into
Open
FRO-211 / ROB-3946 Truncate over-wide summary table cells instead of wrapping#2123alonelish wants to merge 3 commits into
alonelish wants to merge 3 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:476f656
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:476f656 me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:476f656
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:476f656Patch 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:476f656 |
|
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)
Walkthrough
ChangesTable truncation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TableBlock
participant WidthCalculation
participant CellTruncation
participant tabulate
TableBlock->>WidthCalculation: calculate bounded numeric-aware widths
WidthCalculation->>CellTruncation: apply per-column limits
CellTruncation-->>TableBlock: return truncated headers and cells
TableBlock->>tabulate: render pre-truncated table
tabulate-->>TableBlock: return aligned table output
🚥 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>
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
1 participant
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.
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