update dashboard implementation #1348
Conversation
WalkthroughThis update refactors and simplifies the dashboard management system. It renames HTTP handler functions for clarity, centralizes dashboard creation and updates, adds an endpoint for adding tiles, and enforces ownership checks. The dashboard data model is streamlined, legacy migration code is removed, and error handling is improved for loading and modifying dashboards. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTP_Handler
participant Dashboards_Store
User->>HTTP_Handler: POST /dashboards (create_dashboard)
HTTP_Handler->>Dashboards_Store: create(user_id, dashboard)
Dashboards_Store-->>HTTP_Handler: Result
HTTP_Handler-->>User: Response
User->>HTTP_Handler: PUT /dashboards/{id}/add_tile (add_tile)
HTTP_Handler->>Dashboards_Store: get_dashboard_by_user(dashboard_id, user_id)
Dashboards_Store-->>HTTP_Handler: Dashboard or Error
HTTP_Handler->>Dashboards_Store: update(user_id, dashboard_id, dashboard_with_new_tile)
Dashboards_Store-->>HTTP_Handler: Result
HTTP_Handler-->>User: Response
User->>HTTP_Handler: GET /dashboards (list_dashboards)
HTTP_Handler->>Dashboards_Store: list_dashboards()
Dashboards_Store-->>HTTP_Handler: Dashboard summaries
HTTP_Handler-->>User: Response
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/users/dashboards.rs (4)🧬 Code Graph Analysis (1)src/users/dashboards.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
src/handlers/http/modal/server.rs (1)
275-278: Permission looks offUpdating a dashboard is currently guarded by
Action::CreateDashboard.
Double-check whether a dedicatedUpdateDashboard/PutDashboardaction should be used here to avoid over-permissive RBAC rules.src/handlers/http/users/dashboards.rs (2)
292-301: Return type ofensure_dashboard_ownershipis unused
ensure_dashboard_ownershipreturns aDashboard, but callers only rely on theResultside-effect. Returning()avoids an unnecessary clone:-async fn ensure_dashboard_ownership(...) -> Result<Dashboard, DashboardError> { +async fn ensure_dashboard_ownership(...) -> Result<(), DashboardError> { self.get_dashboard_by_user(dashboard_id, user_id) .await - .ok_or(DashboardError::Unauthorized) + .map(|_| ()) }
33-41: Pagination / large payload
list_dashboardsalways returns the full list. For organisations with hundreds of dashboards this may hurt latency. Consider adding pagination (limit/offset) or server-side filtering.src/users/dashboards.rs (2)
83-112:to_summaryomitsdashboard_typeSummaries currently expose
title/author/modified/dashboard_idbut notdashboard_type; Prism UI might need it to distinguish dashboards vs reports. Consider including it.
285-287: Clone-heavy list
list_dashboardsclones the entire vector on every call. For read-heavy scenarios, return anArc<Vec<_>>or iterate over a read lock to avoid frequent allocations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/handlers/http/modal/server.rs(1 hunks)src/handlers/http/users/dashboards.rs(3 hunks)src/prism/home/mod.rs(2 hunks)src/users/dashboards.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/handlers/http/modal/server.rs (1)
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.
src/handlers/http/users/dashboards.rs (1)
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.
🧬 Code Graph Analysis (1)
src/handlers/http/modal/server.rs (2)
src/handlers/http/users/dashboards.rs (7)
dashboards(35-38)create_dashboard(54-66)list_dashboards(33-41)get_dashboard(43-52)delete_dashboard(104-114)update_dashboard(68-102)add_tile(116-146)src/users/dashboards.rs (3)
list_dashboards(285-287)get_dashboard(250-261)delete_dashboard(228-246)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (1)
src/prism/home/mod.rs (1)
360-383: Missing authorization filtering
list_dashboards()returns every dashboard in memory; withoutSessionKeychecks this exposes dashboards the current user may not own or be allowed to view.
Consider filtering by ownership or adding RBAC verification similar to streams/alerts searches.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/users/dashboards.rs (2)
160-162: Possible duplicate‐ID leakage during successive loads
load()appends new dashboards to the in-memory vector without de-duplicating against items already present inself.0, leading to multiple entries with the samedashboard_idifload()is called more than once (e.g., in tests).- let mut s = self.0.write().await; - s.append(&mut this); + let mut s = self.0.write().await; + for d in this { + s.retain(|existing| existing.dashboard_id != d.dashboard_id); + s.push(d); + }This keeps the latest version while preventing duplicates.
285-287:list_dashboardsclones the entire vector – consider borrowing or streamingReturning
Vec<Dashboard>incurs an O(n) clone each call and scales poorly with many dashboards.
If external mutation is not required, expose an immutable slice or iterator to avoid the copy:pub async fn list_dashboards(&self) -> Vec<Dashboard> { self.0.read().await.clone() // current } // possible alternatives pub async fn list_dashboards(&self) -> Vec<DashboardSummary> { … } // lighter type // or pub async fn list_dashboards(&self) -> impl Iterator<Item = Dashboard> { … }Evaluate based on API ergonomics and performance expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/handlers/http/modal/server.rs(1 hunks)src/users/dashboards.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/http/modal/server.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
as per the new design in prism
nikhilsinhaparseable
force-pushed
the
new-dashboard
branch
from
72fdcb7 to
8831313
Compare
Signed-off-by: Nikhil Sinha <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/users/dashboards.rs (2)
69-78: Fix dashboard_type overwrite issue from previous reviewThe
set_metadatamethod still overwritesdashboard_typetoDashboard, ignoring any existing type likeReport.Apply this fix to preserve existing dashboard type:
- self.dashboard_type = Some(DashboardType::Dashboard); + // Preserve existing type if already set + if self.dashboard_type.is_none() { + self.dashboard_type = Some(DashboardType::Dashboard); + }
240-244: Fix compile error from nonexistent methodThe code uses
Option::is_none_orwhich doesn't exist in stable Rust, causing compilation failure.Apply this fix to use correct method:
- .retain(|d| d.dashboard_id.as_ref().is_none_or(|id| *id != dashboard_id)); + .retain(|d| d.dashboard_id.map_or(true, |id| id != dashboard_id));
🧹 Nitpick comments (1)
src/users/dashboards.rs (1)
175-176: Handle potential None dashboard_id correctlyBased on retrieved learnings, dashboard_id is guaranteed to be present after creation, but this error handling could be more descriptive.
Consider improving the error message to be more specific:
- .ok_or(DashboardError::Metadata("Dashboard ID must be provided"))?; + .ok_or(DashboardError::Metadata("Dashboard ID is missing - dashboard may not be properly initialized"))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/users/dashboards.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:63-71
Timestamp: 2025-05-01T10:22:45.695Z
Learning: Dashboard design in Parseable follows a sharing model where any user can read (list/get) any dashboard, enabling dashboard sharing functionality, but only the dashboard author can modify (update/delete) their own dashboards.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/users/dashboards.rs:154-165
Timestamp: 2025-05-01T12:22:42.363Z
Learning: Title validation for dashboards is performed in the `create_dashboard` HTTP handler function rather than in the `DASHBOARDS.create` method, avoiding redundant validation.
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:125-148
Timestamp: 2025-05-01T10:33:51.767Z
Learning: When adding a tile to a dashboard in `add_tile()` function, the tile ID must be provided by the client and should not be generated by the server. If the tile ID is missing (nil), the API should fail the operation with an appropriate error message.
src/users/dashboards.rs (4)
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.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/handlers/http/users/dashboards.rs:63-71
Timestamp: 2025-05-01T10:22:45.695Z
Learning: Dashboard design in Parseable follows a sharing model where any user can read (list/get) any dashboard, enabling dashboard sharing functionality, but only the dashboard author can modify (update/delete) their own dashboards.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1305
File: src/users/dashboards.rs:154-165
Timestamp: 2025-05-01T12:22:42.363Z
Learning: Title validation for dashboards is performed in the `create_dashboard` HTTP handler function rather than in the `DASHBOARDS.create` method, avoiding redundant validation.
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1348
File: src/prism/home/mod.rs:366-368
Timestamp: 2025-06-15T18:18:14.590Z
Learning: In the Parseable dashboard system, dashboard_id is guaranteed to always be present (never None) because IDs are generated at dashboard creation time, and there are no pre-v1 dashboards that could lack IDs.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (3)
src/users/dashboards.rs (3)
194-201: LGTM - Dashboard creation follows ownership patternThe
createmethod correctly implements the ownership model by setting metadata with the user_id and persisting to both storage and memory.
212-213: LGTM - Proper ownership verification before updatesThe ownership check ensures only dashboard authors can modify their dashboards, consistent with the sharing model from retrieved learnings.
298-301: LGTM - Clean ownership enforcement implementationThe
ensure_dashboard_ownershipmethod correctly implements the authorization pattern by checking both dashboard existence and author ownership.
as per the new design in prism
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style