carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

adding missing files #2677

Merged
mfreeman451 merged 1 commit from refs/pull/2677/head into staging 2026-01-15 05:36:09 +00:00
Conversation 2 Commits 1 Files changed 2 +355 -24
mfreeman451 commented 2026-01-15 05:36:03 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2315
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2315
Original created: 2026-01-15T05:36:03Z
Original updated: 2026-01-15T05:37:53Z
Original head: carverauto/serviceradar:chore/missing-files
Original base: staging
Original merged: 2026-01-15T05:36:09Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

  • I have signed the DCO?
  • The build completes without errors?
  • All tests are passing when running make test?

PR Type

Enhancement

Description

  • Implement device creation and CSV import functionality via Ash

  • Add device update capability with proper error handling

  • Support device UID generation and duplicate detection

  • Include tag parsing and device actor building helpers

Diagram Walkthrough

flowchart LR
  A["Device Live Views"] -->|"import_devices"| B["CSV Import Handler"]
  A -->|"create_device"| C["Device Creation"]
  A -->|"update_device"| D["Device Update"]
  B --> E["Ash Device Create"]
  C --> E
  D --> F["Ash Device Update"]
  E -->|"check duplicates"| G["Device UID Lookup"]
  F --> G
  E -->|"success/error"| H["Flash Messages"]
  F --> H

File Walkthrough
Relevant files
Enhancement
index.ex
Device creation and CSV import implementation                       

web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex

  • Added Scope alias for tenant and user context management
  • Implemented import_devices/2 function to handle CSV device imports
    with duplicate detection
  • Replaced placeholder save_device handler with actual device creation
    via Ash
  • Added helper functions for device UID generation, type parsing, tag
    normalization, and error formatting
  • Implemented create_single_device/3 to create devices with validation
    and duplicate checking
 +244/-17
show.ex
Device update functionality with Ash integration                 

web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex

  • Added Device alias for device resource access
  • Implemented update_device/3 function to update device attributes via
    Ash
  • Replaced placeholder save_device handler with actual device update
    logic
  • Added helper functions for tag input parsing and Ash error formatting
  • Included build_device_actor/1 to construct actor context for Ash
    operations
 +111/-7 
Imported from GitHub pull request. Original GitHub pull request: #2315 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2315 Original created: 2026-01-15T05:36:03Z Original updated: 2026-01-15T05:37:53Z Original head: carverauto/serviceradar:chore/missing-files Original base: staging Original merged: 2026-01-15T05:36:09Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement ___ ### **Description** - Implement device creation and CSV import functionality via Ash - Add device update capability with proper error handling - Support device UID generation and duplicate detection - Include tag parsing and device actor building helpers ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Device Live Views"] -->|"import_devices"| B["CSV Import Handler"] A -->|"create_device"| C["Device Creation"] A -->|"update_device"| D["Device Update"] B --> E["Ash Device Create"] C --> E D --> F["Ash Device Update"] E -->|"check duplicates"| G["Device UID Lookup"] F --> G E -->|"success/error"| H["Flash Messages"] F --> H ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>index.ex</strong><dd><code>Device creation and CSV import implementation</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex <ul><li>Added <code>Scope</code> alias for tenant and user context management<br> <li> Implemented <code>import_devices/2</code> function to handle CSV device imports <br>with duplicate detection<br> <li> Replaced placeholder <code>save_device</code> handler with actual device creation <br>via Ash<br> <li> Added helper functions for device UID generation, type parsing, tag <br>normalization, and error formatting<br> <li> Implemented <code>create_single_device/3</code> to create devices with validation <br>and duplicate checking</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7">+244/-17</a></td> </tr> <tr> <td> <details> <summary><strong>show.ex</strong><dd><code>Device update functionality with Ash integration</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex <ul><li>Added <code>Device</code> alias for device resource access<br> <li> Implemented <code>update_device/3</code> function to update device attributes via <br>Ash<br> <li> Replaced placeholder <code>save_device</code> handler with actual device update <br>logic<br> <li> Added helper functions for tag input parsing and Ash error formatting<br> <li> Included <code>build_device_actor/1</code> to construct actor context for Ash <br>operations</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24">+111/-7</a>&nbsp; </td> </tr> </table></td></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2026-01-15 05:36:38 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2315#issuecomment-3752979886
Original created: 2026-01-15T05:36:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Authorization bypass

Description: The new build_device_actor/1 falls back to a "system" actor with role: :admin when no user
is present, which could enable unintended privileged device creation/import if
current_scope is ever missing/mis-set or reachable in an unauthenticated context.
index.ex [2098-2115]

Referred Code 
defp build_device_actor(scope) do
  case scope do
    %{user: user} when not is_nil(user) ->
      %{
        id: user.id,
        email: user.email,
        role: user.role,
        tenant_id: Scope.tenant_id(scope)
      }

    _ ->
      %{
        id: "system",
        email: "system@serviceradar",
        role: :admin,
        tenant_id: Scope.tenant_id(scope)
      }
  end
Privilege escalation

Description: The new build_device_actor/1 falls back to a "system" actor with role: :admin when no user
is present, which could enable unintended privileged device updates if current_scope can
be absent/mis-set or the LiveView is accessible without a fully authenticated scope.
show.ex [2318-2336]

Referred Code 
defp build_device_actor(scope) do
  case scope do
    %{user: user} when not is_nil(user) ->
      %{
        id: user.id,
        email: user.email,
        role: user.role,
        tenant_id: Scope.tenant_id(scope)
      }

    _ ->
      %{
        id: "system",
        email: "system@serviceradar",
        role: :admin,
        tenant_id: Scope.tenant_id(scope)
      }
  end
end
Potential XSS

Description: User-controlled fields (device.hostname/device.ip and error inspect(reason)) are
interpolated into flash messages, which could become an XSS vector if any downstream
rendering uses raw/unsafe HTML for flashes or error output.
index.ex [224-250]

Referred Code 
def handle_event("save_device", %{"device" => params}, socket) do
  scope = socket.assigns.current_scope

  case create_device(scope, params) do
    {:ok, device} ->
      {:noreply,
       socket
       |> assign(:show_add_device_modal, false)
       |> assign(:add_device_form, to_form(%{}, as: :device))
       |> put_flash(:info, "Device '#{device.hostname || device.ip}' created successfully.")
       |> push_patch(to: ~p"/devices")}

    {:error, %Ash.Error.Invalid{} = error} ->
      {:noreply,
       socket
       |> put_flash(:error, format_device_error(error))}

    {:error, :already_exists} ->
      {:noreply,
       socket
       |> put_flash(:error, "A device with this IP address already exists.")}


 ... (clipped 6 lines)
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logging: New critical actions (CSV import/device create) are performed without any explicit audit
log entry containing user/tenant context and outcome.

Referred Code 
def handle_event("import_csv", _params, socket) do
  case socket.assigns.csv_preview do
    nil ->
      {:noreply, assign(socket, :csv_errors, ["No CSV data to import. Preview first."])}

    devices when is_list(devices) and devices != [] ->
      scope = socket.assigns.current_scope

      case import_devices(scope, devices) do
        {:ok, {created, skipped}} ->
          {:noreply,
           socket
           |> assign(:show_import_modal, false)
           |> assign(:csv_preview, nil)
           |> assign(:csv_errors, [])
           |> put_flash(:info, import_success_message(created, skipped))
           |> push_patch(to: ~p"/devices")}

        {:error, errors} when is_list(errors) ->
          {:noreply, assign(socket, :csv_errors, errors)}



 ... (clipped 3 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Error details exposed: User-facing flash messages include inspect(reason)/inspect(error) which can expose
internal implementation or backend details to end users.

Referred Code 
  {:error, %Ash.Error.Invalid{} = error} ->
    {:noreply,
     socket
     |> put_flash(:error, format_ash_error(error))}

  {:error, reason} ->
    {:noreply,
     socket
     |> put_flash(:error, "Failed to update device: #{inspect(reason)}")}
end

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing input validation: External inputs from forms/CSV are used to create devices without explicit
validation/sanitization (e.g., IP format, required fields), relying on downstream behavior
that is not visible in this diff.

Referred Code 
defp create_device(scope, params) do
  tenant_id = Scope.tenant_id(scope)

  if is_nil(tenant_id) do
    {:error, :no_tenant}
  else
    tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id)
    actor = build_device_actor(scope)

    # Build device data from form params
    device_data = %{
      hostname: params["hostname"],
      ip: params["ip"],
      type: params["type"],
      tags: parse_form_tags(params["tags"])
    }

    create_single_device(device_data, actor, tenant_schema)
  end
end



 ... (clipped 33 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated user input: Device update accepts and persists user-provided fields (including ip and tags) without
explicit validation/sanitization in the LiveView layer, and authorization enforcement
depends on Ash policies not shown in this diff.

Referred Code 
defp update_device(scope, device_uid, params) do
  tenant_id = Scope.tenant_id(scope)

  if is_nil(tenant_id) do
    {:error, :no_tenant}
  else
    tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id)
    actor = build_device_actor(scope)

    # Parse tags from newline-separated string to map
    attrs = %{
      hostname: params["hostname"],
      ip: params["ip"],
      vendor_name: params["vendor_name"],
      model: params["model"],
      tags: parse_tags_input(params["tags"])
    }
    |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end)
    |> Map.new()

    # First get the device, then update it


 ... (clipped 6 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2315#issuecomment-3752979886 Original created: 2026-01-15T05:36:38Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/b0f6131b5856708ca5faf4e32fc0a0790387af0f --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=3>⚪</td> <td><details><summary><strong>Authorization bypass </strong></summary><br> <b>Description:</b> The new <code>build_device_actor/1</code> falls back to a <code>"system"</code> actor with <code>role: :admin</code> when no user <br>is present, which could enable unintended privileged device creation/import if <br><code>current_scope</code> is ever missing/mis-set or reachable in an unauthenticated context.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R2098-R2115'>index.ex [2098-2115]</a></strong><br> <details open><summary>Referred Code</summary> ```elixir defp build_device_actor(scope) do case scope do %{user: user} when not is_nil(user) -> %{ id: user.id, email: user.email, role: user.role, tenant_id: Scope.tenant_id(scope) } _ -> %{ id: "system", email: "system@serviceradar", role: :admin, tenant_id: Scope.tenant_id(scope) } end ``` </details></details></td></tr> <tr><td><details><summary><strong>Privilege escalation </strong></summary><br> <b>Description:</b> The new <code>build_device_actor/1</code> falls back to a <code>"system"</code> actor with <code>role: :admin</code> when no user <br>is present, which could enable unintended privileged device updates if <code>current_scope</code> can <br>be absent/mis-set or the LiveView is accessible without a fully authenticated scope.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R2318-R2336'>show.ex [2318-2336]</a></strong><br> <details open><summary>Referred Code</summary> ```elixir defp build_device_actor(scope) do case scope do %{user: user} when not is_nil(user) -> %{ id: user.id, email: user.email, role: user.role, tenant_id: Scope.tenant_id(scope) } _ -> %{ id: "system", email: "system@serviceradar", role: :admin, tenant_id: Scope.tenant_id(scope) } end end ``` </details></details></td></tr> <tr><td><details><summary><strong>Potential XSS </strong></summary><br> <b>Description:</b> User-controlled fields (<code>device.hostname</code>/<code>device.ip</code> and error <code>inspect(reason)</code>) are <br>interpolated into flash messages, which could become an XSS vector if any downstream <br>rendering uses raw/unsafe HTML for flashes or error output.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R224-R250'>index.ex [224-250]</a></strong><br> <details open><summary>Referred Code</summary> ```elixir def handle_event("save_device", %{"device" => params}, socket) do scope = socket.assigns.current_scope case create_device(scope, params) do {:ok, device} -> {:noreply, socket |> assign(:show_add_device_modal, false) |> assign(:add_device_form, to_form(%{}, as: :device)) |> put_flash(:info, "Device '#{device.hostname || device.ip}' created successfully.") |> push_patch(to: ~p"/devices")} {:error, %Ash.Error.Invalid{} = error} -> {:noreply, socket |> put_flash(:error, format_device_error(error))} {:error, :already_exists} -> {:noreply, socket |> put_flash(:error, "A device with this IP address already exists.")} ... (clipped 6 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>🔴</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R190-R213'><strong>Missing audit logging</strong></a>: New critical actions (CSV import/device create) are performed without any explicit audit <br>log entry containing user/tenant context and outcome.<br> <details open><summary>Referred Code</summary> ```elixir def handle_event("import_csv", _params, socket) do case socket.assigns.csv_preview do nil -> {:noreply, assign(socket, :csv_errors, ["No CSV data to import. Preview first."])} devices when is_list(devices) and devices != [] -> scope = socket.assigns.current_scope case import_devices(scope, devices) do {:ok, {created, skipped}} -> {:noreply, socket |> assign(:show_import_modal, false) |> assign(:csv_preview, nil) |> assign(:csv_errors, []) |> put_flash(:info, import_success_message(created, skipped)) |> push_patch(to: ~p"/devices")} {:error, errors} when is_list(errors) -> {:noreply, assign(socket, :csv_errors, errors)} ... (clipped 3 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R211-R220'><strong>Error details exposed</strong></a>: User-facing flash messages include <code>inspect(reason)</code>/<code>inspect(error)</code> which can expose <br>internal implementation or backend details to end users.<br> <details open><summary>Referred Code</summary> ```elixir {:error, %Ash.Error.Invalid{} = error} -> {:noreply, socket |> put_flash(:error, format_ash_error(error))} {:error, reason} -> {:noreply, socket |> put_flash(:error, "Failed to update device: #{inspect(reason)}")} end ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>⚪</td> <td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R1992-R2045'><strong>Missing input validation</strong></a>: External inputs from forms/CSV are used to create devices without explicit <br>validation/sanitization (e.g., IP format, required fields), relying on downstream behavior <br>that is not visible in this diff.<br> <details open><summary>Referred Code</summary> ```elixir defp create_device(scope, params) do tenant_id = Scope.tenant_id(scope) if is_nil(tenant_id) do {:error, :no_tenant} else tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id) actor = build_device_actor(scope) # Build device data from form params device_data = %{ hostname: params["hostname"], ip: params["ip"], type: params["type"], tags: parse_form_tags(params["tags"]) } create_single_device(device_data, actor, tenant_schema) end end ... (clipped 33 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R2285-R2311'><strong>Unvalidated user input</strong></a>: Device update accepts and persists user-provided fields (including <code>ip</code> and <code>tags</code>) without <br>explicit validation/sanitization in the LiveView layer, and authorization enforcement <br>depends on Ash policies not shown in this diff.<br> <details open><summary>Referred Code</summary> ```elixir defp update_device(scope, device_uid, params) do tenant_id = Scope.tenant_id(scope) if is_nil(tenant_id) do {:error, :no_tenant} else tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id) actor = build_device_actor(scope) # Parse tags from newline-separated string to map attrs = %{ hostname: params["hostname"], ip: params["ip"], vendor_name: params["vendor_name"], model: params["model"], tags: parse_tags_input(params["tags"]) } |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end) |> Map.new() # First get the device, then update it ... (clipped 6 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2026-01-15 05:37:53 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2315#issuecomment-3752982092
Original created: 2026-01-15T05:37:53Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Centralize device logic into a context

Refactor the code by moving device management business logic from Phoenix
LiveView modules (index.ex, show.ex) into a dedicated context module. This will
centralize operations like device creation, updates, and data normalization,
eliminating code duplication and improving separation of concerns.

Examples:

web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex [1943-2141] 
  # Device creation helpers

  defp import_success_message(created, skipped) when skipped > 0 and created > 0 do
    "Created #{created} device(s). #{skipped} device(s) skipped (already exist)."
  end

  defp import_success_message(_created, skipped) when skipped > 0 do
    "All #{skipped} device(s) already exist."
  end


 ... (clipped 189 lines)
web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2284-2371] 
  # Update device via Ash
  defp update_device(scope, device_uid, params) do
    tenant_id = Scope.tenant_id(scope)

    if is_nil(tenant_id) do
      {:error, :no_tenant}
    else
      tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id)
      actor = build_device_actor(scope)


 ... (clipped 78 lines)

Solution Walkthrough:

Before:

# In serviceradar_web_ng_web/live/device_live/index.ex
defmodule ...DeviceLive.Index do
  def handle_event("save_device", ..., socket) do
    create_device(scope, params) # Logic defined in this file
    ...
  end

  defp create_device(scope, params), do: ...
  defp build_device_actor(scope), do: ... # Duplicated
  defp format_device_error(error), do: ... # Duplicated
end

# In serviceradar_web_ng_web/live/device_live/show.ex
defmodule ...DeviceLive.Show do
  def handle_event("save_device", ..., socket) do
    update_device(scope, uid, params) # Logic defined in this file
    ...
  end

  defp update_device(scope, uid, params), do: ...
  defp build_device_actor(scope), do: ... # Duplicated
  defp format_ash_error(error), do: ... # Duplicated
end

After:

# In a new context module, e.g., lib/serviceradar/inventory/devices.ex
defmodule ServiceRadar.Inventory.Devices do
  def create_device(scope, params), do: ...
  def update_device(scope, uid, params), do: ...
  def import_devices(scope, devices), do: ...
  def format_error(error), do: ...
  # other helpers...
end

# In serviceradar_web_ng_web/live/device_live/index.ex
defmodule ...DeviceLive.Index do
  alias ServiceRadar.Inventory.Devices

  def handle_event("save_device", ..., socket) do
    case Devices.create_device(scope, params) do
      ...
    end
  end
end

# In serviceradar_web_ng_web/live/device_live/show.ex
defmodule ...DeviceLive.Show do
  alias ServiceRadar.Inventory.Devices

  def handle_event("save_device", ..., socket) do
    case Devices.update_device(scope, uid, params) do
      ...
    end
  end
end
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies significant business logic and duplicated helper functions (build_device_actor, error formatters) placed within LiveView modules, and moving this to a dedicated context module is a crucial architectural improvement for maintainability and code reuse.

High
Possible issue
Avoid race condition in device creation

Refactor create_single_device to directly attempt device creation and handle
unique constraint errors, avoiding a potential race condition from the current
check-then-act pattern.

web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex [2013-2049]

 defp create_single_device(device_data, actor, tenant_schema) do
-  # Generate a UID based on IP (or use a UUID)
   uid = generate_device_uid(device_data.ip)
+  now = DateTime.utc_now()
 
-  # First check if device already exists
-  case Device.get_by_uid(uid, actor: actor, tenant: tenant_schema) do
-    {:ok, _existing} ->
+  attrs = %{
+    uid: uid,
+    hostname: device_data.hostname,
+    ip: device_data.ip,
+    name: device_data.hostname || device_data.ip,
+    type: device_data[:type],
+    type_id: parse_type_id(device_data[:type]),
+    tags: normalize_tags(device_data[:tags]),
+    discovery_sources: ["manual"],
+    first_seen_time: now,
+    last_seen_time: now,
+    created_time: now
+  }
+  |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end)
+  |> Map.new()
+
+  changeset = Ash.Changeset.for_create(Device, :create, attrs)
+
+  case Ash.create(changeset, actor: actor, tenant: tenant_schema) do
+    {:ok, device} ->
+      {:ok, device}
+
+    {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Changes.Unique{} | _]}} ->
       {:error, :already_exists}
-
-    {:error, %Ash.Error.Query.NotFound{}} ->
-      # Device doesn't exist, create it
-      now = DateTime.utc_now()
-
-      attrs = %{
-        uid: uid,
-        hostname: device_data.hostname,
-        ip: device_data.ip,
-        name: device_data.hostname || device_data.ip,
-        type: device_data[:type],
-        type_id: parse_type_id(device_data[:type]),
-        tags: normalize_tags(device_data[:tags]),
-        discovery_sources: ["manual"],
-        first_seen_time: now,
-        last_seen_time: now,
-        created_time: now
-      }
-      |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end)
-      |> Map.new()
-
-      Device
-      |> Ash.Changeset.for_create(:create, attrs)
-      |> Ash.create(actor: actor, tenant: tenant_schema)
 
     {:error, _} = error ->
       error
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a potential race condition in the device creation logic and proposes a more robust "act-then-handle" pattern, which is a critical improvement for system reliability in a concurrent environment.

High
General
Handle not-found error explicitly

In update_device, explicitly match on Ash.Error.Query.NotFound when fetching the
device and return a distinct {:error, :not_found} tuple for better error
handling.

web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2312-2313]

+{:error, %Ash.Error.Query.NotFound{}} ->
+  {:error, :not_found}
 {:error, _} = error ->
   error
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly proposes to explicitly handle the NotFound error, which improves error handling and allows the caller to provide more specific feedback to the user.

Medium
Simplify device update logic

Refactor update_device to update the device directly using its device_uid with
Ash.Changeset.for_update/4, removing the redundant initial database fetch.

web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2285-2316]

 defp update_device(scope, device_uid, params) do
   tenant_id = Scope.tenant_id(scope)
 
   if is_nil(tenant_id) do
     {:error, :no_tenant}
   else
     tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id)
     actor = build_device_actor(scope)
 
     # Parse tags from newline-separated string to map
     attrs = %{
       hostname: params["hostname"],
       ip: params["ip"],
       vendor_name: params["vendor_name"],
       model: params["model"],
       tags: parse_tags_input(params["tags"])
     }
     |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end)
     |> Map.new()
 
-    # First get the device, then update it
-    case Device.get_by_uid(device_uid, actor: actor, tenant: tenant_schema) do
-      {:ok, device} ->
-        device
-        |> Ash.Changeset.for_update(:update, attrs)
-        |> Ash.update(actor: actor, tenant: tenant_schema)
-
-      {:error, _} = error ->
-        error
-    end
+    Device
+    |> Ash.Changeset.for_update(device_uid, :update, attrs)
+    |> Ash.update(actor: actor, tenant: tenant_schema)
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a good suggestion that improves performance by removing a redundant database query and simplifies the code to be more idiomatic with the Ash framework.

Low
  • More
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2315#issuecomment-3752982092 Original created: 2026-01-15T05:37:53Z --- ## PR Code Suggestions ✨ <!-- b0f6131 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Centralize device logic into a context</summary> ___ **Refactor the code by moving device management business logic from Phoenix <br>LiveView modules (<code>index.ex</code>, <code>show.ex</code>) into a dedicated context module. This will <br>centralize operations like device creation, updates, and data normalization, <br>eliminating code duplication and improving separation of concerns.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R1943-R2141">web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex [1943-2141]</a> </summary> ```elixir # Device creation helpers defp import_success_message(created, skipped) when skipped > 0 and created > 0 do "Created #{created} device(s). #{skipped} device(s) skipped (already exist)." end defp import_success_message(_created, skipped) when skipped > 0 do "All #{skipped} device(s) already exist." end ... (clipped 189 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R2284-R2371">web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2284-2371]</a> </summary> ```elixir # Update device via Ash defp update_device(scope, device_uid, params) do tenant_id = Scope.tenant_id(scope) if is_nil(tenant_id) do {:error, :no_tenant} else tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id) actor = build_device_actor(scope) ... (clipped 78 lines) ``` </details> ### Solution Walkthrough: #### Before: ```elixir # In serviceradar_web_ng_web/live/device_live/index.ex defmodule ...DeviceLive.Index do def handle_event("save_device", ..., socket) do create_device(scope, params) # Logic defined in this file ... end defp create_device(scope, params), do: ... defp build_device_actor(scope), do: ... # Duplicated defp format_device_error(error), do: ... # Duplicated end # In serviceradar_web_ng_web/live/device_live/show.ex defmodule ...DeviceLive.Show do def handle_event("save_device", ..., socket) do update_device(scope, uid, params) # Logic defined in this file ... end defp update_device(scope, uid, params), do: ... defp build_device_actor(scope), do: ... # Duplicated defp format_ash_error(error), do: ... # Duplicated end ``` #### After: ```elixir # In a new context module, e.g., lib/serviceradar/inventory/devices.ex defmodule ServiceRadar.Inventory.Devices do def create_device(scope, params), do: ... def update_device(scope, uid, params), do: ... def import_devices(scope, devices), do: ... def format_error(error), do: ... # other helpers... end # In serviceradar_web_ng_web/live/device_live/index.ex defmodule ...DeviceLive.Index do alias ServiceRadar.Inventory.Devices def handle_event("save_device", ..., socket) do case Devices.create_device(scope, params) do ... end end end # In serviceradar_web_ng_web/live/device_live/show.ex defmodule ...DeviceLive.Show do alias ServiceRadar.Inventory.Devices def handle_event("save_device", ..., socket) do case Devices.update_device(scope, uid, params) do ... end end end ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies significant business logic and duplicated helper functions (`build_device_actor`, error formatters) placed within LiveView modules, and moving this to a dedicated context module is a crucial architectural improvement for maintainability and code reuse. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Avoid race condition in device creation</summary> ___ **Refactor <code>create_single_device</code> to directly attempt device creation and handle <br>unique constraint errors, avoiding a potential race condition from the current <br>check-then-act pattern.** [web-ng/lib/serviceradar_web_ng_web/live/device_live/index.ex [2013-2049]](https://github.com/carverauto/serviceradar/pull/2315/files#diff-261a01f4876e5984e1d9e9b38a3540675dfb0272abc30e6bdb2a4fa610353cc7R2013-R2049) ```diff defp create_single_device(device_data, actor, tenant_schema) do - # Generate a UID based on IP (or use a UUID) uid = generate_device_uid(device_data.ip) + now = DateTime.utc_now() - # First check if device already exists - case Device.get_by_uid(uid, actor: actor, tenant: tenant_schema) do - {:ok, _existing} -> + attrs = %{ + uid: uid, + hostname: device_data.hostname, + ip: device_data.ip, + name: device_data.hostname || device_data.ip, + type: device_data[:type], + type_id: parse_type_id(device_data[:type]), + tags: normalize_tags(device_data[:tags]), + discovery_sources: ["manual"], + first_seen_time: now, + last_seen_time: now, + created_time: now + } + |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end) + |> Map.new() + + changeset = Ash.Changeset.for_create(Device, :create, attrs) + + case Ash.create(changeset, actor: actor, tenant: tenant_schema) do + {:ok, device} -> + {:ok, device} + + {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Changes.Unique{} | _]}} -> {:error, :already_exists} - - {:error, %Ash.Error.Query.NotFound{}} -> - # Device doesn't exist, create it - now = DateTime.utc_now() - - attrs = %{ - uid: uid, - hostname: device_data.hostname, - ip: device_data.ip, - name: device_data.hostname || device_data.ip, - type: device_data[:type], - type_id: parse_type_id(device_data[:type]), - tags: normalize_tags(device_data[:tags]), - discovery_sources: ["manual"], - first_seen_time: now, - last_seen_time: now, - created_time: now - } - |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end) - |> Map.new() - - Device - |> Ash.Changeset.for_create(:create, attrs) - |> Ash.create(actor: actor, tenant: tenant_schema) {:error, _} = error -> error end end ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a potential race condition in the device creation logic and proposes a more robust "act-then-handle" pattern, which is a critical improvement for system reliability in a concurrent environment. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Handle not-found error explicitly</summary> ___ **In <code>update_device</code>, explicitly match on <code>Ash.Error.Query.NotFound</code> when fetching the <br>device and return a distinct <code>{:error, :not_found}</code> tuple for better error <br>handling.** [web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2312-2313]](https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R2312-R2313) ```diff +{:error, %Ash.Error.Query.NotFound{}} -> + {:error, :not_found} {:error, _} = error -> error ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=2 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly proposes to explicitly handle the `NotFound` error, which improves error handling and allows the caller to provide more specific feedback to the user. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Simplify device update logic</summary> ___ **Refactor <code>update_device</code> to update the device directly using its <code>device_uid</code> with <br><code>Ash.Changeset.for_update/4</code>, removing the redundant initial database fetch.** [web-ng/lib/serviceradar_web_ng_web/live/device_live/show.ex [2285-2316]](https://github.com/carverauto/serviceradar/pull/2315/files#diff-92d8e0af57d2f65dfb8562e896dbea17ef9f47074ffd14f58261decc76f80c24R2285-R2316) ```diff defp update_device(scope, device_uid, params) do tenant_id = Scope.tenant_id(scope) if is_nil(tenant_id) do {:error, :no_tenant} else tenant_schema = ServiceRadarWebNGWeb.TenantResolver.schema_for_tenant_id(tenant_id) actor = build_device_actor(scope) # Parse tags from newline-separated string to map attrs = %{ hostname: params["hostname"], ip: params["ip"], vendor_name: params["vendor_name"], model: params["model"], tags: parse_tags_input(params["tags"]) } |> Enum.reject(fn {_k, v} -> is_nil(v) or v == "" end) |> Map.new() - # First get the device, then update it - case Device.get_by_uid(device_uid, actor: actor, tenant: tenant_schema) do - {:ok, device} -> - device - |> Ash.Changeset.for_update(:update, attrs) - |> Ash.update(actor: actor, tenant: tenant_schema) - - {:error, _} = error -> - error - end + Device + |> Ash.Changeset.for_update(device_uid, :update, attrs) + |> Ash.update(actor: actor, tenant: tenant_schema) end end ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=3 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: This is a good suggestion that improves performance by removing a redundant database query and simplifies the code to be more idiomatic with the Ash framework. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More <!-- /improve --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2677
No description provided.