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 10 commits intoelementary-data:masterfrom
Conversation
|
👋 @michrzan |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a slack_full_width option (CLI → Config) and full-width Slack rendering path; converts test result samples to markdown tables via a new utility; disables tabulate numeric parsing across formatters; updates Slack message builder and integration selection to support full-width block rendering. 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
Config->>Config: store slack_full_width
CLI->>Integrations: create Integrations(config)
Integrations->>Integrations: evaluate config.slack_full_width
Integrations->>SlackIntegration: instantiate SlackIntegration
SlackIntegration->>MessageBuilder: init(full_width=config.slack_full_width)
MessageBuilder->>MessageBuilder: build blocks and attachments
alt full_width enabled
MessageBuilder->>SlackMessage: place preview/details in top-level blocks
MessageBuilder->>SlackMessage: clear attachments
else full_width disabled
MessageBuilder->>SlackMessage: add preview/details as attachments
end
SlackMessage-->>CLI: return formatted Slack payload
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
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
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
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
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
haritamar
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
- 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
Docs for --slack-full-width are handled in a separate PR targeting the docs branch. Made-with: Cursor
Handle edge cases where max_length is <= 0 or smaller than the truncation note itself. Made-with: Cursor
michrzan
force-pushed
the
slack-rick-text-refactor
branch
from
5500f28 to
5446b6b
Compare
michrzan
requested a deployment
to
elementary_test_env
GitHub Actions
Waiting
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
`@elementary/monitor/data_monitoring/alerts/integrations/slack/message_builder.py`:
- Around line 69-75: The preview block validation (_validate_preview_blocks) is
being called unconditionally before branching on full_width, which enforces the
attachment-era 5-block cap and padding even for top-level blocks; change the
logic so that when self.full_width is True you skip calling
_validate_preview_blocks and directly call
_add_always_displayed_blocks(preview_blocks), and only call
_validate_preview_blocks(preview_blocks) when taking the else branch that calls
_add_blocks_as_attachments(validated_preview_blocks); remove or relocate the
earlier unconditional call to _validate_preview_blocks to avoid raising
PreviewIsTooLongError or adding padding for full-width previews.
In `@elementary/utils/json_utils.py`:
- Around line 105-111: The float formatting collapses tiny non-zero values to
"0" by using f"{value:.10f}"; in elementary/utils/json_utils.py update the float
branch (the block that currently checks isinstance(value, float) and uses
f"{value:.10f}") to format with higher-precision general format (e.g.,
format(value, ".17g")) so very small values become "1e-12" instead of "0", and
ensure you normalize exact zero to "0" (avoid "-0" by checking value == 0.0);
also keep the existing int-like conversion (value == int(value) and abs(value) <
1e15) behavior and add regression tests covering small floats (1e-11, 1e-12,
-1e-12) for the code path used by test_rows_sample Slack alert generation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48a36b92-e5af-446d-803c-55ead20caccd
📒 Files selected for processing (11)
elementary/config/config.pyelementary/messages/formats/block_kit.pyelementary/messages/formats/markdown.pyelementary/messages/formats/text.pyelementary/monitor/cli.pyelementary/monitor/data_monitoring/alerts/integrations/integrations.pyelementary/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.pytests/unit/utils/test_json_utils.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/utils/test_json_utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- elementary/messages/formats/markdown.py
- elementary/monitor/data_monitoring/alerts/integrations/integrations.py
- elementary/messages/formats/text.py
- elementary/monitor/cli.py
| if not preview_blocks: | ||
| return | ||
| validated_preview_blocks = self._validate_preview_blocks(preview_blocks) | ||
| if self.full_width: | ||
| self._add_always_displayed_blocks(validated_preview_blocks) | ||
| else: | ||
| self._add_blocks_as_attachments(validated_preview_blocks) |
There was a problem hiding this comment.
Skip attachment-era preview validation in full_width mode.
Line 71 still runs _validate_preview_blocks() before the full_width branch. That keeps the old 5-block cap and adds padding blocks even when the preview is rendered in top-level blocks, so full-width alerts can still raise PreviewIsTooLongError and end up with extra blank space.
Suggested fix
if not preview_blocks:
return
- validated_preview_blocks = self._validate_preview_blocks(preview_blocks)
if self.full_width:
- self._add_always_displayed_blocks(validated_preview_blocks)
+ self._add_always_displayed_blocks(preview_blocks)
else:
+ validated_preview_blocks = self._validate_preview_blocks(preview_blocks)
self._add_blocks_as_attachments(validated_preview_blocks)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not preview_blocks: | |
| return | |
| validated_preview_blocks = self._validate_preview_blocks(preview_blocks) | |
| if self.full_width: | |
| self._add_always_displayed_blocks(validated_preview_blocks) | |
| else: | |
| self._add_blocks_as_attachments(validated_preview_blocks) | |
| if not preview_blocks: | |
| return | |
| if self.full_width: | |
| self._add_always_displayed_blocks(preview_blocks) | |
| else: | |
| validated_preview_blocks = self._validate_preview_blocks(preview_blocks) | |
| self._add_blocks_as_attachments(validated_preview_blocks) |
🤖 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 69 - 75, The preview block validation (_validate_preview_blocks) is
being called unconditionally before branching on full_width, which enforces the
attachment-era 5-block cap and padding even for top-level blocks; change the
logic so that when self.full_width is True you skip calling
_validate_preview_blocks and directly call
_add_always_displayed_blocks(preview_blocks), and only call
_validate_preview_blocks(preview_blocks) when taking the else branch that calls
_add_blocks_as_attachments(validated_preview_blocks); remove or relocate the
earlier unconditional call to _validate_preview_blocks to avoid raising
PreviewIsTooLongError or adding padding for full-width previews.
| if isinstance(value, float): | ||
| if math.isinf(value) or math.isnan(value): | ||
| return str(value) | ||
| # Format floats without scientific notation | ||
| if value == int(value) and abs(value) < 1e15: | ||
| return str(int(value)) | ||
| return f"{value:.10f}".rstrip("0").rstrip(".") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
for value in [1e-11, 1e-12, -1e-12]:
rendered = f"{value:.10f}".rstrip("0").rstrip(".")
print(f"{value!r} -> {rendered!r}")
PYRepository: elementary-data/elementary
Length of output: 110
🏁 Script executed:
# First, check if the file exists and inspect the relevant lines
find . -name "json_utils.py" -type fRepository: elementary-data/elementary
Length of output: 102
🏁 Script executed:
# Check the file size to understand scope
wc -l elementary/utils/json_utils.py 2>/dev/null || echo "File not found in expected location"Repository: elementary-data/elementary
Length of output: 104
🏁 Script executed:
# Search for the _format_value function to understand context
rg "_format_value" --type py -B 3 -A 15Repository: elementary-data/elementary
Length of output: 2498
🏁 Script executed:
# Search for uses of list_of_dicts_to_markdown_table to understand impact
rg "list_of_dicts_to_markdown_table|_format_value" --type pyRepository: elementary-data/elementary
Length of output: 2001
🏁 Script executed:
# Search for test files related to json_utils
fd -e "test*.py" -o -name "*test.py" | xargs grep -l "json_utils\|_format_value\|markdown_table" 2>/dev/null || echo "No matching tests found"Repository: elementary-data/elementary
Length of output: 341
🏁 Script executed:
# Check if there are any references to this in Slack alert or table rendering code
rg "Slack|alert|markdown|table" elementary/utils/json_utils.py -A 2 -B 2Repository: elementary-data/elementary
Length of output: 1823
🏁 Script executed:
# Look at the actual test file to see what float cases are tested
cat -n tests/unit/utils/test_json_utils.pyRepository: elementary-data/elementary
Length of output: 2574
🏁 Script executed:
# Run the existing tests to see if they pass
grep -A 20 "test_list_of_dicts_to_markdown_table_float" tests/unit/utils/test_json_utils.pyRepository: elementary-data/elementary
Length of output: 964
🏁 Script executed:
# Verify the impact by checking where list_of_dicts_to_markdown_table is used in the Slack integration
rg "test_rows_sample_table" elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py -B 5 -A 5Repository: elementary-data/elementary
Length of output: 857
🏁 Script executed:
# Check if there are any recent fixes or discussions about small float handling
rg "1e-1[0-2]|scientific|notation" elementary/utils/json_utils.pyRepository: elementary-data/elementary
Length of output: 206
Small non-zero floats collapse to "0" in Slack alert tables.
Line 111 uses f"{value:.10f}" which rounds values smaller than 1e-10 to zero. For example, 1e-12 becomes "0" and -1e-12 becomes "-0". This affects test_rows_sample tables in Slack alerts, causing incorrect data to be displayed to users.
The existing test suite does not cover this edge case. Add a regression test for small float values (e.g., 1e-11, 1e-12) to prevent future regressions.
Possible fix direction
+from decimal import Decimal
...
if isinstance(value, float):
if math.isinf(value) or math.isnan(value):
return str(value)
- # Format floats without scientific notation
- if value == int(value) and abs(value) < 1e15:
- return str(int(value))
- return f"{value:.10f}".rstrip("0").rstrip(".")
+ normalized = format(Decimal(str(value)), "f").rstrip("0").rstrip(".")
+ return "0" if normalized in {"", "-0"} else normalized📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(value, float): | |
| if math.isinf(value) or math.isnan(value): | |
| return str(value) | |
| # Format floats without scientific notation | |
| if value == int(value) and abs(value) < 1e15: | |
| return str(int(value)) | |
| return f"{value:.10f}".rstrip("0").rstrip(".") | |
| if isinstance(value, float): | |
| if math.isinf(value) or math.isnan(value): | |
| return str(value) | |
| normalized = format(Decimal(str(value)), "f").rstrip("0").rstrip(".") | |
| return "0" if normalized in {"", "-0"} else normalized |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@elementary/utils/json_utils.py` around lines 105 - 111, The float formatting
collapses tiny non-zero values to "0" by using f"{value:.10f}"; in
elementary/utils/json_utils.py update the float branch (the block that currently
checks isinstance(value, float) and uses f"{value:.10f}") to format with
higher-precision general format (e.g., format(value, ".17g")) so very small
values become "1e-12" instead of "0", and ensure you normalize exact zero to "0"
(avoid "-0" by checking value == 0.0); also keep the existing int-like
conversion (value == int(value) and abs(value) < 1e15) behavior and add
regression tests covering small floats (1e-11, 1e-12, -1e-12) for the code path
used by test_rows_sample Slack alert generation.
|
Thanks for the commits @haritamar I rebased the branch to master. Tested locally and all seems to be working fine. I also aligned |
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
Improvements
Tests