Pr/librenms-id-multi-server U#245 - #41
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (3)
🧰 Additional context used📓 Path-based instructions (1)netbox_librenms_plugin/**/*.py📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (18)📓 Common learnings📚 Learning: 2026-03-12T23:45:47.436ZApplied to files:
📚 Learning: 2026-03-09T19:16:08.084ZApplied to files:
📚 Learning: 2026-03-08T11:35:39.628ZApplied to files:
📚 Learning: 2026-03-03T13:23:33.731ZApplied to files:
📚 Learning: 2026-03-07T10:40:44.412ZApplied to files:
📚 Learning: 2026-03-03T13:23:33.731ZApplied to files:
📚 Learning: 2026-03-07T11:09:07.715ZApplied to files:
📚 Learning: 2026-03-07T12:29:43.766ZApplied to files:
📚 Learning: 2026-03-07T10:33:35.311ZApplied to files:
📚 Learning: 2026-03-08T08:55:52.594ZApplied to files:
📚 Learning: 2026-03-13T11:24:30.934ZApplied to files:
📚 Learning: 2026-03-07T16:59:45.395ZApplied to files:
📚 Learning: 2026-03-07T22:48:34.766ZApplied to files:
📚 Learning: 2026-03-09T21:39:39.919ZApplied to files:
📚 Learning: 2026-03-06T18:41:13.852ZApplied to files:
📚 Learning: 2026-03-07T22:46:57.537ZApplied to files:
📚 Learning: 2026-03-08T13:09:49.031ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds per-server LibreNMS support: librenms_id becomes server-scoped, cache keys and lookups become server-aware, server_key is threaded through imports/VC/VM flows, views and templates propagate server context, adds migration/removal views for legacy IDs, and expands tests and JS to support the multi-server model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User/Client
participant View as Sync View
participant Cache as Cache Layer
participant API as LibreNMS API
participant DB as NetBox DB
Client->>View: GET/POST (includes server_key)
activate View
View->>Cache: get_cache_key(obj, data_type, server_key)
activate Cache
Cache-->>View: cached result or miss
deactivate Cache
alt Cache miss
View->>API: Fetch LibreNMS data (server_key)
activate API
API-->>View: Return data
deactivate API
View->>Cache: Store data under server-scoped key
View->>DB: create/update objects and call set_librenms_device_id(obj, id, server_key)
activate DB
DB-->>View: confirm
deactivate DB
end
View->>View: _build_all_server_mappings(obj, active_server_key)
View-->>Client: Render page/context with server-scoped mappings
deactivate View
sequenceDiagram
participant User as User
participant View as ConvertLegacyView
participant DB as NetBox DB
participant Utils as Utils (find/migrate)
User->>View: POST legacy_id + server_key
activate View
View->>DB: select_for_update(target object)
activate DB
DB-->>View: locked
deactivate DB
View->>Utils: validate legacy value and serial
View->>Utils: find_by_librenms_id(conflict check, server_key)
alt No conflict
View->>Utils: migrate_legacy_librenms_id(obj, server_key)
View->>DB: save updated custom_field_data
View-->>User: success
else Conflict
View-->>User: error (already linked)
end
deactivate View
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netbox_librenms_plugin/views/object_sync/devices.py (1)
109-125:⚠️ Potential issue | 🟠 MajorDon't fall back to the
"default"cache namespace here.If the client omits
server_key, this endpoint will read interface data from thedefaultcache even when the active sync page is scoped to another LibreNMS server. On non-default servers that turns valid cached rows into false 404s.🔧 Suggested fix
- server_key = data.get("server_key") or "default" + server_key = data.get("server_key") or self.librenms_api.server_keyBased on learnings: In
netbox_librenms_plugin/views/base/cables_view.py, request-scopedserver_keymust fall back toself.librenms_api.server_keyso cache lookups stay in the same server namespace as the cached data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/object_sync/devices.py` around lines 109 - 125, The handler currently falls back to the literal "default" when request JSON omits server_key, causing cache lookups to use the wrong namespace; change the server_key initialization from data.get("server_key") or "default" to use the view's Librenms API server key (e.g. self.librenms_api.server_key) so subsequent calls to get_librenms_sync_device(selected_device, server_key=server_key) and cache.get(self.get_cache_key(primary_device, "ports", server_key)) use the same request-scoped namespace as other views; update the server_key assignment in this function to use self.librenms_api.server_key as the fallback.netbox_librenms_plugin/utils.py (1)
80-120:⚠️ Potential issue | 🟠 MajorKeep the unscoped VC fallback separate from the
"default"server namespace.Line 80 changes this helper from “no active server context” semantics to “implicitly use the
defaultserver.” Any caller that intentionally omitsserver_keywill now miss VC members mapped only on non-default servers and fall through to the IP/position heuristics instead. The default should stayNone, with a dedicatedserver_key is Nonebranch that treats any non-null per-server mapping as eligible.Based on learnings: In
netbox_librenms_plugin/utils.py,get_librenms_sync_device()defaults toserver_key=None; callers without an active server context rely on that to scan mappings across all servers, while scoped callers pass a concrete server key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/utils.py` around lines 80 - 120, The function get_librenms_sync_device currently defaults server_key="default", which breaks callers that rely on an unscoped search across all per-server mappings; change the default to server_key: Optional[str] = None and update the Priority 1 logic so that when server_key is None you accept any non-null entry in a dict-valued librenms_id (i.e., treat any per-server mapping as a match), while retaining the existing behavior when a concrete server_key is provided (only match raw_cf.get(server_key)); keep the existing Priority 2 fallback that calls get_librenms_device_id(member, server_key, auto_save=False) for legacy/legacy-int lookups.netbox_librenms_plugin/import_utils/cache.py (1)
122-156:⚠️ Potential issue | 🟠 MajorUse a stable digest instead of
hash(); Python's built-in hash is process-randomized.Line 153 uses
hash(str(sorted(filters.items()))), which produces different values across interpreter processes with differentPYTHONHASHSEEDvalues (verified: seed=1 yields-6682795898117886439, seed=2 yields3262197562461628913). This breaks cache key consistency between the background job worker and the web process, causing_load_job_results()to miss cache entries created by background jobs since they will compute different keys for the same filters.Replace with a stable digest of a canonical serialization:
Proposed fix
+import hashlib +import json @@ - filter_hash = hash(str(sorted(filters.items()))) + canonical_filters = json.dumps(sorted(filters.items()), separators=(",", ":"), ensure_ascii=True) + filter_hash = hashlib.sha256(canonical_filters.encode()).hexdigest()[:16]Per the coding guidelines and background-jobs instructions, both synchronous and background modes must use
get_validated_device_cache_key()to generate cache keys, ensuring_load_job_results()in the list view can retrieve devices regardless of which mode produced them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/cache.py` around lines 122 - 156, The current get_validated_device_cache_key uses Python's non-deterministic built-in hash on str(sorted(filters.items())), causing different processes to produce different keys; change this to compute a stable digest by canonicalizing the filters (e.g., serialize filters in a deterministic order using json with sort_keys=True and compact separators) and then compute a stable hash/digest (e.g., hashlib.sha256 over the UTF-8 bytes and use hexdigest or a truncated portion) to replace filter_hash so both web and background workers derive identical cache keys from the same filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/forms.py`:
- Around line 583-587: The current code instantiates LibreNMSAPI() before
checking the warm cache which breaks the two-phase lookup; change
_populate_librenms_locations to first derive a cheap _server_key from
LibreNMSSettings.selected_server and call get_location_choices_cache_key(...) +
cache.get(...) using that key, returning cached_choices if present, and only if
the cheap-key lookup misses instantiate LibreNMSAPI(), obtain api.server_key,
recompute the cache_key with get_location_choices_cache_key(api.server_key),
check cache.get(...) again, and proceed to fetch from the live API only on cache
miss; reference symbols: _populate_librenms_locations,
LibreNMSSettings.selected_server, LibreNMSAPI(), get_location_choices_cache_key,
cache.get, cache_key, cached_choices.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 170-182: The vc_domain fallback is per-member when member_serials
is empty; change it to a stack-wide key so all members without serials share the
same dedup key. Specifically, when member_serials is empty, build a
deterministic stack identifier from vc_data["members"] (e.g., sorted member
attributes such as member.get("hostname") or member.get("id") converted to
strings and joined) and use f"librenms-stack-{joined_ids}" as vc_domain; only
fall back to f"librenms-{device_id}" if there are no members at all. This
ensures create_virtual_chassis_with_members() is only triggered once for the
whole stack.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 493-496: The code refreshes import_as_vm from result but then
always appends "Cluster must be manually selected..." and "Device role must be
manually selected..." blockers even when an existing device is matched; change
the logic so the cluster-required blocker (VM path) and the device_role-required
blocker (device path) are only appended when not result.get('existing_device')
while still populating available_clusters and available_roles for both branches;
locate and wrap the blocker-appending blocks that reference import_as_vm,
result["existing_device"], available_clusters and available_roles with an if not
result.get('existing_device') guard so create-time prerequisites are only added
for new imports and not for link/update flows.
- Around line 531-547: match_librenms_hardware_to_device_type can return None
for ambiguous mappings, so guard against dt_match being None before subscripting
it; first check "if dt_match is None" and set
result["device_type"]["found"]=False, result["device_type"]["device_type"]=None
and result["device_type"]["match_type"]="ambiguous" (or another sentinel you
use) so callers get a deterministic ambiguous result, then continue the existing
logic that tries chassis lookup via _try_chassis_device_type_match(api,
device_id) only when dt_match is not a dict with matched True/False; ensure
subsequent accesses use dt_match.get(...) only after confirming dt_match is a
dict.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1345-1375: The Q-matcher in
_setup_librenms_id_match/_librenms_id_filter_side_effect is too shape-sensitive
and should detect librenms_id anywhere in the Q expression; replace the current
children-based check with a string check like "librenms_id" in str(q) so
nested/ORed Q objects created by find_by_librenms_id() are matched correctly and
the MagicMock returns the expected hit.
In `@netbox_librenms_plugin/views/base/vlan_table_view.py`:
- Around line 113-119: The error context returned by _get_error_context is
missing the active server identity, so update _get_error_context to include
"server_key": getattr(self.librenms_api, "server_key", None) in its returned
dict (to match the key returned by get_vlan_context) so that after a failed VLAN
refresh the template retains the active server identity for retries.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 337-364: The code currently falls back to the literal "default"
for server_key, causing cache lookups (get_cache_key, get_vlan_overrides_key) to
use a different namespace than the sync view; change the fallback to use the
request-scoped server key from the view instance (self.librenms_api.server_key)
instead of "default" by replacing server_key = data.get("server_key") or
"default" with server_key = data.get("server_key") or
self.librenms_api.server_key so get_librenms_sync_device,
cache.ttl(self.get_cache_key(...)), and
cache.set(self.get_vlan_overrides_key(...)) use the same server namespace as the
cached ports data.
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 122-156: The current get_validated_device_cache_key uses Python's
non-deterministic built-in hash on str(sorted(filters.items())), causing
different processes to produce different keys; change this to compute a stable
digest by canonicalizing the filters (e.g., serialize filters in a deterministic
order using json with sort_keys=True and compact separators) and then compute a
stable hash/digest (e.g., hashlib.sha256 over the UTF-8 bytes and use hexdigest
or a truncated portion) to replace filter_hash so both web and background
workers derive identical cache keys from the same filters.
In `@netbox_librenms_plugin/utils.py`:
- Around line 80-120: The function get_librenms_sync_device currently defaults
server_key="default", which breaks callers that rely on an unscoped search
across all per-server mappings; change the default to server_key: Optional[str]
= None and update the Priority 1 logic so that when server_key is None you
accept any non-null entry in a dict-valued librenms_id (i.e., treat any
per-server mapping as a match), while retaining the existing behavior when a
concrete server_key is provided (only match raw_cf.get(server_key)); keep the
existing Priority 2 fallback that calls get_librenms_device_id(member,
server_key, auto_save=False) for legacy/legacy-int lookups.
In `@netbox_librenms_plugin/views/object_sync/devices.py`:
- Around line 109-125: The handler currently falls back to the literal "default"
when request JSON omits server_key, causing cache lookups to use the wrong
namespace; change the server_key initialization from data.get("server_key") or
"default" to use the view's Librenms API server key (e.g.
self.librenms_api.server_key) so subsequent calls to
get_librenms_sync_device(selected_device, server_key=server_key) and
cache.get(self.get_cache_key(primary_device, "ports", server_key)) use the same
request-scoped namespace as other views; update the server_key assignment in
this function to use self.librenms_api.server_key as the fallback.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d686a70-261c-4b82-ad58-8cfdb9197abf
📒 Files selected for processing (53)
docs/development/testing.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_virtual_chassis.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/object_sync/vms.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/vlans.py
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 11, 2026 21:26
60e050e to
b5edb6f
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 11, 2026 22:50
c2432b0 to
c2f5cf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/jobs.py (1)
211-217:⚠️ Potential issue | 🟠 MajorPass the resolved server namespace into
bulk_import_devices_shared().
LibreNMSAPI(server_key=None)can still resolveapi.server_keyfrom settings, but this call forwards the rawserver_keyargument. On that path, the import runs underNonewhilejob.data["server_key"]records the resolved server, so per-serverlibrenms_idwrites can land in the wrong namespace.🔧 Suggested fix
if device_ids: self.logger.info(f"Importing {len(device_ids)} devices...") device_result = bulk_import_devices_shared( device_ids=device_ids, - server_key=server_key, + server_key=api.server_key, sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, vc_detection_enabled=vc_detection_enabled, job=self, # Pass job context for logging and cancellationPlease add a regression where
run(..., server_key=None)still forwards the resolved key. Based on learnings: since commit5f72979, callers are expected to passapi.server_keyso the deferredserver_key=Nonedefaults in the ID helpers are never hit accidentally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/jobs.py` around lines 211 - 217, The call to bulk_import_devices_shared forwards the raw server_key argument which can be None even though LibreNMSAPI resolves api.server_key (causing writes to land in the wrong namespace); update the bulk_import_devices_shared invocation to pass the resolved server key (e.g., use api.server_key or resolved_server_key) instead of the original server_key variable and ensure job.data["server_key"] is set from that resolved value; also add a regression test (e.g., test_run_forwards_resolved_server_key_when_called_with_none) that calls run(..., server_key=None) and asserts the helper receives the resolved api.server_key so the deferred defaults in ID helpers are never hit accidentally.netbox_librenms_plugin/import_utils/virtual_chassis.py (1)
219-245:⚠️ Potential issue | 🟡 MinorKeep VC preview naming consistent with the creation logic.
These paths keep duplicate positive positions as-is when building
suggested_name, butcreate_virtual_chassis_with_members()later reassigns duplicate slots to the next free position. When a device reports the sameentPhysicalParentRelPosfor multiple members, the import preview can show duplicate names and then create a different layout.Also applies to: 333-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/virtual_chassis.py` around lines 219 - 245, The VC preview currently uses the raw computed position and can produce duplicate suggested_name values when multiple chassis report the same entPhysicalParentRelPos; update the preview code that builds member_data (the position calculation and suggested_name assignment) to normalize positions exactly the same way create_virtual_chassis_with_members() does by assigning duplicate or invalid slots to the next free 1-based slot before calling _generate_vc_member_name; in practice, extract or reuse the dedup/slot-allocation logic from create_virtual_chassis_with_members() (or a new helper used by both) so member_data["position"] is the de-duplicated 1-based slot and suggested_name generation via _generate_vc_member_name(master_name, position, ...) matches creation-time behavior.
♻️ Duplicate comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
57-59:⚠️ Potential issue | 🟠 MajorDon't collapse ambiguous device-type matches into the generic "no match" path.
match_librenms_hardware_to_device_type()usesNoneas the ambiguous-mapping sentinel. The chassis helper currently drops that case, and the outer block still appends the generic "No matching device type found..." issue wheneverfoundis false. An ambiguous mapping now surfaces as either the wrong remediation or both messages at once, instead of clearly telling the user to fix the duplicate mapping rows.🛠️ Minimal direction
- if chassis_match is None: - continue + if chassis_match is None: + return {"matched": False, "device_type": None, "match_type": "ambiguous"} - if not result["device_type"]["found"]: + if result["device_type"]["match_type"] != "ambiguous" and not result["device_type"]["found"]:Based on learnings: In
netbox_librenms_plugin/utils.py,match_librenms_hardware_to_device_typereturnsNone(not a dict) whenDeviceTypeMapping.MultipleObjectsReturnedis raised — callers must guardif result is Noneseparately from the normalif not result["matched"]check.Also applies to: 538-563
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 57 - 59, The code currently treats a None return from match_librenms_hardware_to_device_type(value) the same as a non-matching dict, which hides the "ambiguous mapping" sentinel used when DeviceTypeMapping.MultipleObjectsReturned occurs; update the chassis handling to explicitly check for chassis_match is None and append/emit the specific ambiguous- mapping remediation (ask user to fix duplicate mapping rows) instead of continuing into the generic "no match" path, and keep the existing check for if not chassis_match["matched"] to handle true misses; apply the same explicit None-guard pattern wherever match_librenms_hardware_to_device_type is used in this module (e.g., the other chassis/device mapping block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/README.md:
- Line 49: The quick-start jump link "[LibreNMS Server
configuration](`#librenms-server-configuration`)" doesn't match the actual
generated anchor for the heading "### 📡 LibreNMS Server Configuration"; update
the link fragment to the exact slug produced by the Markdown renderer for that
heading (or reword the heading to match the existing fragment). Locate the link
text "[LibreNMS Server configuration]" and the heading "### 📡 LibreNMS Server
Configuration" and make the fragment portion of the link match the heading's
generated anchor so the jump works (verify in a Markdown preview after
changing).
In `@netbox_librenms_plugin/forms.py`:
- Around line 549-564: The code treats an empty bound QueryDict as "option-only"
so the background-job default isn't injected; update the has_option_only
calculation to require that data is non-empty. Specifically, change
has_option_only to include a truthy-data check (e.g., has_option_only =
bool(data) and not bool(non_option_fields) and not has_filters) so the
subsequent block that sets data["use_background_job"] = "on" runs for the
initial empty-bound form path in the if that checks "use_background_job",
job_id, has_filters and has_option_only.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1550-1557: The fetch call that builds the preview request using
previewUrl, params and signal currently adds an unnecessary 'X-CSRFToken' header
for a GET (query-string) request; remove the headers: { 'X-CSRFToken': ... }
entry from the fetch invocation that constructs the preview request (the block
referencing previewUrl, params, signal) so the GET request is sent without the
CSRF header.
- Around line 633-634: The VLAN save payload is grabbing a non-existent element
ID ('current-server-key') so server_key is always null; update the code that
builds the payload (the object containing vid_group_map and server_key in
librenms_sync.js) to select the hidden input by name instead — use
document.querySelector('input[name="server_key"]') to read its value (falling
back to null) so server_key matches how verifyVlanInGroup/verifyVlanSyncGroup
obtain the server key.
In `@netbox_librenms_plugin/tests/mock_librenms_server.py`:
- Around line 230-236: The mock server registers the inventory handler only at
"/api/v0/inventory/{device_id}" but client code (get_device_inventory) calls
"/api/v0/inventory/{device_id}/all", so the "No filter → return all" branch
never runs; update the route registration to add the "/all" path as well by
mapping both "/api/v0/inventory/{device_id}" and
"/api/v0/inventory/{device_id}/all" to the same _handler (i.e. assign _handler
into self.routes for both keys) so tests exercising the client-side fallback
receive the combined inventory.
In `@netbox_librenms_plugin/tests/test_coverage_api.py`:
- Around line 1-1176: The test file
netbox_librenms_plugin/tests/test_coverage_api.py is not formatted per ruff
rules; run the formatter and commit the changes (e.g., run `ruff format
netbox_librenms_plugin/tests/test_coverage_api.py` or your repo's pre-commit
formatter) so classes like TestLibreNMSAPIInitFallback,
TestTestConnectionErrors, TestGetAvailableServersLegacy, and other test
classes/definitions in this file comply with the project's ruff/formatting
rules; re-run tests and push the formatted file.
In `@netbox_librenms_plugin/tests/test_coverage_base_views.py`:
- Around line 1808-1809: The test is only asserting the context dict and not
that the correct template was rendered; update the assertions to verify both the
rendered template name and the context: inspect mock_render.call_args[0][1] to
assert it equals (or contains) partial_template_name (e.g. "test_template.html")
and keep/assert that mock_render.call_args[0][2] contains the "ip_sync" key so
you validate the full render() contract used by the view.
In `@netbox_librenms_plugin/tests/test_coverage_base_views2.py`:
- Around line 1572-1583: The test test_exception_returns_500 asserts a 500 for
malformed JSON but should expect a 400; update the test to expect status_code ==
400 and assert the JSON response indicates a client error, and also ensure the
handler (the view.post method referenced in the test) catches
json.JSONDecodeError when calling json.loads(req.body) and returns a
JsonResponse with HTTP 400 and an error payload (e.g., {"status":"error", ...})
so malformed client JSON is treated as bad request rather than internal server
error.
In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py`:
- Around line 370-384: Update the test_no_match_result to also exercise the None
return path from match_librenms_hardware_to_device_type: add or modify a test
case that patches
netbox_librenms_plugin.views.sync.device_fields.match_librenms_hardware_to_device_type
to return None (instead of {"matched": False}), call UpdateDeviceTypeView.post
(the existing view.post invocation in test_no_match_result), and assert that
messages.error was invoked (mock_msg.error.assert_called_once()). This ensures
UpdateDeviceTypeView.post’s guard for a falsy match_result is covered.
- Around line 785-814: The current test only simulates IntegrityError from
Platform.save(), but CreateAndAssignPlatformView.post() also has a separate
except block for an IntegrityError raised by device.save() after
select_for_update(); add a new test (e.g., test_integrity_error_on_device_save)
that mirrors test_integrity_error but sets the locked/device mock's save to
raise IntegrityError (mock_locked.save.side_effect =
IntegrityError("duplicate")), patch select_for_update to return this mock_locked
instance (or patch the object returned by get_object_or_404 to have
select_for_update.return_value), mock transaction.atomic as in the existing
test, call CreateAndAssignPlatformView.post (view.post(req, pk=1)), and assert
messages.error (mock_msg.error.assert_called_once()) to cover the device.save()
IntegrityError path.
In `@netbox_librenms_plugin/tests/test_coverage_device_operations.py`:
- Around line 1106-1156: The serial-conflict scenario currently nested inside
test_serial_dash_normalized should be split into its own test function: create a
new def test_serial_conflict() that contains the block starting from the
existing MagicMock setup through the final assert, keeping the same
imports/patches and using validate_device_for_import, mock_device,
_find_side_effect and patches as in the diff; remove that scenario from
test_serial_dash_normalized so each branch is independently tested and names
clearly indicate which branch failed. Ensure the new test function initializes
api = self._make_api(), starts/stops the same patches, uses
patch("netbox_librenms_plugin.utils.find_by_librenms_id",
side_effect=_find_side_effect) and
patch("netbox_librenms_plugin.import_utils.device_operations.Device",
mock_device), and asserts result.get("serial_action") == "conflict".
---
Outside diff comments:
In `@netbox_librenms_plugin/import_utils/virtual_chassis.py`:
- Around line 219-245: The VC preview currently uses the raw computed position
and can produce duplicate suggested_name values when multiple chassis report the
same entPhysicalParentRelPos; update the preview code that builds member_data
(the position calculation and suggested_name assignment) to normalize positions
exactly the same way create_virtual_chassis_with_members() does by assigning
duplicate or invalid slots to the next free 1-based slot before calling
_generate_vc_member_name; in practice, extract or reuse the
dedup/slot-allocation logic from create_virtual_chassis_with_members() (or a new
helper used by both) so member_data["position"] is the de-duplicated 1-based
slot and suggested_name generation via _generate_vc_member_name(master_name,
position, ...) matches creation-time behavior.
In `@netbox_librenms_plugin/jobs.py`:
- Around line 211-217: The call to bulk_import_devices_shared forwards the raw
server_key argument which can be None even though LibreNMSAPI resolves
api.server_key (causing writes to land in the wrong namespace); update the
bulk_import_devices_shared invocation to pass the resolved server key (e.g., use
api.server_key or resolved_server_key) instead of the original server_key
variable and ensure job.data["server_key"] is set from that resolved value; also
add a regression test (e.g.,
test_run_forwards_resolved_server_key_when_called_with_none) that calls run(...,
server_key=None) and asserts the helper receives the resolved api.server_key so
the deferred defaults in ID helpers are never hit accidentally.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 57-59: The code currently treats a None return from
match_librenms_hardware_to_device_type(value) the same as a non-matching dict,
which hides the "ambiguous mapping" sentinel used when
DeviceTypeMapping.MultipleObjectsReturned occurs; update the chassis handling to
explicitly check for chassis_match is None and append/emit the specific
ambiguous- mapping remediation (ask user to fix duplicate mapping rows) instead
of continuing into the generic "no match" path, and keep the existing check for
if not chassis_match["matched"] to handle true misses; apply the same explicit
None-guard pattern wherever match_librenms_hardware_to_device_type is used in
this module (e.g., the other chassis/device mapping block).
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18d6e7e2-a4b6-4f67-9d67-2a8667791dfd
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (84)
.devcontainer/README.md.github/pull_request_template.md.pre-commit-config.yamldocs/development/testing.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/vlans.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_api2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_cache.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_filters.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views3.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_vlans_table.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_integration_virtual_chassis.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/object_sync/vms.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/vlans.pypyproject.toml
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
2 times, most recently
from
March 11, 2026 23:01
9a5f44e to
2d68c3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/jobs.py (1)
103-119:⚠️ Potential issue | 🟡 MinorPreserve the real cache timestamp in
job.data.
process_device_filters()can now reuse validated rows from an existing cache entry and keep that entry’s original timestamp. Overwritingcached_atwithdatetime.now()here makes_load_job_results()show a fresh TTL even when the reused cache is already near expiry.Suggested fix
# Track cache timestamp for frontend expiration warnings from datetime import datetime, timezone + from django.core.cache import cache + from netbox_librenms_plugin.import_utils import get_cache_metadata_key - cached_at = datetime.now(timezone.utc).isoformat() + metadata = cache.get( + get_cache_metadata_key( + server_key=api.server_key, + filters=filters, + vc_enabled=vc_detection_enabled, + use_sysname=use_sysname, + strip_domain=strip_domain, + ) + ) or {} + cached_at = metadata.get("cached_at", datetime.now(timezone.utc).isoformat())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/jobs.py` around lines 103 - 119, The code always sets cached_at = datetime.now(timezone.utc).isoformat() before writing self.job.data, which overwrites the original cache timestamp when process_device_filters reuses an existing cache entry; instead, detect when validated rows were loaded from an existing cache entry and preserve that entry's timestamp (use its cached_at value) when populating self.job.data; update the assignment that builds self.job.data (and the cached_at variable) to prefer the existing cache entry's cached_at (if present) and only fall back to datetime.now(...) when no prior cached_at is available so _load_job_results sees the real TTL.
♻️ Duplicate comments (4)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (1)
635-639:⚠️ Potential issue | 🟠 MajorUse the existing hidden
server_keyinput in this payload.
current-server-keyis not the selector used by the sync templates, so this request still postsserver_key: nulland saves the VLAN override under the wrong server namespace in multi-server flows. Use the sameinput[name="server_key"]selector as the other verification handlers.🔧 Proposed fix
body: JSON.stringify({ device_id: deviceId, vid_group_map: vidGroupMap, - server_key: document.getElementById('current-server-key')?.value || null + server_key: document.querySelector('input[name="server_key"]')?.value || null })Based on learnings, the sync templates expose
server_keyas a hiddenname="server_key"input and the sync views rely on that POST value for server-scoped cache lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 635 - 639, The payload is using document.getElementById('current-server-key') which doesn't exist in the sync templates, causing server_key to be null; update the POST body in the function constructing the request (the block that sends device_id: deviceId and vid_group_map: vidGroupMap) to read the hidden input via document.querySelector('input[name="server_key"]')?.value || null instead of the current-server-key selector so the server-scoped cache lookup uses the correct server_key value.netbox_librenms_plugin/forms.py (1)
549-566:⚠️ Potential issue | 🟠 MajorEmpty bound data still disables the background-job default.
With
has_option_only = not bool(non_option_fields) and not has_filters, an emptyQueryDictis classified as “option-only”, so Line 566 never injectsuse_background_job=on. On the initial bound-form path the checkbox renders unchecked and the default silently flips to synchronous.Suggested fix
- has_option_only = not bool(non_option_fields) and not has_filters + has_option_only = ( + any(field in data for field in option_only_fields) + and not non_option_fields + and not has_filters + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/forms.py` around lines 549 - 566, The code treats an empty bound QueryDict as "option-only" which prevents injecting the default use_background_job; update the has_option_only calculation to consider an empty data mapping as not option-only. Specifically, in the block using option_only_fields, non_option_fields, and has_option_only, change has_option_only = not bool(non_option_fields) and not has_filters to also require that data is non-empty (e.g., use bool(data) or check data.keys()), so has_option_only = bool(data) and not bool(non_option_fields) and not has_filters; this preserves the default injection logic for truly unbound/initial loads while still recognizing real option-only submissions.netbox_librenms_plugin/tests/test_coverage_device_fields.py (1)
371-385:⚠️ Potential issue | 🟡 MinorCover the
Nonedevice-type match sentinel explicitly.This still only exercises
{"matched": False}.match_librenms_hardware_to_device_type()can also returnNonefor ambiguous mappings, and that path is the one the view now guards separately. Add a dedicatedreturn_value=Nonecase so that branch cannot regress silently.Based on learnings: In
netbox_librenms_plugin/utils.py,match_librenms_hardware_to_device_typereturnsNone(not a dict) whenDeviceTypeMapping.MultipleObjectsReturnedis raised — callers must guardif result is Noneseparately from the normalif not result["matched"]check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py` around lines 371 - 385, The test currently only covers the {"matched": False} case; add a new invocation (or extend this test) that patches match_librenms_hardware_to_device_type to return None to exercise the ambiguous-mapping sentinel path and assert the view still triggers an error response; specifically, in test_no_match_result (or a new test), patch netbox_librenms_plugin.views.sync.device_fields.match_librenms_hardware_to_device_type to return_value=None, call view.post(_make_request(), pk=1) and assert mock_msg.error.assert_called_once() (same setup as the existing test) so the branch that checks for result is None is covered.netbox_librenms_plugin/import_utils/bulk_import.py (1)
231-244:⚠️ Potential issue | 🟠 MajorThe serial-less VC dedup key is still per member.
The fallback hash is salted with the current
device_id, so each member of the same stack computes a differentvc_domainwhenevermember_serialsis empty. That reintroduces duplicate VC creation attempts for serial-less stacks.Suggested fix
- if member_parts: - fingerprint = hashlib.md5((f"{device_id}," + ",".join(member_parts)).encode()).hexdigest()[ - :12 - ] - vc_domain = f"librenms-stack-{fingerprint}" + member_ids = sorted( + str(m.get("device_id")) + for m in vc_data.get("members", []) + if m.get("device_id") is not None + ) + if member_ids: + vc_domain = f"librenms-stack-members-{','.join(member_ids)}" + elif member_parts: + fingerprint = hashlib.md5(",".join(member_parts).encode()).hexdigest()[:12] + vc_domain = f"librenms-stack-{fingerprint}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 231 - 244, The fallback currently salts the stack fingerprint with the per-device variable device_id so each member without serials gets a different vc_domain; instead compute the fingerprint solely from the stable sorted member descriptors (member_parts) and remove device_id from the hash input so all members of the same stack produce the same vc_domain; update the block that builds fingerprint/ vc_domain (symbols: vc_data, member_parts, fingerprint, vc_domain, device_id) to hash only ",".join(member_parts) and set vc_domain = f"librenms-stack-{fingerprint}", leaving the existing librenms-{device_id} fallback only when member_parts is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/usage_tips/custom_field.md`:
- Line 41: The docs entry that says "**Type:** JSON (object)" lacks an example
and mismatches the later manual-entry guidance; add a concrete JSON payload
example (e.g. {"librenms_id": 123} or {"librenms_id": "123"} as appropriate)
next to the Type line and update the manual-entry section to show that users
must enter a JSON object with a librenms_id key (not a bare integer),
referencing the field name librenms_id and the "**Type:** JSON (object)" line so
readers see both the expected JSON structure and the exact manual-edit sample to
prevent legacy bare-integer entries.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 125-150: Replace the silent "break" behavior inside the
device-processing loop in bulk_import.py: when RQ shows a stopped/failed job OR
in the DB-fallback branch after job.job.refresh_from_db() and checking
JobStatusChoices.STATUS_FAILED (or "failed"/"errored"), return an early result
object such as {"cancelled": True} (instead of break) so the caller can detect
cancellation; then update ImportDevicesJob.run() to check
result.get("cancelled") after each bulk import call and short-circuit subsequent
phases/recording (mark job cancelled) when true. Ensure you reference the RQ
check that uses RQJob.fetch, the DB fallback path that uses
job.job.refresh_from_db() and JobStatusChoices.STATUS_FAILED, and the
ImportDevicesJob.run() call site to implement the new early-return/short-circuit
flow.
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Line 176: Restore backwards compatibility by making server_key optional on
get_import_device_cache_key; change the signature for
get_import_device_cache_key(device_id: int | str, server_key: str = "default")
so callers can still call it with one arg, and ensure the function logic
continues to use server_key (defaulting to "default") when not provided; update
the type hint to include the default and leave the function body unchanged so
existing one-arg callers do not raise TypeError.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 538-563: The code treats an ambiguous dt_match (None from
match_librenms_hardware_to_device_type) as both "ambiguous" and "no match" —
remove the duplicate "No matching device type..." issue by only appending that
fallback when the device type was not found for a non-ambiguous lookup; i.e.,
change the final guard that currently checks if not
result["device_type"]["found"] to additionally ensure dt_match is not None (or
result["device_type"]["match_type"] != "ambiguous") before appending the "No
matching device type found..." message and invoking the device-type suggestion
logic so ambiguous results (dt_match is None) only produce the ambiguity issue
added earlier.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 461-465: Two fetch response handlers in librenms_sync.js currently
call response.text()/response.json() without checking response.ok; update the
.then(response => ...) handlers that throw new Error(response.text() || `HTTP
${response.status}`) and the one that directly returns response.json() to use
the standard guard: check if (!response.ok) then return response.text().then(t
=> { throw new Error(t || `HTTP ${response.status}`); }); otherwise return
response.json(); Locate the handlers by searching for the exact arrow callbacks
".then(response => {" that either call "response.text().then(... throw new
Error" or directly "return response.json()" and apply this pattern to both
occurrences so all fetch() responses validate response.ok before parsing.
In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py`:
- Around line 246-276: The tests (e.g., test_save_success_with_old_serial and
test_save_success_no_old_serial) only assert messages and not that the model
fields were actually updated; update each success-path test that calls view.post
(and similar tests at the other ranges mentioned) to assert that the mocked
device object's attributes (e.g., mock_device.serial, mock_device.device_type,
mock_device.platform) were assigned the expected new values after view.post
returns and that save() was called, by inspecting mock_device attributes and/or
mock_device.save.assert_called_once() so regressions that skip assigning the new
serial/device_type/platform will fail.
---
Outside diff comments:
In `@netbox_librenms_plugin/jobs.py`:
- Around line 103-119: The code always sets cached_at =
datetime.now(timezone.utc).isoformat() before writing self.job.data, which
overwrites the original cache timestamp when process_device_filters reuses an
existing cache entry; instead, detect when validated rows were loaded from an
existing cache entry and preserve that entry's timestamp (use its cached_at
value) when populating self.job.data; update the assignment that builds
self.job.data (and the cached_at variable) to prefer the existing cache entry's
cached_at (if present) and only fall back to datetime.now(...) when no prior
cached_at is available so _load_job_results sees the real TTL.
---
Duplicate comments:
In `@netbox_librenms_plugin/forms.py`:
- Around line 549-566: The code treats an empty bound QueryDict as "option-only"
which prevents injecting the default use_background_job; update the
has_option_only calculation to consider an empty data mapping as not
option-only. Specifically, in the block using option_only_fields,
non_option_fields, and has_option_only, change has_option_only = not
bool(non_option_fields) and not has_filters to also require that data is
non-empty (e.g., use bool(data) or check data.keys()), so has_option_only =
bool(data) and not bool(non_option_fields) and not has_filters; this preserves
the default injection logic for truly unbound/initial loads while still
recognizing real option-only submissions.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 231-244: The fallback currently salts the stack fingerprint with
the per-device variable device_id so each member without serials gets a
different vc_domain; instead compute the fingerprint solely from the stable
sorted member descriptors (member_parts) and remove device_id from the hash
input so all members of the same stack produce the same vc_domain; update the
block that builds fingerprint/ vc_domain (symbols: vc_data, member_parts,
fingerprint, vc_domain, device_id) to hash only ",".join(member_parts) and set
vc_domain = f"librenms-stack-{fingerprint}", leaving the existing
librenms-{device_id} fallback only when member_parts is empty.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 635-639: The payload is using
document.getElementById('current-server-key') which doesn't exist in the sync
templates, causing server_key to be null; update the POST body in the function
constructing the request (the block that sends device_id: deviceId and
vid_group_map: vidGroupMap) to read the hidden input via
document.querySelector('input[name="server_key"]')?.value || null instead of the
current-server-key selector so the server-scoped cache lookup uses the correct
server_key value.
In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py`:
- Around line 371-385: The test currently only covers the {"matched": False}
case; add a new invocation (or extend this test) that patches
match_librenms_hardware_to_device_type to return None to exercise the
ambiguous-mapping sentinel path and assert the view still triggers an error
response; specifically, in test_no_match_result (or a new test), patch
netbox_librenms_plugin.views.sync.device_fields.match_librenms_hardware_to_device_type
to return_value=None, call view.post(_make_request(), pk=1) and assert
mock_msg.error.assert_called_once() (same setup as the existing test) so the
branch that checks for result is None is covered.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fce02371-2d5e-46d4-a0cd-8bff2dd305a7
📒 Files selected for processing (45)
docs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/vlans.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_cache.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_integration_virtual_chassis.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.py
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/jobs.py (1)
200-214:⚠️ Potential issue | 🟠 MajorResolve
server_keyonce before writing import metadata.The new
api.server_keywiring can leak a mocked object intobulk_import_devices_shared()andjob.data["server_key"]; that is already breakingtest_run_mixed_device_and_vm_importvia cache-key mismatches. Use a concreteresolved_server_keyhere and reuse it in both places.Suggested hardening
# Initialize API client api = LibreNMSAPI(server_key=server_key) + resolved_server_key = server_key if server_key is not None else api.server_key ... device_result = bulk_import_devices_shared( device_ids=device_ids, - server_key=api.server_key, + server_key=resolved_server_key, sync_options=sync_options, manual_mappings_per_device=manual_mappings_per_device, libre_devices_cache=libre_devices_cache, vc_detection_enabled=vc_detection_enabled, job=self, # Pass job context for logging and cancellation user=self.job.user, # Pass user for permission checks ) ... self.job.data = { "imported_device_pks": imported_device_pks, "imported_vm_pks": imported_vm_pks, "imported_libre_device_ids": imported_libre_device_ids, "imported_libre_vm_ids": imported_libre_vm_ids, - "server_key": api.server_key, + "server_key": resolved_server_key, "total": total_count, "success_count": success_count, "failed_count": failed_count,Also applies to: 259-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/jobs.py` around lines 200 - 214, The code is using api.server_key directly which can leak a mocked object into bulk_import_devices_shared and job.data["server_key"]; call LibreNMSAPI(...) once and read a concrete value into a local resolved_server_key variable, then pass resolved_server_key to bulk_import_devices_shared (and any other call sites) and write resolved_server_key into job.data["server_key"] (also update the other occurrence around the bulk import at the later block that mirrors lines ~259-264) so the same concrete key is reused everywhere instead of api.server_key.netbox_librenms_plugin/librenms_api.py (1)
1047-1064:⚠️ Potential issue | 🟠 Major
vlansstill needs container and entry type guards.
vlan_entry.get("vlan")assumesvlansis a list of dicts. A payload like{"vlans": [None, "bad", {...}]}or a non-listvlansvalue still raises or suppresses theifVlanfallback. Normalizevlans_datato a list and skip non-dict entries before reading keys.Proposed fix
- vlans_data = port_data.get("vlans", []) + vlans_data = port_data.get("vlans", []) + if not isinstance(vlans_data, list): + vlans_data = [] untagged_vlan = None tagged_vlans = [] if vlans_data: # Parse from detailed vlans array for vlan_entry in vlans_data: + if not isinstance(vlan_entry, dict): + continue vlan_id = vlan_entry.get("vlan") if vlan_id is None: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/librenms_api.py` around lines 1047 - 1064, Normalize port_data.get("vlans") into a safe iterable and skip non-dict entries before accessing keys: ensure vlans_data is coerced to a list (e.g., empty list if None or not a list) and inside the loop check that each vlan_entry is an instance of dict before calling vlan_entry.get("vlan") or vlan_entry.get("untagged"); continue on non-dict or invalid vlan_id values so untagged_vlan and tagged_vlans logic remains correct (references: vlans_data, vlan_entry, untagged_vlan, tagged_vlans).
♻️ Duplicate comments (11)
docs/usage_tips/custom_field.md (1)
41-65:⚠️ Potential issue | 🟡 MinorDocument the actual JSON shape and update the manual-entry steps accordingly.
The type now says
JSON (object), but Line 41 still shows no concrete payload example, and the manual-entry section below still reads like users should enter a bare device ID. That keeps the docs aligned with the legacy format instead of the per-server mapping introduced by this PR.Suggested doc update
- - **Type:** JSON (object) — stores a per-server mapping, e.g. or + - **Type:** JSON (object) — stores a per-server mapping, e.g. `{"default": 123}` or `{"server-a": 123, "server-b": 456}` @@ - - Enter the LibreNMS device ID in the `librenms_id` field. + - Enter a JSON object in the `librenms_id` field, for example `{"default": 123}`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usage_tips/custom_field.md` around lines 41 - 65, Update the docs to show the concrete JSON shape for the librenms_id custom field (replace the vague "JSON (object)" example with a sample payload such as a per-server mapping like {"server.example.com": 123, "other-server": 456}) and change the "Manually assign a value to librenms_id" steps to instruct users to enter a JSON object in the Custom Fields input (with the example mapping and a note about keys being server identifiers and values being LibreNMS device IDs) so the manual-entry UI guidance matches the new per-server mapping format..devcontainer/README.md (1)
49-49:⚠️ Potential issue | 🟡 MinorFix the quick-start anchor target.
The fragment on Line 49 still does not match the generated anchor for
### 📡 LibreNMS Server Configuration, so this jump link is broken in the rendered README.📝 Suggested fix
-5. Create your plugin config — see [LibreNMS Server configuration](`#librenms-server-configuration`): +5. Create your plugin config — see [LibreNMS Server configuration](`#-librenms-server-configuration`):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/README.md at line 49, The jump link target is wrong: update the fragment in the "Create your plugin config — see [LibreNMS Server configuration](`#librenms-server-configuration`)" link to match the generated anchor for the header "### 📡 LibreNMS Server Configuration" by changing the fragment to include the emoji and hyphenation (use "#📡-librenms-server-configuration"); edit the link text occurrence (the bracketed/link target around "LibreNMS Server configuration") so it points to "#📡-librenms-server-configuration" to restore the broken quick-start anchor.netbox_librenms_plugin/tests/mock_librenms_server.py (1)
234-240:⚠️ Potential issue | 🟡 MinorMount the VC inventory handler on
/alltoo.The fallback branch on Lines 234-238 builds the combined inventory for unfiltered requests, but Line 240 only registers
_handleron/api/v0/inventory/{device_id}. Calls to/api/v0/inventory/{device_id}/allstill 404, so tests exercising the client-side fallback path never hit this branch.Suggested fix
- self.routes[f"/api/v0/inventory/{device_id}"] = _handler + self.routes[f"/api/v0/inventory/{device_id}"] = _handler + self.routes[f"/api/v0/inventory/{device_id}/all"] = _handler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/mock_librenms_server.py` around lines 234 - 240, The inventory handler builds a combined inventory for unfiltered requests but is only registered at "/api/v0/inventory/{device_id}", so requests to "/api/v0/inventory/{device_id}/all" 404; register the same _handler under the "/api/v0/inventory/{device_id}/all" route as well (add an entry to the routes dict with the key "/api/v0/inventory/{device_id}/all" mapping to _handler) so both paths hit the fallback branch.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (1)
633-634:⚠️ Potential issue | 🔴 CriticalStill unresolved: read
server_keyfrom the hidden input, not#current-server-key.Line 634 still uses an element ID that isn’t present on the sync forms, so this request posts
server_key: null. On multi-server installs that breaks the namespace for saved VLAN overrides.🔧 Suggested fix
- server_key: document.getElementById('current-server-key')?.value || null + server_key: document.querySelector('input[name="server_key"]')?.value || null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 633 - 634, The request is pulling server_key from a non-existent '#current-server-key' causing null values; update the assignment in the payload (where vid_group_map/vidGroupMap and server_key are set) to read the value from the hidden input used on the sync forms instead (the hidden input that carries the server_key, e.g. the input with name or id "server_key"), falling back to null if not present; ensure you replace document.getElementById('current-server-key') with a selector that targets that hidden input so multi-server namespace handling works correctly.netbox_librenms_plugin/forms.py (1)
549-564:⚠️ Potential issue | 🟠 MajorEmpty bound data is still treated as an option-only submission.
has_option_onlybecomes true for an empty bound dict becausenon_option_fieldsandhas_filtersare both false. That means the initial bound-form path still skipsdata["use_background_job"] = "on"and silently flips the default execution mode back to synchronous.🔧 Suggested fix
- has_option_only = not bool(non_option_fields) and not has_filters + has_option_only = ( + any(field in data for field in option_only_fields) + and not non_option_fields + and not has_filters + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/forms.py` around lines 549 - 564, The code treats an empty bound dict as an option-only submission because has_option_only is computed without checking if data is non-empty, which causes the initial-load default for use_background_job to be skipped; fix by ensuring has_option_only only becomes true when data is non-empty (e.g. compute has_option_only = bool(data) and not bool(non_option_fields) and not has_filters) and keep the existing default-apply check that sets data["use_background_job"] = "on" for initial loads when no real submission is present; update the logic around option_only_fields, non_option_fields and has_option_only to use this non-empty-data guard.netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
1572-1583:⚠️ Potential issue | 🟠 MajorTreat malformed JSON as a 400, not a 500.
This still hard-codes a server error for a body that fails JSON parsing. That makes bad client input indistinguishable from a real backend fault and will turn parser hardening into a test regression; once
post()catchesjson.JSONDecodeError, this should assert400instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_coverage_base_views2.py` around lines 1572 - 1583, The test test_exception_returns_500 currently expects a 500 when view.post sees malformed JSON; update it to expect a 400 instead so client JSON parse errors are treated as Bad Request. Specifically, change the assertion on response.status_code from 500 to 400 (the test invokes view.post and triggers json.loads failure), and keep verifying the JSON error payload (data["status"] == "error"); refer to the test method test_exception_returns_500 and the handler view.post/json.JSONDecodeError to locate the change.netbox_librenms_plugin/tests/test_coverage_device_fields.py (1)
674-698:⚠️ Potential issue | 🟡 MinorAssert the assignment half of the transaction.
This test only proves a success message was emitted. It would still pass if the platform was created but never attached to
mock_lockedor never saved. Add direct assertions on the assigned platform and the locked-device save.Proposed fix
with ( patch("netbox_librenms_plugin.views.sync.device_fields.get_object_or_404", return_value=MagicMock()), patch("netbox_librenms_plugin.views.sync.device_fields.Platform", mock_platform_cls), patch("netbox_librenms_plugin.views.sync.device_fields.Device", mock_device_cls), patch("netbox_librenms_plugin.views.sync.device_fields.transaction"), patch("netbox_librenms_plugin.views.sync.device_fields.messages") as mock_msg, patch("netbox_librenms_plugin.views.sync.device_fields.redirect"), ): view.post(req, pk=1) mock_msg.success.assert_called_once() + assert mock_locked.platform is mock_platform_instance + mock_locked.save.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py` around lines 674 - 698, Add assertions that the created Platform instance is assigned to the locked device and that the device was saved: after view.post(...) assert mock_locked.platform is mock_platform_instance (or compare to mock_platform_cls.objects.create.return_value if the test uses objects.create) and assert mock_locked.save.assert_called_once() to ensure the assignment and persistence occurred.netbox_librenms_plugin/tests/test_coverage_base_views.py (1)
1808-1809:⚠️ Potential issue | 🟠 MajorAssert the rendered template, not just the context dict.
mock_render.call_args[0][2]is the context argument. This never verifies thatpost()renderedpartial_template_name, so the test can pass while the wrong template is used.🔧 Suggested assertion update
- render_call_kwargs = mock_render.call_args[0] - assert "ip_sync" in render_call_kwargs[2] + render_args = mock_render.call_args[0] + assert render_args[1] == view.partial_template_name + assert render_args[2] is fake_ctx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_coverage_base_views.py` around lines 1808 - 1809, The test is only asserting the context from mock_render (mock_render.call_args[0][2]) and not that post() used the expected template; update the assertion to check the rendered template by inspecting mock_render.call_args[0][1] (the template name) and assert it equals partial_template_name, while keeping the existing context assertion for "ip_sync" so both template and context are verified for the post() call.netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1111-1161:⚠️ Potential issue | 🟡 MinorSplit the serial-conflict branch into its own test.
This block is still nested inside
test_serial_dash_normalized(). If the first half fails, the conflict path never runs, and the reported test name won’t tell you which scenario regressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_coverage_device_operations.py` around lines 1111 - 1161, The serial-conflict scenario nested inside test_serial_dash_normalized() should be extracted into its own unit test: create a new test function (e.g., test_serial_conflict()) that sets up the same fixtures/patches (existing MagicMock with serial "OLD_SN", conflict_device, libre_device with "NEW_SN", mock_device returning exclude().first() -> conflict_device), uses the same _find_side_effect and patches for find_by_librenms_id and Device, calls validate_device_for_import(libre_device, api=api), and asserts result.get("serial_action") == "conflict"; ensure you start/stop the same patches (via self._get_patches()) and remove the duplicated block from test_serial_dash_normalized so each scenario is independently reported and run.netbox_librenms_plugin/import_utils/device_operations.py (1)
57-63:⚠️ Potential issue | 🟠 MajorDon’t collapse an ambiguous chassis lookup into “no match”.
match_librenms_hardware_to_device_type()usesNoneas the duplicate-mapping sentinel. The helper drops that sentinel, and the caller only adopts fallback results whenmatchedis true, so an ambiguous chassis mapping still falls through to the generic “No matching device type found” issue instead of surfacing the configuration error.Based on learnings: In `netbox_librenms_plugin/utils.py`, `match_librenms_hardware_to_device_type` returns `None` when `DeviceTypeMapping.MultipleObjectsReturned` is raised, so callers must handle that sentinel separately.🛠️ Suggested fix
def _try_chassis_device_type_match(api, device_id): + ambiguous_match = None ... chassis_match = match_librenms_hardware_to_device_type(value) if chassis_match is None: - continue + ambiguous_match = { + "matched": False, + "device_type": None, + "match_type": "ambiguous", + "chassis_model": value, + } + continue if chassis_match["matched"]: chassis_match["match_type"] = "chassis" chassis_match["chassis_model"] = value return chassis_match + return ambiguous_match- if chassis_match and chassis_match["matched"]: - dt_match = chassis_match + if chassis_match is not None: + dt_match = chassis_matchAlso applies to: 548-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 57 - 63, The code currently treats the helper's duplicate-mapping sentinel (None) as "no match" by continuing; instead, when match_librenms_hardware_to_device_type(...) returns None you must propagate that sentinel so the caller can surface the ambiguous-mapping error — change the branch that checks chassis_match to return None when chassis_match is None (rather than continue), and apply the same fix to the other identical block that assigns to chassis_match elsewhere; keep the existing behavior of returning the mapping when chassis_match["matched"] is true.netbox_librenms_plugin/import_utils/bulk_import.py (1)
125-150:⚠️ Potential issue | 🟠 MajorCancellation status is not propagated to the caller.
The
breakstatements on lines 139 and 150 exit the loop but return the same result structure as a completed import. The caller (ImportDevicesJob.run()) cannot distinguish between a cancelled job and a successful partial completion. This may lead to recording a partial import as "completed" rather than "cancelled."This was flagged in a previous review. Consider returning an early result with a
cancelled: Trueflag:Suggested direction
if job.logger: job.logger.warning( f"Import job stopped at device {idx} of {total} (RQ status: {rq_job.get_status()})" ) else: logger.warning(f"Import cancelled at device {idx} of {total}") - break + return { + "total": total, + "success": success_list, + "failed": failed_list, + "skipped": skipped_list, + "virtual_chassis_created": vc_created_count, + "cancelled": True, + }Apply the same pattern to the DB-fallback branch (line 150), and have
ImportDevicesJob.run()checkresult.get("cancelled").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 125 - 150, When detecting cancellation in bulk_import.import_devices (the loop that checks RQ job status and the DB-fallback branch), stop using bare break and instead return an early result dict including cancelled: True (e.g., return {"cancelled": True, "processed": idx, ...} or merge with the existing result structure) so the caller can distinguish cancellation from normal completion; apply this change in both the RQ-check branch (where rq_job.is_failed or rq_job.is_stopped is detected) and the DB-fallback branch (where job.job.status indicates failure), and update ImportDevicesJob.run() to check result.get("cancelled") and handle cancelled runs appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/testing.md`:
- Line 49: Update the "Running Tests by Area" commands in
docs/development/testing.md to include the new VC integration suite by adding or
replacing the existing test selector that currently runs
test_integration_sync.py so it also runs test_integration_virtual_chassis.py (or
provide a separate area entry/command for the VC integration tests);
specifically update references to test_integration_sync.py and add
test_integration_virtual_chassis.py in the example commands and the list rows
(also apply the same change to the other occurrences noted around lines 93-94).
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 17-40: The metadata key generation in get_cache_metadata_key
currently replaces the readable filter segment with only a hash; instead
preserve a readable fragment derived from the non-None filter items (e.g.,
canonicalize sorted filters into "k=v" tokens joined by "_" or another existing
separator) and append the existing filter_hash for stability, so the key still
contains human-readable tokens like "location=DC1" while retaining the hash
suffix; update the return to include server_key, the readable filter fragment,
the filter_hash, vc_enabled, and the sysname/strip flags (refer to
get_cache_metadata_key, filter_hash, and filters) and ensure any long values are
safely truncated/normalized to avoid overly long keys so existing callers/tests
remain compatible.
In `@netbox_librenms_plugin/import_utils/virtual_chassis.py`:
- Around line 367-369: The VC domain is currently built only from LibreNMS
device_id which can collide across servers; update
create_virtual_chassis_with_members to accept a server_key argument (e.g.,
server_key: str) and use that when composing the persisted domain (include
server_key together with libre_device['device_id'] or the existing domain logic)
so saved domains are namespaced per server; apply the same change to the other
helper referenced in the comment (the function around lines 434-435) so both
functions thread and persist the server_key-scoped domain consistently.
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 211-240: The code stores and caches librenms_id even when int()
conversion fails, which can poison _store_librenms_id with malformed values;
update the IP, DNS-name and hostname branches (where get_device_id_by_ip and
get_device_id_by_hostname are called) to only call _store_librenms_id and return
when the librenms_id successfully converts to int — if int(...) raises
ValueError or TypeError, skip storing that candidate and continue to the next
fallback instead.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 547-620: applyButtonUpdates is using vidCssMap and vidMissingMap
values that may still be pending from async verification, producing stale
tooltip/summary; modify the Save flow so you do not call applyButtonUpdates
until all in-flight verification requests complete (e.g. await the verification
promise(s) or check a pending-verification flag/collection and await those
promises) or disable/block the Save action while verification is running; ensure
the caller that triggers applyButtonUpdates waits for the verification
promise(s) to resolve (or checks a verified flag) before invoking
applyButtonUpdates so v.missing/v.css reflect the verified state.
- Around line 1558-1577: The pending preview fetch can overwrite the shared
modal after it's closed; modify closeHtmxModal() to call
_activeReplaceController.abort() if present and then set
_activeReplaceController = null; also ensure the code path that starts the fetch
stores the AbortController in _activeReplaceController and clears
_activeReplaceController when the request settles (in the .then/.catch/.finally
handlers) so late responses don't repaint `#htmx-modal-body`.
In `@netbox_librenms_plugin/tests/test_background_jobs.py`:
- Around line 475-477: The test currently asserts that
bulk_import_devices_shared received server_key=="default" but the mocked
LibreNMSAPI() returns a bare MagicMock whose .server_key is another mock, so the
assertion is meaningless; update the test to set the mocked API's server_key
explicitly (e.g., configure the MagicMock returned by LibreNMSAPI() so
return_value.server_key == "default") before calling ImportDevicesJob.run(), and
make the same change to the adjacent ImportDevicesJob tests that use a bare
MagicMock so they also set return_value.server_key to the expected namespace;
ensure you still read the called kwargs from mock_bulk_devices.call_args[1] to
assert the forwarded value.
In `@netbox_librenms_plugin/tests/test_cable_verify.py`:
- Around line 76-83: The fake_process_remote_device used in the test overwrites
netbox_remote_* and URL fields unconditionally, so update the test to ensure
stale cached IDs/URLs are removed before re-enrichment: modify
fake_process_remote_device (or add assertions before it sets new values) to
assert that the incoming link does NOT contain netbox_remote_device_id,
netbox_remote_interface_id, remote_device_url, or remote_port_url (and possibly
remote_port_name) coming from cache, then set the fresh IDs/URLs as before; this
will force SingleCableVerifyView.post() (and the call site
process_remote_device) to clear stale cache-derived fields before re-enriching.
In `@netbox_librenms_plugin/tests/test_coverage_device_operations.py`:
- Around line 1594-1643: validate_device_for_import is still invoking unrelated
helpers (find_matching_site, find_matching_platform and the virtual-chassis
detection helper) so this test can fail for reasons outside the chassis-override
branch; patch/stub those helpers in the test the same way other helpers are
patched: add patches for
netbox_librenms_plugin.import_utils.device_operations.find_matching_site and
.find_matching_platform to return controlled neutral values (e.g. None or a
simple MagicMock) and patch the VC-detection helper used by
validate_device_for_import (the function responsible for virtual chassis
detection) to return a non-VC result; ensure these new patches are
started/stopped with the existing patches so the assertion only exercises the
chassis override logic.
In `@netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py`:
- Around line 264-281: The test is using the same server_key for both the posted
form and the mocked librenms_api, so it won't catch cases where
SyncInterfacesView.post ignores the request-scoped server key; update the test
to submit a distinct POST server_key (e.g., "posted_key") while keeping
librenms_api.server_key as a different value (e.g., "default"), then assert that
SyncInterfacesView.post reads request.POST["server_key"] and assigns it to
self._post_server_key (or passes it into get_cache_key) so cache lookups use the
per-request namespace; specifically change the POST data in the test and add an
assertion that view.get_cache_key (or the mocked get_cache_key on
view.__class__) was called with the posted key or that view._post_server_key
equals the posted key after view.post(req, ...).
---
Outside diff comments:
In `@netbox_librenms_plugin/jobs.py`:
- Around line 200-214: The code is using api.server_key directly which can leak
a mocked object into bulk_import_devices_shared and job.data["server_key"]; call
LibreNMSAPI(...) once and read a concrete value into a local resolved_server_key
variable, then pass resolved_server_key to bulk_import_devices_shared (and any
other call sites) and write resolved_server_key into job.data["server_key"]
(also update the other occurrence around the bulk import at the later block that
mirrors lines ~259-264) so the same concrete key is reused everywhere instead of
api.server_key.
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 1047-1064: Normalize port_data.get("vlans") into a safe iterable
and skip non-dict entries before accessing keys: ensure vlans_data is coerced to
a list (e.g., empty list if None or not a list) and inside the loop check that
each vlan_entry is an instance of dict before calling vlan_entry.get("vlan") or
vlan_entry.get("untagged"); continue on non-dict or invalid vlan_id values so
untagged_vlan and tagged_vlans logic remains correct (references: vlans_data,
vlan_entry, untagged_vlan, tagged_vlans).
---
Duplicate comments:
In @.devcontainer/README.md:
- Line 49: The jump link target is wrong: update the fragment in the "Create
your plugin config — see [LibreNMS Server
configuration](`#librenms-server-configuration`)" link to match the generated
anchor for the header "### 📡 LibreNMS Server Configuration" by changing the
fragment to include the emoji and hyphenation (use
"#📡-librenms-server-configuration"); edit the link text occurrence (the
bracketed/link target around "LibreNMS Server configuration") so it points to
"#📡-librenms-server-configuration" to restore the broken quick-start anchor.
In `@docs/usage_tips/custom_field.md`:
- Around line 41-65: Update the docs to show the concrete JSON shape for the
librenms_id custom field (replace the vague "JSON (object)" example with a
sample payload such as a per-server mapping like {"server.example.com": 123,
"other-server": 456}) and change the "Manually assign a value to librenms_id"
steps to instruct users to enter a JSON object in the Custom Fields input (with
the example mapping and a note about keys being server identifiers and values
being LibreNMS device IDs) so the manual-entry UI guidance matches the new
per-server mapping format.
In `@netbox_librenms_plugin/forms.py`:
- Around line 549-564: The code treats an empty bound dict as an option-only
submission because has_option_only is computed without checking if data is
non-empty, which causes the initial-load default for use_background_job to be
skipped; fix by ensuring has_option_only only becomes true when data is
non-empty (e.g. compute has_option_only = bool(data) and not
bool(non_option_fields) and not has_filters) and keep the existing default-apply
check that sets data["use_background_job"] = "on" for initial loads when no real
submission is present; update the logic around option_only_fields,
non_option_fields and has_option_only to use this non-empty-data guard.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 125-150: When detecting cancellation in bulk_import.import_devices
(the loop that checks RQ job status and the DB-fallback branch), stop using bare
break and instead return an early result dict including cancelled: True (e.g.,
return {"cancelled": True, "processed": idx, ...} or merge with the existing
result structure) so the caller can distinguish cancellation from normal
completion; apply this change in both the RQ-check branch (where
rq_job.is_failed or rq_job.is_stopped is detected) and the DB-fallback branch
(where job.job.status indicates failure), and update ImportDevicesJob.run() to
check result.get("cancelled") and handle cancelled runs appropriately.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 57-63: The code currently treats the helper's duplicate-mapping
sentinel (None) as "no match" by continuing; instead, when
match_librenms_hardware_to_device_type(...) returns None you must propagate that
sentinel so the caller can surface the ambiguous-mapping error — change the
branch that checks chassis_match to return None when chassis_match is None
(rather than continue), and apply the same fix to the other identical block that
assigns to chassis_match elsewhere; keep the existing behavior of returning the
mapping when chassis_match["matched"] is true.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 633-634: The request is pulling server_key from a non-existent
'#current-server-key' causing null values; update the assignment in the payload
(where vid_group_map/vidGroupMap and server_key are set) to read the value from
the hidden input used on the sync forms instead (the hidden input that carries
the server_key, e.g. the input with name or id "server_key"), falling back to
null if not present; ensure you replace
document.getElementById('current-server-key') with a selector that targets that
hidden input so multi-server namespace handling works correctly.
In `@netbox_librenms_plugin/tests/mock_librenms_server.py`:
- Around line 234-240: The inventory handler builds a combined inventory for
unfiltered requests but is only registered at "/api/v0/inventory/{device_id}",
so requests to "/api/v0/inventory/{device_id}/all" 404; register the same
_handler under the "/api/v0/inventory/{device_id}/all" route as well (add an
entry to the routes dict with the key "/api/v0/inventory/{device_id}/all"
mapping to _handler) so both paths hit the fallback branch.
In `@netbox_librenms_plugin/tests/test_coverage_base_views.py`:
- Around line 1808-1809: The test is only asserting the context from mock_render
(mock_render.call_args[0][2]) and not that post() used the expected template;
update the assertion to check the rendered template by inspecting
mock_render.call_args[0][1] (the template name) and assert it equals
partial_template_name, while keeping the existing context assertion for
"ip_sync" so both template and context are verified for the post() call.
In `@netbox_librenms_plugin/tests/test_coverage_base_views2.py`:
- Around line 1572-1583: The test test_exception_returns_500 currently expects a
500 when view.post sees malformed JSON; update it to expect a 400 instead so
client JSON parse errors are treated as Bad Request. Specifically, change the
assertion on response.status_code from 500 to 400 (the test invokes view.post
and triggers json.loads failure), and keep verifying the JSON error payload
(data["status"] == "error"); refer to the test method test_exception_returns_500
and the handler view.post/json.JSONDecodeError to locate the change.
In `@netbox_librenms_plugin/tests/test_coverage_device_fields.py`:
- Around line 674-698: Add assertions that the created Platform instance is
assigned to the locked device and that the device was saved: after
view.post(...) assert mock_locked.platform is mock_platform_instance (or compare
to mock_platform_cls.objects.create.return_value if the test uses
objects.create) and assert mock_locked.save.assert_called_once() to ensure the
assignment and persistence occurred.
In `@netbox_librenms_plugin/tests/test_coverage_device_operations.py`:
- Around line 1111-1161: The serial-conflict scenario nested inside
test_serial_dash_normalized() should be extracted into its own unit test: create
a new test function (e.g., test_serial_conflict()) that sets up the same
fixtures/patches (existing MagicMock with serial "OLD_SN", conflict_device,
libre_device with "NEW_SN", mock_device returning exclude().first() ->
conflict_device), uses the same _find_side_effect and patches for
find_by_librenms_id and Device, calls validate_device_for_import(libre_device,
api=api), and asserts result.get("serial_action") == "conflict"; ensure you
start/stop the same patches (via self._get_patches()) and remove the duplicated
block from test_serial_dash_normalized so each scenario is independently
reported and run.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45174a9d-b5b6-4f1b-b3b1-b38b2524ed62
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (87)
.devcontainer/README.md.github/pull_request_template.md.pre-commit-config.yamldocs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/vlans.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_cable_verify.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_api.pynetbox_librenms_plugin/tests/test_coverage_api2.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_cache.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_filters.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_sync_views3.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_coverage_vlans_table.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_integration_virtual_chassis.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_interfaces.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vm_operations.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/object_sync/vms.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/vlans.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/usage_tips/custom_field.md (1)
70-72: 🧹 Nitpick | 🔵 TrivialClarify the expected format for manual entry.
The instruction "Enter the LibreNMS device ID in the
librenms_idfield" doesn't specify whether users should enter a bare integer (42) or the JSON dict format ({"default": 42}). Since the field Type is JSON (object), users should be guided to:
- Enter
{"server_key": device_id}for the recommended multi-server format (e.g.,{"default": 42})- Note that bare integers are supported for backward compatibility but not recommended for new entries
📝 Suggested clarification
Replace line 72 with:
- - Enter the LibreNMS device ID in the `librenms_id` field. + - Enter the LibreNMS device ID in the `librenms_id` field using JSON format. + - For single-server: `{"default": 42}` (replace `42` with your LibreNMS device ID) + - For multi-server: `{"production": 42, "staging": 17}` (replace with your server keys and device IDs) + - Legacy bare integers (e.g., `42`) are also supported for backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usage_tips/custom_field.md` around lines 70 - 72, Update the instruction for the `librenms_id` custom field to clarify expected JSON object format: state that because the field Type is JSON (object) users should enter a server-keyed object like {"server_key": device_id} (recommended), show the common example {"default": 42}, and add a brief note that bare integers (e.g., 42) are still accepted for backward compatibility but are not recommended for new entries; target the sentence referencing the `librenms_id` field in docs/usage_tips/custom_field.md and replace it with this clarified guidance.
♻️ Duplicate comments (5)
docs/development/testing.md (1)
93-94:⚠️ Potential issue | 🟡 MinorInclude the VC integration suite in the integration test command.
The table now documents
test_integration_virtual_chassis.py, but this runnable example still executes onlytest_integration_sync.py, so following the guide misses part of the integration coverage.📝 Suggested doc update
# Integration tests (API client against mock HTTP server) -pytest netbox_librenms_plugin/tests/test_integration_sync.py -v +pytest netbox_librenms_plugin/tests/test_integration_sync.py \ + netbox_librenms_plugin/tests/test_integration_virtual_chassis.py -v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/testing.md` around lines 93 - 94, Update the runnable pytest example so it runs both integration suites by including test_integration_virtual_chassis.py (or a glob pattern matching both tests) instead of only test_integration_sync.py; modify the command string shown (the example invoking pytest) to reference test_integration_virtual_chassis.py or use something like both filenames or a pattern (e.g., test_integration_*.py) so the documented command matches the table and actually executes the VC integration suite.netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (2)
1558-1577:⚠️ Potential issue | 🟠 MajorGuard the shared modal body against late preview responses.
closeHtmxModal()aborts the current controller, but that only helps while the request is still pending. If the preview has already moved intoresponse.text()or the success callback queue, Lines 1558-1561 will still overwrite#htmx-modal-bodyafter the modal was closed or another preview started, which can clobber the shared wrapper. Check that this request is still the active one before injecting HTML, and clear_activeReplaceControllerinfinallywhen it settles.Suggested fix
fetch(`${previewUrl}?${params.toString()}`, { signal, headers: { 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value }, }) .then(response => { if (!response.ok) return response.text().then(t => { throw new Error(t); }); return response.text(); }) .then(html => { + if (signal.aborted || _activeReplaceController?.signal !== signal) { + return; + } const modalBody = document.getElementById('htmx-modal-body'); if (modalBody) { modalBody.innerHTML = html; } }) .catch(err => { if (err.name === 'AbortError') return; // Superseded by a newer click — ignore const modalBody = document.getElementById('htmx-modal-body'); if (modalBody) { const alert = document.createElement('div'); alert.className = 'alert alert-danger'; const icon = document.createElement('i'); icon.className = 'mdi mdi-alert me-1'; alert.appendChild(icon); alert.appendChild(document.createTextNode(err.message || 'Failed to load preview.')); modalBody.textContent = ''; modalBody.appendChild(alert); } - }); + }) + .finally(() => { + if (_activeReplaceController?.signal === signal) { + _activeReplaceController = null; + } + });Based on learnings, the module replace flow reuses the shared
#htmx-modal-contentwrapper.Also applies to: 1582-1597
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 1558 - 1577, This response is overwriting the shared modal after the request may have been superseded; for the fetch that produces html, capture its AbortController in a local variable (e.g., const controller = _activeReplaceController) and before mutating the shared '#htmx-modal-body' check that _activeReplaceController === controller (return early if not); do the same guard in the catch branch before appending the error alert; finally, when the promise settles (in a finally block), clear _activeReplaceController if it still equals that controller so late handlers won't affect future previews (reference _activeReplaceController, closeHtmxModal(), and the '#htmx-modal-body'/'#htmx-modal-content' DOM updates).
547-620:⚠️ Potential issue | 🟡 MinorWait for pending VLAN verification before recomputing the saved summary.
If Save is clicked while
verifyVlanInGroup()is still in flight,vidCssMap/vidMissingMapstay empty and this path falls back to stalev.css/v.missingvalues when rebuildinggroup_name, tooltips, and inline badges. The override is saved, but the row summary can still describe the previous validation result until the modal is reopened. Block Save while rows are verifying, or await the outstanding verification promises before callingapplyButtonUpdates().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 547 - 620, The save flow can run applyButtonUpdates() before pending verifyVlanInGroup() calls populate vidCssMap/vidMissingMap, causing stale UI; modify the save handler to await outstanding verification promises (e.g. maintain an array like pendingVerifications that verifyVlanInGroup() pushes its Promise into and removes on settle) and either disable the Save button while pending or await Promise.all(pendingVerifications) before calling applyButtonUpdates() so vidCssMap/vidMissingMap are up-to-date when group_name, tooltip and inline summary are recomputed.netbox_librenms_plugin/import_utils/cache.py (1)
178-188:⚠️ Potential issue | 🟠 MajorKeep the legacy default on
get_import_device_cache_key().Making
server_keymandatory here is a backwards-incompatible change for any remaining one-argument callers of this shared helper. The compatibility default was explicitly deferred, so this PR should keep the"default"fallback.🔧 Suggested fix
-def get_import_device_cache_key(device_id: int | str, server_key: str) -> str: +def get_import_device_cache_key(device_id: int | str, server_key: str = "default") -> str:Based on learnings: In
netbox_librenms_plugin/import_utils/cache.py,get_import_device_cache_key(device_id, server_key="default")retains its"default"default intentionally, and removing it was deferred to a follow-up hardening PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/cache.py` around lines 178 - 188, The function get_import_device_cache_key removed the backwards-compatible default for server_key; restore the legacy default by changing the signature of get_import_device_cache_key to accept server_key with a default value of "default" so one-argument callers continue to work, and ensure any internal references still treat server_key as a str (e.g., in cache key composition) without changing call sites.netbox_librenms_plugin/import_utils/bulk_import.py (1)
235-247:⚠️ Potential issue | 🟠 MajorMake the serial-less VC fallback key stack-wide.
The fallback still mixes the current
device_idinto the fingerprint, so two members of the same serial-less stack compute differentvc_domainvalues. That defeatsprocessed_vc_domainsand can triggercreate_virtual_chassis_with_members()once per member.🔧 Suggested fix
- fingerprint = hashlib.md5((f"{device_id}," + ",".join(member_parts)).encode()).hexdigest()[ + fingerprint = hashlib.md5(",".join(member_parts).encode()).hexdigest()[ :12 ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 235 - 247, The fallback fingerprint for serial-less VC members incorrectly includes device_id so each member gets a different vc_domain; update the logic in bulk_import.py (the block that builds member_parts, computes fingerprint, and sets vc_domain) to compute the MD5 only from the sorted member_parts (and any stable constant if desired) so all members in vc_data["members"] produce the same fingerprint/ vc_domain; ensure processed_vc_domains and create_virtual_chassis_with_members() then see a single shared vc_domain for the whole stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 943-956: The code redundantly checks isinstance(result, dict)
multiple times after it's already confirmed; simplify by removing those extra
checks and use result.get(...) directly: assign all_vlans = result.get("vlans"),
set msg = result.get("message"), and keep the VLAN filtering into device_vlans
and the existing type guards for each VLAN item (isinstance(v, dict)) unchanged;
ensure the same return values and error messages remain.
- Around line 790-800: The code redundantly re-checks isinstance(data, dict)
after already confirming it; simplify the block by using the confirmed local
variable 'data' directly when extracting 'inventory' and 'message' and when
calling logger.debug; update the logic inside the function that handles the
response (the variables 'data', 'inventory', and 'params' are the targets) to
remove the repeated isinstance(data, dict) checks and rely on the initial check
that data is a dict before accessing data.get(...).
---
Outside diff comments:
In `@docs/usage_tips/custom_field.md`:
- Around line 70-72: Update the instruction for the `librenms_id` custom field
to clarify expected JSON object format: state that because the field Type is
JSON (object) users should enter a server-keyed object like {"server_key":
device_id} (recommended), show the common example {"default": 42}, and add a
brief note that bare integers (e.g., 42) are still accepted for backward
compatibility but are not recommended for new entries; target the sentence
referencing the `librenms_id` field in docs/usage_tips/custom_field.md and
replace it with this clarified guidance.
---
Duplicate comments:
In `@docs/development/testing.md`:
- Around line 93-94: Update the runnable pytest example so it runs both
integration suites by including test_integration_virtual_chassis.py (or a glob
pattern matching both tests) instead of only test_integration_sync.py; modify
the command string shown (the example invoking pytest) to reference
test_integration_virtual_chassis.py or use something like both filenames or a
pattern (e.g., test_integration_*.py) so the documented command matches the
table and actually executes the VC integration suite.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 235-247: The fallback fingerprint for serial-less VC members
incorrectly includes device_id so each member gets a different vc_domain; update
the logic in bulk_import.py (the block that builds member_parts, computes
fingerprint, and sets vc_domain) to compute the MD5 only from the sorted
member_parts (and any stable constant if desired) so all members in
vc_data["members"] produce the same fingerprint/ vc_domain; ensure
processed_vc_domains and create_virtual_chassis_with_members() then see a single
shared vc_domain for the whole stack.
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 178-188: The function get_import_device_cache_key removed the
backwards-compatible default for server_key; restore the legacy default by
changing the signature of get_import_device_cache_key to accept server_key with
a default value of "default" so one-argument callers continue to work, and
ensure any internal references still treat server_key as a str (e.g., in cache
key composition) without changing call sites.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1558-1577: This response is overwriting the shared modal after the
request may have been superseded; for the fetch that produces html, capture its
AbortController in a local variable (e.g., const controller =
_activeReplaceController) and before mutating the shared '#htmx-modal-body'
check that _activeReplaceController === controller (return early if not); do the
same guard in the catch branch before appending the error alert; finally, when
the promise settles (in a finally block), clear _activeReplaceController if it
still equals that controller so late handlers won't affect future previews
(reference _activeReplaceController, closeHtmxModal(), and the
'#htmx-modal-body'/'#htmx-modal-content' DOM updates).
- Around line 547-620: The save flow can run applyButtonUpdates() before pending
verifyVlanInGroup() calls populate vidCssMap/vidMissingMap, causing stale UI;
modify the save handler to await outstanding verification promises (e.g.
maintain an array like pendingVerifications that verifyVlanInGroup() pushes its
Promise into and removes on settle) and either disable the Save button while
pending or await Promise.all(pendingVerifications) before calling
applyButtonUpdates() so vidCssMap/vidMissingMap are up-to-date when group_name,
tooltip and inline summary are recomputed.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd45923a-90a8-47f4-b86a-09a8e0bb99aa
📒 Files selected for processing (16)
docs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_coverage_cache.pynetbox_librenms_plugin/tests/test_coverage_forms.pynetbox_librenms_plugin/tests/test_coverage_virtual_chassis.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/views/sync/cables.py
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 12, 2026 08:18
7305b86 to
1b24785
Compare
left a comment
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/views/sync/cables.py (1)
98-105:⚠️ Potential issue | 🟠 MajorApply the posted VC member override before creating the cable.
Line 104 only copies the selected
device_idintolink_data;handle_cable_creation()still uses the cachednetbox_local_interface_id. If the user changesdevice_selection_<port_id>, this path will still cable the originally cached interface/member instead of the one they selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/sync/cables.py` around lines 98 - 105, process_single_interface currently copies the posted device override into link_data.device_id but does not update the cached netbox_local_interface_id, so handle_cable_creation still uses the original cached interface/member; update link_data to also override "netbox_local_interface_id" (and any related local interface identifiers used by handle_cable_creation) from the incoming interface (e.g., use interface.get("netbox_local_interface_id", link_data.get("netbox_local_interface_id"))), then call handle_cable_creation with the updated link_data and interface to ensure the selected VC member is used.
♻️ Duplicate comments (9)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (1)
547-663:⚠️ Potential issue | 🟡 MinorWait for pending VLAN re-verification before re-rendering the saved summary.
applyButtonUpdates()still depends onrow.dataset.resolvedCss/resolvedMissing, but Save can run before the asyncverifyVlanInGroup()call finishes. In that case the override is saved, yet the inline summary/tooltip is rebuilt from the oldv.css/v.missingstate until the modal is reopened. Please block Save while verification is in flight, or await those verification promises before callingapplyButtonUpdates().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 547 - 663, The save path calls applyButtonUpdates() while async verification may still be running (verifyVlanInGroup updates vidCssMap/vidMissingMap and row.dataset.resolvedCss), causing stale summary rendering; fix by awaiting pending verification promises before calling applyButtonUpdates(): track verification promises for each button/VID when verifyVlanInGroup is invoked (e.g., store them in a Map or array like pendingVerifications), ensure the Save handler (the branch that posts or the else path) awaits Promise.all(pendingVerifications) (or rejects if any fail) and disable the Save control (modalEl save button) while awaiting so applyButtonUpdates() sees the final vidCssMap/vidMissingMap state; reference applyButtonUpdates, verifyVlanInGroup, buttonsToUpdate, vidCssMap, vidMissingMap, modalEl and pendingVerifications in your changes.docs/usage_tips/custom_field.md (1)
41-49: 🧹 Nitpick | 🔵 TrivialGood addition of JSON examples; consider aligning manual-entry guidance.
The multi-server and legacy examples are helpful. However, the "Manually assign a value" section (lines 60-76) still instructs users to "Enter the LibreNMS device ID in the
librenms_idfield" without clarifying the JSON object format needed for multi-server setups.Consider adding a brief note or example in that section showing users should enter JSON like
{"default": 42}when using the new format, to prevent confusion.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usage_tips/custom_field.md` around lines 41 - 49, Update the "Manually assign a value" section to clarify that the new format expects a JSON object for multi-server setups and give a concrete example; specifically mention the librenms_id field should be set as a JSON object (e.g., {"default": 42} or {"production": 42, "staging": 17}) rather than a bare integer, so users understand how to enter values when using the new multi-server mapping format..devcontainer/README.md (1)
49-49:⚠️ Potential issue | 🟡 MinorFix the quick-start jump link target.
This fragment still doesn't match the rendered slug for
### 📡 LibreNMS Server Configuration, so the quick-start link is broken.📝 Suggested fix
-5. Create your plugin config — see [LibreNMS Server configuration](`#librenms-server-configuration`): +5. Create your plugin config — see [LibreNMS Server configuration](`#-librenms-server-configuration`):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/README.md at line 49, Update the quick-start link so its anchor exactly matches the rendered slug of the header "### 📡 LibreNMS Server Configuration": open the README, locate the link text "LibreNMS Server configuration" and replace its current anchor with the header's actual GitHub-rendered slug (compute the slug by lowercasing the header, replacing spaces with hyphens and applying GitHub slug rules—remove/normalize the emoji if necessary) so the link target matches the "### 📡 LibreNMS Server Configuration" heading.netbox_librenms_plugin/tests/mock_librenms_server.py (1)
234-240:⚠️ Potential issue | 🟡 MinorRegister the VC inventory handler on
/allas well.The fallback branch above is meant to serve the unfiltered inventory response, but only
/api/v0/inventory/{device_id}is mounted here.get_device_inventory()uses/api/v0/inventory/{device_id}/allfor that path, so tests relying on the fallback will still 404.📝 Suggested fix
- self.routes[f"/api/v0/inventory/{device_id}"] = _handler + self.routes[f"/api/v0/inventory/{device_id}"] = _handler + self.routes[f"/api/v0/inventory/{device_id}/all"] = _handler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/mock_librenms_server.py` around lines 234 - 240, The handler _handler is only mounted at f"/api/v0/inventory/{device_id}" but get_device_inventory() expects the unfiltered fallback at f"/api/v0/inventory/{device_id}/all"; register the same _handler for the "/all" path as well by adding a second entry to self.routes keyed with f"/api/v0/inventory/{device_id}/all" (pointing to _handler) so both routes return the unfiltered inventory.docs/development/testing.md (1)
93-94:⚠️ Potential issue | 🟡 MinorInclude the VC integration suite in the example command.
The table now documents
test_integration_virtual_chassis.py, but this "Integration tests" command still runs onlytest_integration_sync.py, so following the guide misses the new coverage.📝 Suggested fix
-# Integration tests (API client against mock HTTP server) -pytest netbox_librenms_plugin/tests/test_integration_sync.py -v +# Integration tests (API client against mock HTTP server) +pytest netbox_librenms_plugin/tests/test_integration_sync.py \ + netbox_librenms_plugin/tests/test_integration_virtual_chassis.py -v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/testing.md` around lines 93 - 94, The example "Integration tests" command only runs test_integration_sync.py and must include the VC suite; update the command in testing.md to run both integration tests (test_integration_sync.py and test_integration_virtual_chassis.py) or use a pattern (e.g., pytest netbox_librenms_plugin/tests/test_integration_*.py -v) so the virtual chassis test (test_integration_virtual_chassis.py) is executed alongside test_integration_sync.py.netbox_librenms_plugin/import_utils/virtual_chassis.py (1)
367-369:⚠️ Potential issue | 🟠 MajorMake
server_keymandatory for persisted VC domains.The collision fix only works when callers pass
server_key. With the current=Nonedefault, any missed call site silently falls back tolibrenms-<device_id>and can still collide across LibreNMS servers.#!/bin/bash # Find any call site that still relies on the old 3-argument form. python - <<'PY' import ast import pathlib for path in pathlib.Path(".").rglob("*.py"): try: tree = ast.parse(path.read_text()) except Exception: continue for node in ast.walk(tree): if not isinstance(node, ast.Call): continue func = node.func name = ( func.id if isinstance(func, ast.Name) else func.attr if isinstance(func, ast.Attribute) else None ) if name != "create_virtual_chassis_with_members": continue has_server_key_kw = any(kw.arg == "server_key" for kw in node.keywords if kw.arg) if len(node.args) < 4 and not has_server_key_kw: print(f"{path}:{node.lineno}: missing server_key") PYAlso applies to: 431-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/virtual_chassis.py` around lines 367 - 369, The function create_virtual_chassis_with_members currently defaults server_key to None which allows silent fallback and collisions; change its signature to require server_key (remove the = None default) and do the same for the related function(s) around lines 431-436 (e.g., create_virtual_chassis or any helper that persists VC domains) so callers must pass server_key; update all call sites to supply a server_key (or propagate it from higher-level functions) and run the provided AST check to find and fix any remaining 3-argument calls to create_virtual_chassis_with_members.netbox_librenms_plugin/import_utils/cache.py (1)
178-188:⚠️ Potential issue | 🟠 MajorRestore the temporary default on
get_import_device_cache_key().Making
server_keymandatory here turns any remaining one-argument call into aTypeError. This helper was intentionally kept backward-compatible during the multi-server rollout.Based on learnings: In `netbox_librenms_plugin/import_utils/cache.py`, `get_import_device_cache_key(device_id, server_key="default")` retains its `"default"` default intentionally, and removing it was deferred to a follow-up hardening PR.Suggested fix
-def get_import_device_cache_key(device_id: int | str, server_key: str) -> str: +def get_import_device_cache_key(device_id: int | str, server_key: str = "default") -> str:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/cache.py` around lines 178 - 188, The function get_import_device_cache_key currently requires both device_id and server_key causing existing one-argument calls to break; revert the signature to include the backward-compatible default by changing server_key to have a default of "default" (i.e., get_import_device_cache_key(device_id, server_key="default")), and ensure callers that relied on the single-argument behavior continue to work without modification.netbox_librenms_plugin/import_utils/bulk_import.py (1)
242-245:⚠️ Potential issue | 🟠 MajorMake the serial-less VC dedup key stack-wide.
Line 242 still salts the fallback fingerprint with the current
device_id. Two members of the same serial-less stack will therefore compute differentvc_domainvalues, soprocessed_vc_domainswill not suppress duplicate VC creation attempts.🛠️ Suggested fix
- fingerprint = hashlib.md5((f"{device_id}," + ",".join(member_parts)).encode()).hexdigest()[ - :12 - ] + fingerprint = hashlib.md5(",".join(member_parts).encode()).hexdigest()[:12]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/bulk_import.py` around lines 242 - 245, The dedup key for serial-less VC stacks is being salted with the local device_id when computing fingerprint (see variables fingerprint, device_id, member_parts, vc_domain, processed_vc_domains), which causes different members of the same stack to produce different vc_domain values; remove device_id from the hash input and compute the fingerprint only from the stack members (e.g., join a deterministic ordering of member_parts such as sorted(member_parts) or unique set) so the resulting vc_domain is identical across all members and processed_vc_domains can suppress duplicates.netbox_librenms_plugin/import_utils/device_operations.py (1)
57-59:⚠️ Potential issue | 🟡 MinorPreserve ambiguous chassis matches instead of treating them as misses.
Lines 57-59 drop the
Nonesentinel thatmatch_librenms_hardware_to_device_type()uses for duplicateDeviceTypeMappingrows. Because Lines 548-553 only acceptmatched=True, an ambiguous chassis lookup now falls through to the generic “No matching device type found...” path instead of surfacing the mapping ambiguity that needs cleanup.🛠️ Suggested direction
- if chassis_match is None: - continue + if chassis_match is None: + return { + "matched": False, + "device_type": None, + "match_type": "ambiguous", + "chassis_model": value, + } ... - if chassis_match and chassis_match["matched"]: - dt_match = chassis_match + if chassis_match: + dt_match = chassis_matchBased on learnings: In
netbox_librenms_plugin/utils.py,match_librenms_hardware_to_device_typereturnsNone(not a dict) whenDeviceTypeMapping.MultipleObjectsReturnedis raised — callers must guardif result is Noneseparately from the normalif not result["matched"]check.Also applies to: 548-553
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 57 - 59, The code currently treats a None return from match_librenms_hardware_to_device_type as a simple miss and continues, which hides the sentinel used to indicate ambiguous/duplicate DeviceTypeMapping rows; change callers (the block around chassis_match and the later check at the location handling matched results) to explicitly test for None first and surface/handle it (e.g., log or return an ambiguity error) before performing the normal if not chassis_match["matched"] path — specifically update the usage of match_librenms_hardware_to_device_type and the conditional logic that checks matched to distinguish three states: None (ambiguous/multiple), dict with matched=True, and dict with matched=False.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 74-83: Currently VirtualMachine.objects.create(...) inserts the
row and then set_librenms_device_id(...) mutates fields causing vm.save() to
issue a second UPDATE; instead instantiate the model with VirtualMachine(...) to
set initial fields, call set_librenms_device_id(vm, librenms_device_id,
server_key) to apply the custom-field mutation on the in-memory instance, and
then call vm.save() inside the same transaction.atomic() so the DB sees a single
INSERT; modify the block around VirtualMachine.objects.create, replacing the
create call with an instance constructor (VirtualMachine(...)) before calling
set_librenms_device_id and vm.save().
In `@netbox_librenms_plugin/tests/test_integration_sync.py`:
- Around line 369-374: The test registers the mock at the wrong endpoint so the
API call in get_device_inventory() isn't intercepted; update the mock in
test_null_inventory_returns_false (the mock_server.register call) to use the
path /api/v0/inventory/1/all (matching get_device_inventory's call to
/api/v0/inventory/{device_id}/all) and keep the same response payload so the
assertion on success being False still runs against the actual mocked request.
In `@netbox_librenms_plugin/tests/test_librenms_id.py`:
- Around line 108-118: The test should assert that get_librenms_device_id(obj,
"default", auto_save=True) not only calls save but calls it with the scoped
update_fields so only custom_field_data is persisted; update the tests around
test_auto_save_true_mutates_bare_string (and the similar case at lines 144-154)
to expect obj.save.assert_called_once_with(update_fields=["custom_field_data"])
instead of just obj.save.assert_called_once(), and ensure the assertion still
verifies obj.custom_field_data["librenms_id"] == 42 and the returned result ==
42 so the behaviour and scoped save contract of get_librenms_device_id are
enforced.
In `@netbox_librenms_plugin/tests/test_utils.py`:
- Around line 419-438: Remove the test that treats a LibreNMS device ID of 0 as
valid: delete or replace the test_zero_id_is_still_valid case so we no longer
simulate or assert on member.cf/custom_field_data returning 0 or patch
get_librenms_device_id to return 0; instead keep tests aligned with real
LibreNMS behavior (IDs start at 1) and ensure get_librenms_sync_device tests
only cover None/missing-ID cases and valid positive IDs when referencing
get_librenms_sync_device, device.virtual_chassis, and vc.members.all.
In `@netbox_librenms_plugin/views/base/librenms_sync_view.py`:
- Around line 115-120: librenms_id_serial_confirmed currently requires a
non-empty NetBox serial which never exists for VirtualMachine, so update the
condition to allow VM targets to bypass the serial-match gate: when computing
librenms_id_serial_confirmed (using _librenms_serial, _netbox_serial, and
_lookup_device), treat the check as true if the librenms serial is present and
either the serials match OR the target is a VM (detect by model name normalized
the same way as ConvertLegacyLibreNMSIdView.post() — e.g., check
_lookup_device._meta.model_name in ("virtualmachine","vm") or replicate that
normalization), so the "Convert ID" button is enabled for VM mappings.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 500-508: Replace the ad-hoc truthiness check for
vc_detection_enabled with the shared helper to ensure consistent parsing: use
the _is_truthy helper (defined near _TRUTHY) instead of
request.POST.get("enable_vc_detection") in ("on", "true", "1", "True"); then
keep sync_options construction as-is, relying on _is_truthy for any other
boolean POST fields if you refactor them later so all truthy checks use the same
logic.
- Around line 1000-1012: The VM action guard currently blocks all actions except
"migrate_librenms_id"; change it to only block device-only actions by
importing/using the _DEVICE_ONLY_ACTIONS set and replacing the condition `if
action != "migrate_librenms_id"` with a check like `if action in
_DEVICE_ONLY_ACTIONS` so that VirtualMachine flows set existing_model to
VirtualMachine (NetBoxVM) but still allow actions such as sync_name,
sync_platform, and migrate_librenms_id; ensure you reference
existing_device_type, action, _DEVICE_ONLY_ACTIONS, existing_model,
VirtualMachine (NetBoxVM), and Device when making the change.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 563-589: The POST handler's serial-empty gate currently blocks
VirtualMachine objects from reaching migrate_legacy_librenms_id(); update the
check in post (which retrieves model,obj via _get_model_and_object) so it only
rejects when the target model is Device and obj.serial is empty (i.e., allow VMs
with empty serial to continue), and ensure any confirmation/permission logic
that treats serial-touching actions as device-only remains unchanged for Device
but is skipped for VirtualMachine.
In `@netbox_librenms_plugin/views/sync/interfaces.py`:
- Around line 103-107: get_cached_ports_data currently reads the cache using the
viewed object's key but the GET path writes ports under the resolved sync
device's key; update get_cached_ports_data to resolve the same sync device as
BaseLibreNMSSyncView.get (the same own-ID guard used there) before building the
cache key: if server_key is None, determine the actual sync device PK used for
syncing (reuse the same logic/function used by BaseLibreNMSSyncView.get or a
helper like get_sync_device_for(obj)), then call self.get_cache_key(obj,
"ports", resolved_server_key) and read from cache so VC members without their
own active-server mapping see the same cached ports as the GET tab.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/sync/cables.py`:
- Around line 98-105: process_single_interface currently copies the posted
device override into link_data.device_id but does not update the cached
netbox_local_interface_id, so handle_cable_creation still uses the original
cached interface/member; update link_data to also override
"netbox_local_interface_id" (and any related local interface identifiers used by
handle_cable_creation) from the incoming interface (e.g., use
interface.get("netbox_local_interface_id",
link_data.get("netbox_local_interface_id"))), then call handle_cable_creation
with the updated link_data and interface to ensure the selected VC member is
used.
---
Duplicate comments:
In @.devcontainer/README.md:
- Line 49: Update the quick-start link so its anchor exactly matches the
rendered slug of the header "### 📡 LibreNMS Server Configuration": open the
README, locate the link text "LibreNMS Server configuration" and replace its
current anchor with the header's actual GitHub-rendered slug (compute the slug
by lowercasing the header, replacing spaces with hyphens and applying GitHub
slug rules—remove/normalize the emoji if necessary) so the link target matches
the "### 📡 LibreNMS Server Configuration" heading.
In `@docs/development/testing.md`:
- Around line 93-94: The example "Integration tests" command only runs
test_integration_sync.py and must include the VC suite; update the command in
testing.md to run both integration tests (test_integration_sync.py and
test_integration_virtual_chassis.py) or use a pattern (e.g., pytest
netbox_librenms_plugin/tests/test_integration_*.py -v) so the virtual chassis
test (test_integration_virtual_chassis.py) is executed alongside
test_integration_sync.py.
In `@docs/usage_tips/custom_field.md`:
- Around line 41-49: Update the "Manually assign a value" section to clarify
that the new format expects a JSON object for multi-server setups and give a
concrete example; specifically mention the librenms_id field should be set as a
JSON object (e.g., {"default": 42} or {"production": 42, "staging": 17}) rather
than a bare integer, so users understand how to enter values when using the new
multi-server mapping format.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 242-245: The dedup key for serial-less VC stacks is being salted
with the local device_id when computing fingerprint (see variables fingerprint,
device_id, member_parts, vc_domain, processed_vc_domains), which causes
different members of the same stack to produce different vc_domain values;
remove device_id from the hash input and compute the fingerprint only from the
stack members (e.g., join a deterministic ordering of member_parts such as
sorted(member_parts) or unique set) so the resulting vc_domain is identical
across all members and processed_vc_domains can suppress duplicates.
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 178-188: The function get_import_device_cache_key currently
requires both device_id and server_key causing existing one-argument calls to
break; revert the signature to include the backward-compatible default by
changing server_key to have a default of "default" (i.e.,
get_import_device_cache_key(device_id, server_key="default")), and ensure
callers that relied on the single-argument behavior continue to work without
modification.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 57-59: The code currently treats a None return from
match_librenms_hardware_to_device_type as a simple miss and continues, which
hides the sentinel used to indicate ambiguous/duplicate DeviceTypeMapping rows;
change callers (the block around chassis_match and the later check at the
location handling matched results) to explicitly test for None first and
surface/handle it (e.g., log or return an ambiguity error) before performing the
normal if not chassis_match["matched"] path — specifically update the usage of
match_librenms_hardware_to_device_type and the conditional logic that checks
matched to distinguish three states: None (ambiguous/multiple), dict with
matched=True, and dict with matched=False.
In `@netbox_librenms_plugin/import_utils/virtual_chassis.py`:
- Around line 367-369: The function create_virtual_chassis_with_members
currently defaults server_key to None which allows silent fallback and
collisions; change its signature to require server_key (remove the = None
default) and do the same for the related function(s) around lines 431-436 (e.g.,
create_virtual_chassis or any helper that persists VC domains) so callers must
pass server_key; update all call sites to supply a server_key (or propagate it
from higher-level functions) and run the provided AST check to find and fix any
remaining 3-argument calls to create_virtual_chassis_with_members.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 547-663: The save path calls applyButtonUpdates() while async
verification may still be running (verifyVlanInGroup updates
vidCssMap/vidMissingMap and row.dataset.resolvedCss), causing stale summary
rendering; fix by awaiting pending verification promises before calling
applyButtonUpdates(): track verification promises for each button/VID when
verifyVlanInGroup is invoked (e.g., store them in a Map or array like
pendingVerifications), ensure the Save handler (the branch that posts or the
else path) awaits Promise.all(pendingVerifications) (or rejects if any fail) and
disable the Save control (modalEl save button) while awaiting so
applyButtonUpdates() sees the final vidCssMap/vidMissingMap state; reference
applyButtonUpdates, verifyVlanInGroup, buttonsToUpdate, vidCssMap,
vidMissingMap, modalEl and pendingVerifications in your changes.
In `@netbox_librenms_plugin/tests/mock_librenms_server.py`:
- Around line 234-240: The handler _handler is only mounted at
f"/api/v0/inventory/{device_id}" but get_device_inventory() expects the
unfiltered fallback at f"/api/v0/inventory/{device_id}/all"; register the same
_handler for the "/all" path as well by adding a second entry to self.routes
keyed with f"/api/v0/inventory/{device_id}/all" (pointing to _handler) so both
routes return the unfiltered inventory.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d18350e5-e7dc-4e2b-8c33-c70a8a254f80
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.devcontainer/README.md.github/pull_request_template.md.pre-commit-config.yamldocs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/jobs.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/vlans.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_background_jobs.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_mixins.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_view_wiring.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/object_sync/vms.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/vlans.pypyproject.toml
marcinpsk
force-pushed
the
pr/librenms-id-multi-server
branch
from
March 12, 2026 09:10
ee465d0 to
8556058
Compare
left a comment
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
netbox_librenms_plugin/views/imports/actions.py (1)
500-508: 🧹 Nitpick | 🔵 TrivialMinor inconsistency in truthy value parsing.
Line 501 uses
in ("on", "true", "1", "True")while_is_truthyat lines 74-75 usesval.lower() in _TRUTHY. Consider using_is_truthyfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/views/imports/actions.py` around lines 500 - 508, Replace the ad-hoc truthy check for VC detection with the existing helper to ensure consistent parsing: change the assignment to vc_detection_enabled to call _is_truthy(request.POST.get("enable_vc_detection")) and keep sync_options construction as-is; locate the code around _resolve_naming_preferences and vc_detection_enabled in actions.py and use the _is_truthy helper (defined near lines 74-75) instead of the inline in(...) check so both VC detection and other flags share the same truthy semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/tests/test_integration_sync.py`:
- Around line 384-390: The test test_empty_devices_list_get_device_info
currently only asserts success is False but must also assert that data is None
per its docstring; update the test to include "assert data is None" after the
existing success assertion so the behavior of api.get_device_info(42) is fully
verified.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1000-1014: The allowlist _VM_SUPPORTED_ACTIONS currently only
contains "migrate_librenms_id" and "sync_name", which blocks valid VM actions;
update the frozenset defined near existing_device_type handling to also include
"sync_platform" so VirtualMachine (imported as VirtualMachine/NetBoxVM) objects
permit sync_platform actions the same way they permit migrate_librenms_id and
sync_name, leaving the rest of the Device branch unchanged.
- Around line 922-930: The legacy fallback is too restrictive: in the block
using servers_config, srv_cfg, sk and plugins_config (where display_name is set)
remove the extra "and not servers_config" check so the condition becomes
"srv_cfg is None and sk == 'default'" — this ensures when the "default" key
exists in cf_value but srv_cfg is missing you fall back to
plugins_config.get('display_name') or sk, matching the reference
_build_all_server_mappings behavior; leave the subsequent type-checking of
srv_cfg and the else-path unchanged.
---
Duplicate comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 500-508: Replace the ad-hoc truthy check for VC detection with the
existing helper to ensure consistent parsing: change the assignment to
vc_detection_enabled to call _is_truthy(request.POST.get("enable_vc_detection"))
and keep sync_options construction as-is; locate the code around
_resolve_naming_preferences and vc_detection_enabled in actions.py and use the
_is_truthy helper (defined near lines 74-75) instead of the inline in(...) check
so both VC detection and other flags share the same truthy semantics.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: af7a1ec3-2ced-4bd5-8293-1bf0734b0063
📒 Files selected for processing (4)
netbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
commented
Mar 12, 2026
Owner
Author
|
Thanks for the thorough review. Below is a consolidated response to all actionable findings, including duplicate/outside-diff comments. Items are grouped by resolution status. Fixed in this PRCommits 8556058, 2fb120d, 1025627:
Already Correct — No Change Needed
Addressed in Follow-up Branch (pr/librems-id-multi-fixes)These are production-valid findings addressed in the immediate follow-up branch (97d752e):
Acknowledged — No Code Change
|
left a comment
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js (1)
1504-1617: 🧹 Nitpick | 🔵 TrivialLGTM with minor suggestion.
The Module Replace implementation correctly handles the critical cancellation scenarios:
- Aborts previous in-flight request when a new Replace button is clicked.
- Aborts request when modal is closed via
closeHtmxModal().- Gracefully ignores
AbortErrorin the catch block.- GET request correctly omits CSRF token per coding guidelines.
Optional cleanup: Consider adding a
.finally()to clear_activeReplaceControllerafter the request settles. This prevents attempting to abort an already-completed request on modal close, though callingabort()on a settled controller is harmless.🧹 Optional cleanup
modalBody.appendChild(alert); } - }); + }) + .finally(() => { + if (_activeReplaceController?.signal === signal) { + _activeReplaceController = null; + } + }); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js` around lines 1504 - 1617, The in-flight AbortController (_activeReplaceController) isn't cleared after the fetch settles; update the fetch promise chain in initializeModuleReplaceButtons (the fetch(...).then(...).catch(...)) to add a .finally() that sets _activeReplaceController = null so the controller is cleared whether the request succeeds, fails, or is aborted; keep closeHtmxModal() as-is (it should still abort if needed) and reference the module-level variable _activeReplaceController when clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/tests/mock_librenms_server.py`:
- Around line 202-208: The mocked inventory_response helper always returns
{"status": "ok", ...} even for non-200 HTTP responses; update inventory_response
(and its call to register) to make the payload reflect the HTTP status: when
status == 200 set the payload's "status" to "ok" (and include "inventory":
items), otherwise set payload's "status" to "error" (and include a "message"
describing the error or pass through the HTTP status) so error-path tests can
reliably distinguish success vs failure; locate inventory_response and the
register invocation to implement this conditional payload change.
- Around line 223-233: The VC inventory branch in _handler currently only checks
entPhysicalContainedIn; update _handler to also read entPhysicalClass =
query.get("entPhysicalClass", [None])[0] and require entPhysicalClass ==
"chassis" before returning child inventory; if entPhysicalClass is present but
not "chassis" return a 404 error (or the same error shape as other bad filters)
so the mock enforces the real query contract when looking up children from the
children dict by contained_in.
In `@netbox_librenms_plugin/tests/test_integration_sync.py`:
- Around line 369-375: Update the test_null_inventory_returns_false test to
assert the diagnostic payload returned by get_device_inventory(1) when inventory
is None contains an error message (not None) describing schema mismatch; locate
the call to api.get_device_inventory in test_null_inventory_returns_false and
replace the loose assertion with checks that success is False and that data is a
string or dict containing an explanatory error (e.g., mentions "inventory" or
"not a list") so callers can reliably detect and propagate schema drift from
get_device_inventory.
In `@netbox_librenms_plugin/tests/test_utils.py`:
- Around line 23-24: Replace the generic Exception used for the mock model
DoesNotExist with a dedicated sentinel exception class so production except
...DoesNotExist handlers only catch that specific case; specifically, in
tests/test_utils.py update the mocks (e.g., mock_dtm.DoesNotExist and other
mock_* .DoesNotExist usages at the blocks around where
mock_dtm.objects.get.side_effect is set) to assign a custom exception class
(class ModelDoesNotExist(Exception): pass) and use that class as the side effect
for objects.get when simulating "not found" scenarios, leaving other
side_effects as real Exceptions so unrelated errors still surface.
---
Duplicate comments:
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js`:
- Around line 1504-1617: The in-flight AbortController
(_activeReplaceController) isn't cleared after the fetch settles; update the
fetch promise chain in initializeModuleReplaceButtons (the
fetch(...).then(...).catch(...)) to add a .finally() that sets
_activeReplaceController = null so the controller is cleared whether the request
succeeds, fails, or is aborted; keep closeHtmxModal() as-is (it should still
abort if needed) and reference the module-level variable
_activeReplaceController when clearing.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67f1eef0-0f94-48b8-9d13-6801d123de33
📒 Files selected for processing (10)
.devcontainer/README.mddocs/development/testing.mddocs/usage_tips/custom_field.mdnetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tests/mock_librenms_server.pynetbox_librenms_plugin/tests/test_integration_sync.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/sync/interfaces.py
Replace inline VC resolution logic in SingleCableVerifyView and SingleInterfaceVerifyView with the centralized get_librenms_sync_device() utility. The inline logic only checked primary_ip, causing a 500 error (AttributeError: 'NoneType' object has no attribute '_meta') when no VC member has a primary_ip set. get_librenms_sync_device() handles this by also checking librenms_id and falling back to lowest vc_position. Added None guards for the edge case where no sync device can be resolved at all. Includes 8 new tests covering both views' VC resolution paths.
commented
Mar 12, 2026
|
Summary of my understanding:
I'll record the key design decisions and "already correct" items in long-term memory to avoid re-raising them. ( ˶ᵔ ᵕ ᵔ˶ ) Thanks for the clean accounting — looking forward to reviewing ✏️ Learnings added
🧠 Learnings used✅ Actions performedComments resolved. Auto-approval is disabled; enable |
left a comment
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 865-880: The device-type comparison block incorrectly runs for
VirtualMachine objects and can flip device_type_synced to False; modify the
logic around librenms_hardware/device_type_synced to skip this whole check when
existing_device is a VirtualMachine (use an isinstance check against the
VirtualMachine model), so for VirtualMachine conflicts you leave
device_type_synced True and do not call match_librenms_hardware_to_device_type;
add the necessary import for VirtualMachine (e.g. from virtualization.models
import VirtualMachine) where used.
- Around line 1091-1099: Before performing link/update flows, reject legacy
bare-int/string LibreNMS IDs by validating the device's
custom_field_data["librenms_id"] and the incoming librenms_id; if the stored or
incoming ID is the legacy bare-int form, return an error response instead of
proceeding. Update the branches around action == "link" (the block that calls
_get_hostname_for_action, set_librenms_device_id(existing_device, librenms_id,
...), assigns existing_device.name/device_type and calls _save_device) to first
call a validation helper (or inline check) that detects legacy mapping formats
and aborts with a clear HTTP error; only if validation passes, call
set_librenms_device_id, update fields and call _save_device.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed8170ff-7a17-4a7c-a745-d90fb65dbd5f
📒 Files selected for processing (2)
netbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/device_fields.py
…lti-server support
left a comment
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py`:
- Around line 19-27: The migration currently uses the default manager and saves
without specifying the migration DB alias; update reads/writes to use
schema_editor.connection.alias: retrieve the model via
CustomField.objects.using(db_alias).get(...) (where db_alias =
schema_editor.connection.alias) and save the instance with
cf.save(update_fields=["type"], using=db_alias); ensure any other ORM access in
this migration (e.g., the check cf.type and assignment) operates against the
same db_alias variable.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ba4823f-757e-486a-98ac-038e5529e7fa
📒 Files selected for processing (1)
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📜 Review details
⏰ 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). (3)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.12)
🧰 Additional context used
📓 Path-based instructions (1)
netbox_librenms_plugin/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
netbox_librenms_plugin/**/*.py: Hook into NetBox Django 5 plugin APIs vianavigation.py,urls.py, andapi/modules undernetbox_librenms_plugin/; respect NetBox plugin conventions
UseLibreNMSAPI.get_librenms_id()instead of directly accessing thelibrenms_idcustom field when mapping Devices/VMs to LibreNMS
Reuselibrenms_api.pyclient for all LibreNMS communication instead of making directrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and caching
Always use exact-only matching (not fuzzy) for site, platform, device type, and role via functions inutils.py(find_matching_site,match_librenms_hardware_to_device_type,find_matching_platform)
Centralize validation state mutation during import usingimport_validation_helpers.pyfor role/cluster/rack assignment, issue removal, and status recalculation
Useget_virtual_chassis_member()for port-to-member mapping andget_librenms_sync_device()for VC priority-based device selection in virtual chassis operations
Files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
🧠 Learnings (14)
📓 Common learnings
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 23
File: netbox_librenms_plugin/import_utils/vm_operations.py:61-69
Timestamp: 2026-03-08T11:35:39.628Z
Learning: In `netbox_librenms_plugin/import_utils/vm_operations.py`, `create_vm_from_librenms` currently writes `custom_field_data={"librenms_id": librenms_device_id}` as a bare-int (legacy format) intentionally. This file is on the `pr/librenms-pre-id-multi-server` pre-PR branch which is a staging branch to reduce the size of the upcoming multi-server PR. The migration to per-server scoping via `set_librenms_device_id(vm, device_id, server_key)` will be done in the subsequent multi-server PR. Do not flag the bare-int librenms_id in `create_vm_from_librenms` as a multi-server bug on this branch.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 23
File: netbox_librenms_plugin/views/base/librenms_sync_view.py:29-36
Timestamp: 2026-03-09T18:18:03.945Z
Learning: In `netbox_librenms_plugin/views/base/librenms_sync_view.py` on branch `pr/librenms-pre-id-multi-server` (PR `#23`), `get_librenms_sync_device()` is called without a `server_key` argument because the parameter did not yet exist on that branch. The full fix — passing `server_key=self.librenms_api.server_key` at all VC resolution points in `librenms_sync_view.py` — is deferred to PR `#246` (`pr/librems-id-multi-fixes`). Do not re-raise the missing `server_key` argument in `get_librenms_sync_device()` calls within PR `#23`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/views/base/cables_view.py:0-0
Timestamp: 2026-03-08T19:06:26.505Z
Learning: In `netbox_librenms_plugin/views/base/cables_view.py`, `get_device_by_id_or_name`, `enrich_local_port`, `enrich_remote_port`, and `process_remote_device` each accept a `server_key=None` parameter, falling back to `self.librenms_api.server_key` only when absent. `_prepare_context` propagates `server_key` to `enrich_links_data`, and `SingleCableVerifyView.post` passes the request-scoped `server_key` (from `data.get("server_key") or self.librenms_api.server_key`) through to `process_remote_device`. This ensures all `_librenms_id_q` calls and interface/device lookups use the same server namespace as the cache entry being verified. The fix was introduced in commit e89d946 and cherry-picked to `pr/librems-id-multi-fixes` as `5b0e8be`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 41
File: netbox_librenms_plugin/views/imports/actions.py:0-0
Timestamp: 2026-03-12T10:41:36.619Z
Learning: In `netbox_librenms_plugin/views/imports/actions.py`, `_VM_SUPPORTED_ACTIONS` (frozenset used to gate VM actions in `DeviceConflictActionView`) includes `sync_platform` in addition to `migrate_librenms_id` and `sync_name`. `sync_platform` is valid for VirtualMachine objects because `VirtualMachine` has a `platform` FK in NetBox. This was confirmed in commit `6ce0108` on PR `#41`. The `sync_platform` action is present in this branch (`pr/librenms-id-multi-server`). Do not omit `sync_platform` from `_VM_SUPPORTED_ACTIONS`.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: .github/pull_request_template.md:1-49
Timestamp: 2026-03-13T12:31:37.665Z
Learning: In `marcinpsk/netbox-librenms-plugin`, `.github/pull_request_template.md` tracks the upstream `develop` version. Modifications to this file are not made within individual feature/fix PRs — any cleanup must go through upstream maintainers. Do not raise review comments or suggestions on `.github/pull_request_template.md` in any PR; flag at most as informational if the file is unexpectedly modified.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:604-612
Timestamp: 2026-03-07T11:09:07.715Z
Learning: In `netbox_librenms_plugin/utils.py`, `find_by_librenms_id(model, librenms_id, server_key)` intentionally merges the server-scoped JSON lookup (`custom_field_data__librenms_id__{server_key}`) and the legacy bare-int/str fallback (`custom_field_data__librenms_id`) into a single OR Q object and calls `.first()`. This is correct and intentional because: (1) librenms_id values are unique per LibreNMS device, so two different DB objects cannot hold the same ID in different formats simultaneously; (2) the migration workflow converts bare-int to `{server_key: id}` atomically, preventing a state where both forms coexist on separate objects; (3) splitting into two sequential queries would double DB hits on a hot path called for every port during cable enrichment; (4) string normalization is already covered by the `str(librenms_id)` variants on the legacy branches. Do not suggest splitting this into a two-step lookup.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 41
File: netbox_librenms_plugin/tests/test_utils.py:0-0
Timestamp: 2026-03-12T09:57:04.607Z
Learning: In `netbox_librenms_plugin/utils.py`, inside `get_librenms_sync_device`, the return value of `get_librenms_device_id(member, server_key, auto_save=False)` is evaluated with a bare truthy check (`if result:`) rather than `if result is not None:`. This intentionally skips device_id=0, which is not a valid LibreNMS ID (MySQL auto-increment starts at 1). Fixed in commit 2fb120d on PR `#41`. Do not revert this to an `is not None` guard.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_device_fields.py:1503-1533
Timestamp: 2026-03-09T20:15:11.480Z
Learning: In `netbox_librenms_plugin/tests/test_coverage_device_fields.py`, the `ConvertLegacyLibreNMSIdView` post() tests (e.g. `test_virtualmachine_object_type_normalised` and related tests in `TestConvertLegacyLibreNMSIdViewPost`) patch `find_by_librenms_id` and `migrate_legacy_librenms_id` but do not assert that these helpers are called with `server_key=view._librenms_api.server_key`. Strengthening these assertions to verify the active server namespace is propagated into both migration helpers is a known deferred improvement tracked in the test backlog (low priority). Do not re-raise the missing server_key call-arg assertion on these tests as a new finding.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:44.412Z
Learning: In netbox_librenms_plugin, `set_librenms_device_id` is used for both Device/VM objects (to store the LibreNMS device ID) and Interface/VMInterface objects (to store the LibreNMS port ID). The legacy bare-int guard in `set_librenms_device_id` is only relevant for Device/VM objects where a pre-existing bare integer might exist from before multi-server support. For Interface/VMInterface objects, the `librenms_id` custom field starts empty (null/{}) because `port_id` is always freshly written from the LibreNMS API JSON response; there is no legacy bare-int migration concern for interfaces. Do not flag the warning-log path on interfaces as a silent no-op bug.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:573-588
Timestamp: 2026-03-07T13:14:12.447Z
Learning: In `netbox_librenms_plugin/views/sync/device_fields.py`, `ConvertLegacyLibreNMSIdView.post()` intentionally uses `self.librenms_api.server_key` (the active server from the global plugin setting) rather than reading `server_key` from `request.POST`. The "Convert Legacy ID" form template (`librenms_sync_base.html`, lines ~160-175) only submits `object_type` and CSRF token — no `server_key` field — because conversion always targets the currently active server the user is viewing. Serial verification via `get_device_info()` also correctly queries the active server. Do not flag this as a missing or wrong server_key; the active server context is the correct and only sensible target for the legacy-ID conversion.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:13:04.691Z
Learning: In `netbox_librenms_plugin/views/base/librenms_sync_view.py`, `BaseLibreNMSSyncView.get()` resolves the active LibreNMS server exclusively through `self.librenms_api.server_key`, which comes from the global plugin setting `settings.selected_server`. It does NOT read `?server_key=` from `request.GET`. Therefore, redirects in views such as `RemoveServerMappingView` and `ConvertLegacyLibreNMSIdView` (in `views/sync/device_fields.py`) do not need to append `server_key` as a query parameter — the server context is preserved across requests through the global setting. Do not flag these redirects as dropping the server context.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T13:05:27.529Z
Learning: In `netbox_librenms_plugin/views/base/ip_addresses_view.py`, `_prefetch_netbox_data` already reads `server_key` from `self.librenms_api.server_key` and passes it to `get_librenms_device_id` when building the `interfaces_by_librenms_id` map. Do not flag `enrich_ip_data` or `_prefetch_netbox_data` as missing server context — per-server interface ID lookups are already handled correctly without needing additional server_key threading through the call chain.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-07T12:23:02.486Z
Learning: In `netbox_librenms_plugin/views/base/interfaces_view.py`, `get_context_data()` calls `self.get_table()` which dispatches polymorphically to concrete overrides. Both the device override (lines ~91-109 in `views/object_sync/devices.py`) and the VM override (lines ~54 in `views/object_sync/vms.py`) already pass `server_key=self.librenms_api.server_key` to all table constructors (LibreNMSInterfaceTable, VCInterfaceTable, LibreNMSVMInterfaceTable). Do not flag the base `get_context_data()` as failing to propagate server_key into tables — the concrete implementations already handle this correctly.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:439-447
Timestamp: 2026-03-06T18:41:13.852Z
Learning: In netbox_librenms_plugin/views/sync/device_fields.py, `RemoveServerMappingView._normalize_librenms_mapping` is intentionally a local helper that converts any raw librenms_id CF value (int, numeric str, dict) into a full `{server_key: device_id}` dict for membership checks and key deletion. This is distinct from utils.py helpers (`get_librenms_device_id`, `set_librenms_device_id`, `migrate_legacy_librenms_id`, `find_by_librenms_id`) which all operate on a single server key; none return the full mapping dict. Do not flag `_normalize_librenms_mapping` as duplication of the utils layer.
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/tests/test_coverage_sync_view.py:77-100
Timestamp: 2026-03-08T23:14:41.515Z
Learning: In `netbox_librenms_plugin/views/base/librenms_sync_view.py`, `BaseLibreNMSSyncView.get()` now contains an explicit own-ID guard for VC members: if the viewed VC member already has a `librenms_id` for the active server (via `get_librenms_device_id(obj, server_key, auto_save=False)` returning a non-None value — covering both legacy bare-int and per-server dict formats), it stays as `self._librenms_lookup_device` without calling `get_librenms_sync_device()`. Only when the member has no own ID does it fall through to `get_librenms_sync_device(obj, server_key=self.librenms_api.server_key)`. This guard prevents auto-discovery side-effects on members that already have an explicit mapping. Do not revert this guard or suggest making delegation unconditional.
📚 Learning: 2026-03-09T19:16:08.084Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:0-0
Timestamp: 2026-03-09T19:16:08.084Z
Learning: In `netbox_librenms_plugin/utils.py`, the `auto_save` branch inside `get_librenms_device_id()` must call `obj.save(update_fields=["custom_field_data"])` (not a bare `obj.save()`) so that normalising a string-stored librenms_id to int only writes the custom_field_data column and does not accidentally persist unrelated dirty fields on the object. Fixed in commit 88b225d.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-12T23:45:47.421Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: :0-0
Timestamp: 2026-03-12T23:45:47.421Z
Learning: Migration 0009 (netbox_librenms_plugin/migrations/0009_inventory_models.py) uses schema_editor.connection.alias and .using(db_alias) when seeding/deleting default rules to support multi-database setups.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-08T11:35:39.628Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 23
File: netbox_librenms_plugin/import_utils/vm_operations.py:61-69
Timestamp: 2026-03-08T11:35:39.628Z
Learning: In `netbox_librenms_plugin/import_utils/vm_operations.py`, `create_vm_from_librenms` currently writes `custom_field_data={"librenms_id": librenms_device_id}` as a bare-int (legacy format) intentionally. This file is on the `pr/librenms-pre-id-multi-server` pre-PR branch which is a staging branch to reduce the size of the upcoming multi-server PR. The migration to per-server scoping via `set_librenms_device_id(vm, device_id, server_key)` will be done in the subsequent multi-server PR. Do not flag the bare-int librenms_id in `create_vm_from_librenms` as a multi-server bug on this branch.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-07T11:09:07.715Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:604-612
Timestamp: 2026-03-07T11:09:07.715Z
Learning: In `netbox_librenms_plugin/utils.py`, `find_by_librenms_id(model, librenms_id, server_key)` intentionally merges the server-scoped JSON lookup (`custom_field_data__librenms_id__{server_key}`) and the legacy bare-int/str fallback (`custom_field_data__librenms_id`) into a single OR Q object and calls `.first()`. This is correct and intentional because: (1) librenms_id values are unique per LibreNMS device, so two different DB objects cannot hold the same ID in different formats simultaneously; (2) the migration workflow converts bare-int to `{server_key: id}` atomically, preventing a state where both forms coexist on separate objects; (3) splitting into two sequential queries would double DB hits on a hot path called for every port during cable enrichment; (4) string normalization is already covered by the `str(librenms_id)` variants on the legacy branches. Do not suggest splitting this into a two-step lookup.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-03T13:23:33.731Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-03T13:23:33.731Z
Learning: Applies to netbox_librenms_plugin/models.py : Coordinate schema changes through Django migrations in `migrations/` directory; update `models.py`, admin, and Pydantic representations accordingly
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-07T10:40:44.412Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:44.412Z
Learning: In netbox_librenms_plugin, `set_librenms_device_id` is used for both Device/VM objects (to store the LibreNMS device ID) and Interface/VMInterface objects (to store the LibreNMS port ID). The legacy bare-int guard in `set_librenms_device_id` is only relevant for Device/VM objects where a pre-existing bare integer might exist from before multi-server support. For Interface/VMInterface objects, the `librenms_id` custom field starts empty (null/{}) because `port_id` is always freshly written from the LibreNMS API JSON response; there is no legacy bare-int migration concern for interfaces. Do not flag the warning-log path on interfaces as a silent no-op bug.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-03T13:23:33.731Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-03T13:23:33.731Z
Learning: Applies to netbox_librenms_plugin/**/*.py : Use `LibreNMSAPI.get_librenms_id()` instead of directly accessing the `librenms_id` custom field when mapping Devices/VMs to LibreNMS
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-07T10:33:35.311Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:22-39
Timestamp: 2026-03-07T10:33:35.311Z
Learning: In `netbox_librenms_plugin/views/base/cables_view.py`, `_librenms_id_q(server_key, value)` intentionally combines `custom_field_data__librenms_id__{server_key}` (server-scoped JSON) and `custom_field_data__librenms_id` (legacy bare-int/str) in a single OR Q object. This is correct and safe because all callers scope queries to a specific device's interfaces (`obj.interfaces.filter(...)` or `chassis_member.interfaces.filter(...)`), so cross-object collisions cannot occur. The two formats represent the same logical identity stored differently (legacy vs. migrated). Do not suggest splitting this into a two-step lookup — it would add unnecessary DB round-trips for no correctness benefit. The same OR pattern is used in `find_by_librenms_id` in `utils.py`.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-07T12:29:43.766Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/imports/actions.py:1283-1295
Timestamp: 2026-03-07T12:29:43.766Z
Learning: In `netbox_librenms_plugin/views/imports/actions.py`, the `migrate_librenms_id` action's conflict check uses `find_by_librenms_id(existing_model, cf_locked_int, server_key)` (not a hand-rolled Q) to detect whether another object of the same model already owns the ID before migrating. Cross-model conflict checks (Device vs VirtualMachine) are intentionally not performed here because migration only converts storage format (bare int → {server_key: id}) on the same object; it does not change associations across models. Do not suggest adding cross-model conflict checks to the migrate_librenms_id action.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-08T08:55:52.594Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:52.594Z
Learning: In `netbox_librenms_plugin/utils.py`, `set_librenms_device_id(obj, device_id, server_key)` is intentionally a mutate-only helper — it updates `obj.custom_field_data["librenms_id"]` in-memory but never calls `obj.save()`. Every caller is responsible for persistence: `device_operations.py` (around line 864-866) follows the pattern `set_librenms_device_id → full_clean() → save()` so that NetBox validation runs after mutation but before persistence; `librenms_api.py` (around line 261-262) calls `set_librenms_device_id` then `obj.save()` explicitly. Do not suggest adding `obj.save()` inside `set_librenms_device_id` — it would break the `full_clean()` → `save()` pattern and couple persistence with mutation.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-06T18:41:13.852Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:439-447
Timestamp: 2026-03-06T18:41:13.852Z
Learning: In netbox_librenms_plugin/views/sync/device_fields.py, `RemoveServerMappingView._normalize_librenms_mapping` is intentionally a local helper that converts any raw librenms_id CF value (int, numeric str, dict) into a full `{server_key: device_id}` dict for membership checks and key deletion. This is distinct from utils.py helpers (`get_librenms_device_id`, `set_librenms_device_id`, `migrate_legacy_librenms_id`, `find_by_librenms_id`) which all operate on a single server key; none return the full mapping dict. Do not flag `_normalize_librenms_mapping` as duplication of the utils layer.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py
🔇 Additional comments (1)
netbox_librenms_plugin/migrations/0009_convert_librenms_id_to_json.py (1)
20-25: Good defensive no-op when the custom field is absent.Line 22-24 and Line 34-35 correctly keep the migration idempotent for environments where
librenms_idis not created yet.Also applies to: 32-35
…nst existing JSON values
Closed
This was referenced Mar 26, 2026
This was referenced May 16, 2026
Merged
Merged
Closed
This was referenced Jun 12, 2026
Open
Merged
This was referenced Jun 19, 2026
Merged
Merged
This was referenced Jul 7, 2026
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
2 participants
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.
Summary
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Delete items that don’t apply:
How Was This Tested?
Delete items that don’t apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Improvements
Documentation & Tests