feat(web): add desired firmware page#3250
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
WalkthroughThis PR adds a desired firmware admin view with HTML and JSON endpoints, database-backed record retrieval, routing and navigation updates, a new template, and integration tests for both response formats. ChangesDesired Firmware Admin View
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant FirmwareHandler
participant Database
Client->>Router: GET /admin/firmware or /admin/firmware.json
Router->>FirmwareHandler: show_html or show_json
FirmwareHandler->>Database: fetch_desired_firmware query
Database-->>FirmwareHandler: DesiredFirmware rows
FirmwareHandler-->>Client: Html or Json response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-web/src/firmware.rs (3)
70-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fetch + error-handling logic between
show_htmlandshow_json.Both handlers repeat the identical match-and-log-error block for
fetch_desired_firmware. Extract a shared helper to reduce duplication.♻️ Proposed fix
+async fn fetch_desired_firmware_or_error(state: &Api) -> Result<Vec<DesiredFirmware>, Response> { + fetch_desired_firmware(state).await.map_err(|err| { + tracing::error!(%err, "fetch desired firmware"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + "Error loading desired firmware", + ) + .into_response() + }) +} + pub async fn show_html(AxumState(state): AxumState<Arc<Api>>) -> Response { - let desired_firmware = match fetch_desired_firmware(&state).await { - Ok(rows) => rows, - Err(err) => { - tracing::error!(%err, "fetch desired firmware"); - return ( - StatusCode::INTERNAL_SERVER_ERROR, - "Error loading desired firmware", - ) - .into_response(); - } - }; + let desired_firmware = match fetch_desired_firmware_or_error(&state).await { + Ok(rows) => rows, + Err(response) => return response, + };Apply similarly in
show_json.🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` around lines 70 - 103, Both `show_html` and `show_json` duplicate the same `fetch_desired_firmware` match, error logging, and 500 response handling; extract that logic into a shared helper in `firmware.rs` and have both handlers call it. Keep the existing `tracing::error!(%err, "fetch desired firmware")` behavior and return the fetched rows on success so `show_html` and `show_json` only handle rendering/JSON conversion.
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid needless clones: consume the Vec instead of borrowing.
From<&DesiredFirmware>forcesvendor.clone()/model.clone()per row. Sincedesired_firmwareisn't reused after line 84, converting by value avoids the clones entirely.♻️ Proposed fix
-impl From<&DesiredFirmware> for DesiredFirmwareDisplay { - fn from(row: &DesiredFirmware) -> Self { +impl From<DesiredFirmware> for DesiredFirmwareDisplay { + fn from(row: DesiredFirmware) -> Self { Self { - vendor: row.vendor.clone(), - model: row.model.clone(), - versions: display_versions(&row.versions), + versions: display_versions(&row.versions), + vendor: row.vendor, + model: row.model, explicit_update_start_needed: row.explicit_update_start_needed, } } }let tmpl = FirmwareShow { - desired_firmware: desired_firmware.iter().map(Into::into).collect(), + desired_firmware: desired_firmware.into_iter().map(Into::into).collect(), };As per coding guidelines, "Avoid needless clones... Prefer implementing
From<T>overFrom<&T>... Use.into_iter()instead of.iter()to get owned values that can be moved without cloning."Also applies to: 123-132
🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` at line 84, The `desired_firmware` conversion is cloning each `DesiredFirmware` entry because it iterates by reference and uses `From<&DesiredFirmware>`. Update the conversion in the `desired_firmware` mapping to consume the Vec with ownership (use `into_iter()` and a value-based `From<DesiredFirmware>`/`Into` implementation) so `vendor` and `model` can be moved instead of cloned, and apply the same pattern to the related conversions in the referenced range.
86-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider handling
render()failure instead ofunwrap().Unlike the DB-fetch path (which returns a controlled 500), a template rendering failure here will panic. For consistency with the rest of the handler's error handling, consider logging and returning a 500 instead.
🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` at line 86, The firmware handler currently panics on template rendering by calling unwrap on tmpl.render(), unlike the rest of the flow that returns a controlled 500. Update the response path in the firmware handler to handle render failures explicitly, logging the render error and returning an internal server error instead of panicking. Use the firmware handler logic around tmpl.render() and the existing controlled-error path as the reference for consistent handling.
🤖 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.
Nitpick comments:
In `@crates/api-web/src/firmware.rs`:
- Around line 70-103: Both `show_html` and `show_json` duplicate the same
`fetch_desired_firmware` match, error logging, and 500 response handling;
extract that logic into a shared helper in `firmware.rs` and have both handlers
call it. Keep the existing `tracing::error!(%err, "fetch desired firmware")`
behavior and return the fetched rows on success so `show_html` and `show_json`
only handle rendering/JSON conversion.
- Line 84: The `desired_firmware` conversion is cloning each `DesiredFirmware`
entry because it iterates by reference and uses `From<&DesiredFirmware>`. Update
the conversion in the `desired_firmware` mapping to consume the Vec with
ownership (use `into_iter()` and a value-based `From<DesiredFirmware>`/`Into`
implementation) so `vendor` and `model` can be moved instead of cloned, and
apply the same pattern to the related conversions in the referenced range.
- Line 86: The firmware handler currently panics on template rendering by
calling unwrap on tmpl.render(), unlike the rest of the flow that returns a
controlled 500. Update the response path in the firmware handler to handle
render failures explicitly, logging the render error and returning an internal
server error instead of panicking. Use the firmware handler logic around
tmpl.render() and the existing controlled-error path as the reference for
consistent handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3262b99f-a46e-49fa-b01a-2ab8cf98deed
📒 Files selected for processing (6)
crates/api-web/src/firmware.rscrates/api-web/src/lib.rscrates/api-web/src/tests/firmware.rscrates/api-web/src/tests/mod.rscrates/api-web/templates/base.htmlcrates/api-web/templates/firmware_show.html
rahmonov
force-pushed
the
firmware-tab-table
branch
from
a138a98 to
c4a81d5
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3250.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/api-web/src/firmware.rs (4)
106-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a compile-time checked query.
fetch_desired_firmwareuses the runtimesqlx::query_as::<_, T>(&str)form rather than thesqlx::query_as!macro. The project's tech stack explicitly relies on SQLx for compile-time checked queries, and this bypasses that guarantee — a typo in a column name or type mismatch withDesiredFirmwareRowwon't be caught until runtime.♻️ Suggested fix
-const DESIRED_FIRMWARE_QUERY: &str = r#" - SELECT vendor, model, versions, explicit_update_start_needed - FROM desired_firmware - ORDER BY vendor, model -"#; - -async fn fetch_desired_firmware(api: &Api) -> Result<Vec<DesiredFirmware>, sqlx::Error> { - sqlx::query_as::<_, DesiredFirmwareRow>(DESIRED_FIRMWARE_QUERY) - .fetch_all(&api.database_connection) - .await - .map(|rows| rows.into_iter().map(Into::into).collect()) -} +async fn fetch_desired_firmware(api: &Api) -> Result<Vec<DesiredFirmware>, sqlx::Error> { + sqlx::query_as!( + DesiredFirmwareRow, + r#" + SELECT vendor, model, versions as "versions: SqlxJson<DesiredFirmwareVersions>", explicit_update_start_needed + FROM desired_firmware + ORDER BY vendor, model + "# + ) + .fetch_all(&api.database_connection) + .await + .map(|rows| rows.into_iter().map(Into::into).collect()) +}As per coding guidelines, "Database: SQLx (compile-time checked queries)".
🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` around lines 106 - 111, fetch_desired_firmware currently uses the runtime sqlx::query_as::<_, DesiredFirmwareRow> path, which bypasses SQLx compile-time validation. Replace it with the sqlx::query_as! macro in the same function so the DesiredFirmwareRow mapping and DESIRED_FIRMWARE_QUERY are checked at compile time, keeping the existing fetch_all and Into::into conversion flow intact.Source: Coding guidelines
31-35: 🚀 Performance & Scalability | 🔵 TrivialUnpaginated listing query.
The query fetches all
desired_firmwarerows with no limit/pagination. Repo guidelines recommend paginated list APIs for scalability. Given this table is likely bounded by vendor/model combinations, this is low priority, but worth keeping in mind if the catalog grows significantly.🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` around lines 31 - 35, The desired_firmware listing query is unpaginated and may return the full table, so update the list path around DESIRED_FIRMWARE_QUERY in firmware.rs to support pagination. Add limit/offset (or equivalent cursor-based paging) to the SQL and thread pagination parameters through the API handler that uses this query, while preserving the existing vendor/model ordering.Source: Coding guidelines
84-88: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAvoid
unwrap()on template rendering.
tmpl.render().unwrap()will panic the request task if Askama rendering ever fails (e.g. afmt::Writeerror). Propagate it as a 500 response instead of unwrapping, consistent with the error handling already done forfetch_desired_firmware.♻️ Suggested fix
- let tmpl = FirmwareShow { - desired_firmware: desired_firmware.iter().map(Into::into).collect(), - }; - (StatusCode::OK, Html(tmpl.render().unwrap())).into_response() + let tmpl = FirmwareShow { + desired_firmware: desired_firmware.iter().map(Into::into).collect(), + }; + match tmpl.render() { + Ok(body) => (StatusCode::OK, Html(body)).into_response(), + Err(err) => { + tracing::error!(%err, "render firmware template"); + (StatusCode::INTERNAL_SERVER_ERROR, "Error rendering firmware page").into_response() + } + }🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` around lines 84 - 88, The FirmwareShow response path currently unwraps Askama rendering and can panic the request task; update the firmware handler to handle rendering failures gracefully instead. In the function that builds FirmwareShow and returns Html, replace tmpl.render().unwrap() with error propagation into an HTTP 500 response, matching the existing fetch_desired_firmware error handling style so the handler returns a proper failure instead of panicking.
84-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an owned conversion to avoid cloning
vendor/model.
From<&DesiredFirmware> for DesiredFirmwareDisplayclonesvendorandmodelon every row. Sincedesired_firmwareisn't reused after buildingtmpl, an ownedFrom<DesiredFirmware>combined with.into_iter()would move the strings instead of cloning them.♻️ Suggested fix
-impl From<&DesiredFirmware> for DesiredFirmwareDisplay { - fn from(row: &DesiredFirmware) -> Self { - Self { - vendor: row.vendor.clone(), - model: row.model.clone(), - versions: display_versions(&row.versions), - explicit_update_start_needed: row.explicit_update_start_needed, - } - } -} +impl From<DesiredFirmware> for DesiredFirmwareDisplay { + fn from(row: DesiredFirmware) -> Self { + Self { + versions: display_versions(&row.versions), + vendor: row.vendor, + model: row.model, + explicit_update_start_needed: row.explicit_update_start_needed, + } + } +}And update the caller:
- desired_firmware: desired_firmware.iter().map(Into::into).collect(), + desired_firmware: desired_firmware.into_iter().map(Into::into).collect(),As per coding guidelines, "Prefer implementing
From<T>overFrom<&T>... this allows the conversion to move data without cloning."Also applies to: 124-133
🤖 Prompt for 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. In `@crates/api-web/src/firmware.rs` around lines 84 - 86, The `FirmwareShow`/`DesiredFirmwareDisplay` conversion is cloning `vendor` and `model` unnecessarily because it uses `From<&DesiredFirmware>` with `iter().map(Into::into)`. Add an owned `From<DesiredFirmware> for DesiredFirmwareDisplay` so the strings can be moved, then update the `FirmwareShow` builder to consume `desired_firmware` with `.into_iter()` and convert by value in the affected `firmware.rs` paths.Source: Coding guidelines
crates/api-web/src/tests/firmware.rs (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract shared test setup into a helper.
Both tests repeat the identical
TestEnv::new+insert_desired_firmware+make_test_appsequence. A small helper (e.g., returning the builtapp) would reduce duplication and keep future setup changes in one place.♻️ Proposed helper extraction
+async fn setup_firmware_app(pool: &sqlx::PgPool) -> impl tower::Service<axum::http::Request<Body>, Response = Response, Error = std::convert::Infallible> { + let env = TestEnv::new(pool.clone()).await; + insert_desired_firmware(pool).await; + make_test_app(&env.test_harness) +}Also applies to: 93-97
🤖 Prompt for 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. In `@crates/api-web/src/tests/firmware.rs` around lines 66 - 70, The firmware tests duplicate the same setup sequence in both test cases, so extract that repeated `TestEnv::new` + `insert_desired_firmware` + `make_test_app` flow into a small shared helper in `firmware.rs` that returns the prepared app; then update `firmware_page_shows_desired_firmware_table` and the other test that uses the same setup to call the helper instead of repeating the steps.
🤖 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.
Nitpick comments:
In `@crates/api-web/src/firmware.rs`:
- Around line 106-111: fetch_desired_firmware currently uses the runtime
sqlx::query_as::<_, DesiredFirmwareRow> path, which bypasses SQLx compile-time
validation. Replace it with the sqlx::query_as! macro in the same function so
the DesiredFirmwareRow mapping and DESIRED_FIRMWARE_QUERY are checked at compile
time, keeping the existing fetch_all and Into::into conversion flow intact.
- Around line 31-35: The desired_firmware listing query is unpaginated and may
return the full table, so update the list path around DESIRED_FIRMWARE_QUERY in
firmware.rs to support pagination. Add limit/offset (or equivalent cursor-based
paging) to the SQL and thread pagination parameters through the API handler that
uses this query, while preserving the existing vendor/model ordering.
- Around line 84-88: The FirmwareShow response path currently unwraps Askama
rendering and can panic the request task; update the firmware handler to handle
rendering failures gracefully instead. In the function that builds FirmwareShow
and returns Html, replace tmpl.render().unwrap() with error propagation into an
HTTP 500 response, matching the existing fetch_desired_firmware error handling
style so the handler returns a proper failure instead of panicking.
- Around line 84-86: The `FirmwareShow`/`DesiredFirmwareDisplay` conversion is
cloning `vendor` and `model` unnecessarily because it uses
`From<&DesiredFirmware>` with `iter().map(Into::into)`. Add an owned
`From<DesiredFirmware> for DesiredFirmwareDisplay` so the strings can be moved,
then update the `FirmwareShow` builder to consume `desired_firmware` with
`.into_iter()` and convert by value in the affected `firmware.rs` paths.
In `@crates/api-web/src/tests/firmware.rs`:
- Around line 66-70: The firmware tests duplicate the same setup sequence in
both test cases, so extract that repeated `TestEnv::new` +
`insert_desired_firmware` + `make_test_app` flow into a small shared helper in
`firmware.rs` that returns the prepared app; then update
`firmware_page_shows_desired_firmware_table` and the other test that uses the
same setup to call the helper instead of repeating the steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 99d50b3c-db51-49cc-9057-ebaa1a644483
📒 Files selected for processing (6)
crates/api-web/src/firmware.rscrates/api-web/src/lib.rscrates/api-web/src/tests/firmware.rscrates/api-web/src/tests/mod.rscrates/api-web/templates/base.htmlcrates/api-web/templates/firmware_show.html
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/api-web/templates/base.html
- crates/api-web/src/lib.rs
- crates/api-web/src/tests/mod.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Adding firmware tab to the admin web ui. With the changes, it will look like this:
I will later add a form to create/update firmware configuration to make host firmware upgrades easier.
Type of Change
Testing