feat(api-web): support exact health-alert filtering on managed-host page#3489
Open
RajMandaliya wants to merge 5 commits into
Open
feat(api-web): support exact health-alert filtering on managed-host page#3489RajMandaliya wants to merge 5 commits into
RajMandaliya wants to merge 5 commits into
Conversation
Keeping Pace with original repo
The existing health-alerts-filter only supported a binary healthy/
unhealthy toggle, checking whether health_probe_alerts was empty or
not -- it never matched against a specific alert ID. Extends the
filter to also support exact matching against HealthProbeAlert.id,
with the dropdown dynamically populated from every distinct alert ID
currently active across machines (same pattern already used for the
vendor/model/GPU filters).
Also adds a clear empty-state message ('No machines match the
selected filters.') for the zero-results case, which previously
rendered an empty table with no explanation for any filter.
Addresses NVIDIA#3429.
Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe managed-host view now discovers available health alert IDs, filters hosts by matching alert IDs, renders dynamic filter options, and displays an explicit empty state when no hosts match. ChangesManaged host health-alert filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant show_html
participant managed_host_show_html
Browser->>show_html: request health-alerts-filter
show_html->>show_html: collect alert IDs and filter hosts
show_html->>managed_host_show_html: render hosts and dynamic alert IDs
managed_host_show_html-->>Browser: filtered table and filter options
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@crates/api-web/src/tests/managed_host.rs`:
- Around line 377-399: The managed-host filter test should also verify that the
dynamic health-alert option is rendered and selected. In the test around the
response body assertions, confirm that the body contains the
IntrusionSensorTriggered option with its selected state, while preserving the
existing host-row assertions.
In `@crates/api-web/templates/managed_host_individual_table.html`:
- Around line 25-27: Update the empty-state row in the hosts table, using the
surrounding maintenance-mode condition, to render colspan="11" in maintenance
mode and colspan="12" otherwise. Replace the current fixed colspan="15" while
preserving the existing no-hosts message and row structure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 944d2729-bf88-45b4-bc80-e085acb031c0
📒 Files selected for processing (4)
crates/api-web/src/managed_host.rscrates/api-web/src/tests/managed_host.rscrates/api-web/templates/managed_host_individual_table.htmlcrates/api-web/templates/managed_host_show.html
- Match empty-state colspan to actual table column count (12 normally, 11 in maintenance mode) instead of a fixed 15. - Strengthen the exact-filter test to assert the dynamic alert ID actually renders as the selected dropdown option, not just that filtering excludes/includes the right hosts. Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.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.
Addresses #3429.
Problem
The existing
health-alerts-filteron the managed-host page onlysupports a binary toggle — "healthy" / "unhealthy" — checking whether
health_probe_alertsis empty or not. It never matches against aspecific alert. Operators affected by the same alert (e.g. a specific
BMC or sensor issue) have to manually inspect each host individually to
find others affected by it.
Fix
HealthProbeAlert.id.alert ID currently active across machines, following the same
pattern already used for the vendor/model/GPU filters.
?health-alerts-filter=...)required no changes — it already worked generically.
the zero-results case, which previously rendered an empty table with
no explanation, for any filter.
Testing
only the host with the matching alert (not a healthy host), and one
confirming the empty-state message renders when nothing matches.
carbide-api-webtest suite passes (39 tests), including aftermerging in a large upstream
mainupdate (482 files) to make surenothing regressed.