feat: add --slack-full-width for full-width Slack alerts with markdown test result tables#2107
feat: add --slack-full-width for full-width Slack alerts with markdown test result tables#2107michrzan wants to merge 11 commits intoelementary-data:masterfrom
Conversation
|
👋 @michrzan |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces full-width Slack alert support with markdown table formatting for test result samples. It adds a new Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Monitor Command
participant Config as Config
participant Integrations as Integrations
participant SlackIntegration as SlackIntegration
participant MessageBuilder as SlackAlertMessageBuilder
participant SlackMessage as Slack Message
CLI->>Config: Initialize with slack_full_width flag
Config->>Config: Store slack_full_width attribute
CLI->>Integrations: Create with config
Integrations->>Integrations: Check slack_full_width condition
Integrations->>SlackIntegration: Instantiate (selected via slack_full_width)
SlackIntegration->>MessageBuilder: Initialize with config.slack_full_width
MessageBuilder->>MessageBuilder: Store full_width flag
MessageBuilder->>SlackMessage: Build message structure
alt full_width enabled
MessageBuilder->>SlackMessage: Place preview/details in main blocks
MessageBuilder->>SlackMessage: Clear attachments
else full_width disabled
MessageBuilder->>SlackMessage: Use attachment path (legacy)
end
SlackMessage-->>CLI: Formatted Slack alert
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 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 |
michrzan
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/oss/deployment-and-configuration/slack.mdx`:
- Around line 56-67: Change the lowercase "markdown" occurrences to the proper
noun "Markdown" in the Slack docs section that describes full-width alerts (the
paragraph referencing `--slack-full-width` and the sentence about test result
samples). Locate the two instances where "markdown table" and "markdown tables"
are used and update them to "Markdown table" and "Markdown tables" respectively
so the term is capitalized consistently.
In `@docs/oss/guides/alerts/send-slack-alerts.mdx`:
- Around line 27-28: Replace the lowercase word "markdown" with capitalized
"Markdown" in the sentence that mentions `--slack-full-width` so the doc reads
"show test results as Markdown tables"; update the phrase near the
`--slack-full-width` reference to use the corrected capitalization.
In `@elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py`:
- Around line 225-234: The markdown table from list_of_dicts_to_markdown_table
can be truncated mid-row by get_limited_markdown_msg causing malformed markdown;
update the Slack message construction in slack.py (the code that calls
create_text_section_block with the code-fenced test_rows_sample) to first check
the full table length/character limit using the same logic as the test-query
branch, and if it exceeds the limit, either (a) render a safe summary by
converting only the first N rows (or columns) and append a clear "(truncated)"
note, or (b) replace the code block with a short text section that says the
table was truncated and provide row/column counts; ensure this uses
list_of_dicts_to_markdown_table for the sampled subset and then pass the safe
string to create_text_section_block/get_limited_markdown_msg so no markdown is
cut mid-structure.
🧹 Nitpick comments (4)
elementary/utils/json_utils.py (1)
101-112: Consider precision loss for very small decimal floats.The fixed
.10fformat means values with magnitude smaller than ~1e-10 will be rounded to zero. For example,0.00000000001would render as"0.0". If test result data could contain very small non-zero decimals, this might silently hide meaningful values.If this is acceptable for the display use case, no change needed — just flagging the edge case.
tests/unit/monitor/data_monitoring/alerts/integrations/slack/test_slack_alert_message_builder.py (1)
199-220: Consider using an exact assertion for block count.Line 213:
assert len(blocks) >= 5— given the schema has exactly one title block, one preview block, and one detail block, the expected count is deterministically 5 (rich_text + title + divider + preview + detail). Using== 5would make the test stricter and catch unexpected extra blocks.Proposed fix
- assert len(blocks) >= 5 + assert len(blocks) == 5elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py (1)
38-46: The emptyrich_textblock hack relies on undocumented Slack behavior.The empty
rich_textblock to force full-width rendering is a known workaround, but it's not part of the official Slack Block Kit API contract. Slack could change this behavior without notice. The inline comment is helpful — consider also adding a link to any community discussion or internal reference documenting this technique, so future maintainers understand the fragility.elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py (1)
199-205: Inconsistent description rendering between dbt and elementary test templates.
_get_dbt_test_templatenow uses a simplified single-block approach (description_text = alert.test_description or "_No description_"), while_get_elementary_test_template(lines 363–378) still uses the old if/else with separatecreate_text_section_block+create_context_blockfor descriptions. Consider aligning both templates for consistency — unless the difference is intentional due to rendering requirements.
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py (1)
56-63: Full-width mode bypasses preview validation, allowing unbounded preview blocks.Line 58-59: When
full_width=True, preview blocks bypass_validate_preview_blocks(), soPreviewIsTooLongErroris never raised. This was intentional design (blocks go directly to main message instead of attachments), but it removes the safety bound that prevents oversized payloads. Consider adding validation even for full-width mode to maintain consistency and prevent Slack message size issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py` around lines 56 - 63, The full-width branch bypasses validation, so modify the preview handling in the method containing full_width to always run _validate_preview_blocks(preview_blocks) (or call a new helper that enforces the same bounds) before passing blocks on; if validation raises PreviewIsTooLongError, handle it the same way as the non-full-width branch (e.g., trim/fallback to _add_blocks_as_attachments or log/raise consistently) and then call _add_always_displayed_blocks(validated_preview_blocks) when full_width is True, otherwise keep the existing _add_blocks_as_attachments(validated_preview_blocks) flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py`:
- Around line 37-45: The empty rich_text block sent when self.full_width is True
is invalid; instead of calling self._add_always_displayed_blocks([{"type":
"rich_text", "elements": []}]) remove that empty block usage and implement a
valid alternative: either remove attachments early (set
self.slack_message["attachments"] = []) before building blocks or add a valid
non-empty rich_text block (with required nested content) if you must use
rich_text; update places that rely on the sentinel (self.full_width,
add_title_to_slack_alert, add_preview_to_slack_alert,
add_details_to_slack_alert) so they work with the new approach and do not append
an empty elements array to slack_message.
---
Nitpick comments:
In
`@elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py`:
- Around line 56-63: The full-width branch bypasses validation, so modify the
preview handling in the method containing full_width to always run
_validate_preview_blocks(preview_blocks) (or call a new helper that enforces the
same bounds) before passing blocks on; if validation raises
PreviewIsTooLongError, handle it the same way as the non-full-width branch
(e.g., trim/fallback to _add_blocks_as_attachments or log/raise consistently)
and then call _add_always_displayed_blocks(validated_preview_blocks) when
full_width is True, otherwise keep the existing
_add_blocks_as_attachments(validated_preview_blocks) flow.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
elementary/messages/formats/block_kit.pyelementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elementary/messages/formats/block_kit.py
elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py
Show resolved
Hide resolved
- Replace invalid empty rich_text block with valid non-empty structure (Slack API rejects empty elements arrays) - Add max_length param to list_of_dicts_to_markdown_table for graceful row-by-row truncation instead of cutting mid-row - Capitalize "markdown" to "Markdown" in docs (proper noun) - Run preview validation in full-width mode for consistent safety bounds - Update tests for valid rich_text block and exact assertions Made-with: Cursor
haritamar
temporarily deployed
to
elementary_test_env
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/utils/json_utils.py`:
- Around line 141-143: The truncation logic can produce outputs longer than
max_length when max_length is smaller than the truncation marker length; update
the code around truncation_note/effective_max/processed_data so effective_max =
max(0, max_length - len(truncation_note)) and ensure any returned string is
trimmed to max_length (for example, if effective_max == 0 return
truncation_note[:max_length]); also ensure the loop that reduces rows (uses
processed_data and row_count) handles the zero-effective_max case and that the
final concatenation always enforces the max_length limit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
docs/oss/deployment-and-configuration/slack.mdxdocs/oss/guides/alerts/send-slack-alerts.mdxelementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.pyelementary/monitor/data_monitoring/alerts/integrations/slack/slack.pyelementary/utils/json_utils.pytests/unit/monitor/data_monitoring/alerts/integrations/slack/test_slack_alert_message_builder.py
🚧 Files skipped from review as they are similar to previous changes (2)
- elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py
- docs/oss/deployment-and-configuration/slack.mdx
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
Docs for --slack-full-width are handled in a separate PR targeting the docs branch. Made-with: Cursor
haritamar
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
Handle edge cases where max_length is <= 0 or smaller than the truncation note itself. Made-with: Cursor
2 participants
Summary
This PR adds a --slack-full-width option that allows Slack alerts to use the full message width by rendering the entire message with Block Kit (no attachments) and displaying test result samples as markdown tables.
This significantly improves readability when test results contain many columns or long values.
Problem
When the Test Results Sample includes multiple columns or long strings, the current Slack alerts become difficult to read due to the narrow attachment layout. In practice, this makes it hard for alert recipients to quickly understand the context and take action.
or when there is more columns like this:
Solution
With the --slack-full-width option enabled, Slack alerts are rendered at full width and test results are consistently shown as markdown tables:
or when there is more columns like this:
Admittedly, it looks better on a wide screen.
Implementation details
This is achieved by:
Trade-offs
As a result of moving away from attachments, the colored severity indicator (yellow/red pipe on the left) is no longer available. This is a known limitation of the approach.
Notes
This resolves #1079.
This is jsut something I am using internally and wanted to share. Happy to discuss alternatives, trade-offs, or refinements!
Summary by CodeRabbit
New Features
--slack-full-widthCLI flag to the monitor command for controlling Slack alert layout width.Improvements
Tests