-
-
Notifications
You must be signed in to change notification settings - Fork 149
fix: list alert summary by user in home api #1431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: list alert summary by user in home api #1431
Conversation
WalkthroughShifted alerts summary to be user-scoped by accepting a SessionKey and using list_alerts_for_user. Updated iteration to use AlertConfig fields instead of trait methods. Adjusted prism home to pass SessionKey. Minor formatting change in dashboards without logic change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Home as prism::home::generate_home_response
participant Alerts as alerts::get_alerts_summary
participant Store as list_alerts_for_user
User->>Home: Request home (with SessionKey)
Note over Home: Concurrently fetch streams and alerts
Home->>Alerts: get_alerts_summary(SessionKey)
Alerts->>Store: list_alerts_for_user(SessionKey)
Store-->>Alerts: Vec<AlertConfig>
Alerts->>Alerts: Aggregate by state, select top-5 per state
Alerts-->>Home: AlertsSummary
Home-->>User: Home response with alerts summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/alerts/mod.rs (1)
1338-1345
: Avoid holding ALERTS read-lock across an await.You await list_alerts_for_user while a read guard on ALERTS is live. Clone the Arc out, drop the guard, then await to prevent potential writer starvation/deadlocks.
Apply:
-pub async fn get_alerts_summary(key: &SessionKey) -> Result<AlertsSummary, AlertError> { - let guard = ALERTS.read().await; - let alerts = if let Some(alerts) = guard.as_ref() { - alerts.list_alerts_for_user(key.clone(), vec![]).await? - } else { - return Err(AlertError::CustomError("No AlertManager registered".into())); - }; +pub async fn get_alerts_summary(key: &SessionKey) -> Result<AlertsSummary, AlertError> { + let manager = { + let guard = ALERTS.read().await; + guard + .as_ref() + .cloned() + .ok_or_else(|| AlertError::CustomError("No AlertManager registered".into()))? + }; + let alerts = manager + .list_alerts_for_user(key.clone(), Vec::new()) + .await?;
🧹 Nitpick comments (1)
src/alerts/mod.rs (1)
1355-1384
: Avoid clones by consuming alerts.alerts is owned; iterate with into_iter() and move fields to AlertsInfo to drop String/enum clones.
- for alert in alerts.iter() { - match alert.state { + for alert in alerts.into_iter() { + match alert.state { AlertState::Triggered => { triggered += 1; triggered_alerts.push(AlertsInfo { - title: alert.title.clone(), - id: alert.id, - severity: alert.severity.clone(), + title: alert.title, + id: alert.id, + severity: alert.severity, }); } AlertState::Disabled => { disabled += 1; disabled_alerts.push(AlertsInfo { - title: alert.title.clone(), - id: alert.id, - severity: alert.severity.clone(), + title: alert.title, + id: alert.id, + severity: alert.severity, }); } AlertState::NotTriggered => { not_triggered += 1; not_triggered_alerts.push(AlertsInfo { - title: alert.title.clone(), - id: alert.id, - severity: alert.severity.clone(), + title: alert.title, + id: alert.id, + severity: alert.severity, }); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/alerts/mod.rs
(2 hunks)src/prism/home/mod.rs
(1 hunks)src/users/dashboards.rs
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-01T10:27:56.858Z
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:0-0
Timestamp: 2025-05-01T10:27:56.858Z
Learning: The `add_tile()` function in `src/handlers/http/users/dashboards.rs` should use `get_dashboard_by_user(dashboard_id, &user_id)` instead of `get_dashboard(dashboard_id)` to ensure proper authorization checks when modifying a dashboard.
Applied to files:
src/users/dashboards.rs
🧬 Code graph analysis (2)
src/alerts/mod.rs (1)
src/handlers/http/alerts.rs (1)
alerts
(98-101)
src/prism/home/mod.rs (1)
src/alerts/mod.rs (1)
get_alerts_summary
(1338-1412)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
🔇 Additional comments (3)
src/users/dashboards.rs (1)
299-301
: LGTM: ownership check call is clearer and unchanged semantically.src/alerts/mod.rs (1)
1338-1338
: Confirmed: call sites updated to &SessionKey.
Only usage found: src/prism/home/mod.rs:106 (tokio::join!(…, get_alerts_summary(key))) and it matches the signature in src/alerts/mod.rs:1338 (pub async fn get_alerts_summary(key: &SessionKey)). No legacy zero-arg invocations found.src/prism/home/mod.rs (1)
106-107
: LGTM: concurrent fetch aligns with user-scoped alerts.Passing key into get_alerts_summary correctly wires per-user filtering.
Summary by CodeRabbit
New Features
Refactor